Skip to content

generated meta is not respected by new type system #13727

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
hissssst opened this issue Jul 21, 2024 · 6 comments
Closed

generated meta is not respected by new type system #13727

hissssst opened this issue Jul 21, 2024 · 6 comments

Comments

@hissssst
Copy link
Contributor

hissssst commented Jul 21, 2024

Elixir and Erlang/OTP versions

Erlang/OTP 27 [erts-15.0] [source] [64-bit] [smp:8:8] [ds:8:8:10] [async-threads:1] [jit:ns]

Elixir 1.17.1 (compiled with Erlang/OTP 27)

Operating system

linux

Current behavior

defmodule Xxx do
  defmacro f(x) do
    quote generated: true do
      unquote(x) < 2
    end
  end

  def g do
    f(:x)
  end
end
Compiling 1 file (.ex)
    warning: comparison between incompatible types found:

        :x < 2

    While Elixir can compare across all types, you are comparing across types which are always distinct, and the result is either always true or always false

    typing violation found at:
    │
  9 │     f(:x)
    │     ~~~~~
    │
    └─ lib/xxx.ex:9: Xxx.g/0

Expected behavior

No warning produced

@hissssst
Copy link
Contributor Author

hissssst commented Jul 21, 2024

This code generates warning too

defmodule Xxx do
  defmacro f(x) do
    quote generated: true do
      unquote(x) < 2
    end
    |> Macro.prewalk(&Macro.update_meta(&1, fn x -> Keyword.put(x, :generated, true) end))
  end

  def g do
    f(:x)
  end
end

@hissssst
Copy link
Contributor Author

This makes usage of any not type-correct macro broken with --warnings-as-errors which is a default for building in most production environments

@sabiwara
Copy link
Contributor

As discussed here, this has been a conscious decision to not ignore generated code yet and to delay its implementation.

But I agree that while most type errors should have no false positives, this example sounds like one.

@josevalim
Copy link
Member

Thanks for the context @sabiwara, although I think there is no false positive here? @hissssst, can you please expand the context this is used? Would it be possible to address this by compiling code conditionally?

@hissssst
Copy link
Contributor Author

hissssst commented Jul 28, 2024

This library https://hexdocs.pm/pathex/readme.html generates lenses from paths like view!(..., path(a / b / c)) will work like get_in(..., [a, b, c]), but without requiring Access behaviour and times faster than Access or get_in does.

Actually path(a / b / c) generates fn with a bunch of nested case clauses like

case x do
  %{^a => y} ->
    case y do
      %{^b => z} ->
        ...
    end

  _ when is_list(x) and is_integer(a) and a >= 0 ->
    at_index(x, a)
    
  _ when is_list(x) and is_integer(a) and a < 0 ->
    at_negative_index(x, a)
    
  ... # other clauses for tuples and keywords
end

Pathex is capable of generation of paths for negative indexes

index = -2
result = force_set!([0, 1, 2, 3, 4, 5, 6, 7], path(index), 123)
assert result == [0, 1, 2, 3, 4, 5, 123, 7]

But on the moment of macro expansion, it is unable to detect the type of index variable. But it still generates a case where this index can be a key in map/keyword or index in tuple/list

Therefore code like this

key = :x
result = get(%{x: 123}, path(key))
assert result == 123

will emit a warning, because path will generate a case which performs key < 0 comparison, which is a comparison of different types which is (highly questionably, imo) considered a type error by the compiler.

@josevalim
Copy link
Member

Thanks. Agreed those cases cannot be detected. We will start checking :generated soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants