elixir-lang/elixir

Opaqueness warning on OTP28

Closed this issue · 10 comments

Elixir and Erlang/OTP versions

1.18.4 and v1.19 branches, OTP28

Operating system

Current behavior

  @base_uri URI.new!("https://example.com")
  def foo_uri do
    @base_uri |> URI.append_path("/foo")
  end
Function call without opaqueness type mismatch.

Call does not have expected term of type %URI{
  :authority => URI.authority(),
  :fragment => nil | binary(),
  :host => nil | binary(),
  :path => nil | binary(),
  :port => nil | char(),
  :query => nil | binary(),
  :scheme => nil | binary(),
  :userinfo => nil | binary()
} (with opaque subterms) in the 1st position.

URI.append_path(
  %URI{
    :authority => nil,
    :fragment => nil,
    :host => <<103, 111, 111, 103, 108, 101, 46, 99, 97>>,
    :path => nil,
    :port => 443,
    :query => nil,
    :scheme => <<104, 116, 116, 112, 115>>,
    :userinfo => nil
  },
  <<47, 102, 111, 111>>
)

Expected behavior

No warning.

This is exactly like #14576 all over again, but for module attributes rather than inline calls 😭

We could:

  1. mark as generated after escaping (again, reducing the usefulness of the type system :/)
  2. remove opaqueness as previously suggested - this should eventually be checked via the new type system anyway?
  3. do nothing and require people to locally silent warning in such cases
  4. disable dialyzer opaqueness checks as previously mentioned - not sure at which level though?

Originally reported here

In this case we can remove the opaqueness of the authority field. It was not opaque before, it became opaque with the deprecation, but we can keep it public and deprecated.

Although the issue will still happen with MapSet and others... could we have the internal parts of the MapSet as private? Or will that still be an issue?

@mapset MapSet.new(1..3)
def foo, do: MapSet.member?(@mapset, 1)
lib/repro.ex:75:call_without_opaque
Function call without opaqueness type mismatch.

Call does not have expected term of type %MapSet{:map => MapSet.internal(_)} (with opaque subterms) in the 1st position.

MapSet.member?(%MapSet{:map => %{1 => [], 2 => [], 3 => []}}, 1)

😢

Maybe we should remove all opaques from the stdlib for now, just document it with words, and wait for the type system to enforce it in a more reliable way?

@sabiwara the above happens with @typep as well?

Actually the :sets.set(value) type used internally is itself opaque.
Just changing to typep doesn't cut it.

Gah, so I guess we need to make it as generated indeed? Probably as part of Macro.escape/elixir_quote.escape.

I’m worried that we’re making the type system blind by going this way. We’re trading the potential to catch real issues for these opaqueness checks, I’m not sure it’s worth it.
The previous inlining one had the same issue :/

I am not worried about this because the generated will apply only for the generated structure, not any operations you do with it. For example:

1 + {:__block__, [generated: true], ["foo"]}

Should still emit a violation.

Good point, it seems to have less of an impact than I expected.

For function calls, it still catches the no_return but doesn't explain why anymore. But the new type system is usually able to explain instead.

Example 1: Regex.match?(@base_uri, "foo")

We still get this warning:

lib/repro.ex:73:7:no_return
Function foo/0 has no local return.

but this one disappears if @base_uri is generated:

lib/repro.ex:74:call
The function call will not succeed.

Regex.match?(
  %URI{
    :authority => nil,
    :host => <<103, 111, 111, 103, 108, 101, 46, 99, 97>>,
    ...
  <<>>
)

will never return since the success typing is:
(%Regex{:opts => _, :re_pattern => _, :re_version => _, :source => _, _ => _}, binary()) ::
  boolean()

and the contract is
(t(), String.t()) :: boolean()

It's fine though because we get the Elixir type system warning: warning: incompatible types given to Regex.match?/2:.

Example 2: Date.to_erl(@base_uri)

We just get the no_return, but dialyzer's "The function call will not succeed." disappears, and the Elixir type system doesn't warn yet.

It seems that the generated flag is the way to go indeed, especially because when thinking about the Elixir type system, if we introduce a similar sort of opaqueness, we'll need to use the same type of annotations as well.