elixir-lang / elixir

Elixir is a dynamic, functional language for building scalable and maintainable applications
https://elixir-lang.org/
Apache License 2.0
24.37k stars 3.36k forks source link

generated meta is not respected by new type system #13727

Closed hissssst closed 2 months ago

hissssst commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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