Skip to content
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

Dialyzer errors with File.stream changes in Elixir 1.16 #13212

Closed
nathany-copia opened this issue Dec 28, 2023 · 4 comments
Closed

Dialyzer errors with File.stream changes in Elixir 1.16 #13212

nathany-copia opened this issue Dec 28, 2023 · 4 comments

Comments

@nathany-copia
Copy link

nathany-copia commented Dec 28, 2023

Elixir and Erlang/OTP versions

Erlang/OTP 26 [erts-14.2.1] [source] [64-bit] [smp:10:10] [ds:10:10:10] [async-threads:1] [jit] [dtrace]

Elixir 1.16.0 (compiled with Erlang/OTP 26)

(Erlang 26.2.1)

Operating system

macOS 14.2.1

Current behavior

Deprecate File.stream!(file, options, line_or_bytes) in favor of keeping the options as last argument, as in File.stream!(file, line_or_bytes, options)

The File.stream deprecation in Elixir 1.16.0 appears to be causing Dialyzer errors in libraries like Tesla:
elixir-tesla/tesla#643

lib/tesla/multipart.ex:94: Function add_file/2 has no local return
lib/tesla/multipart.ex:94: Function add_file/3 has no local return
lib/tesla/multipart.ex:106: The call 'Elixir.File':'stream!'(_path@1::binary() | maybe_improper_list(binary() | maybe_improper_list(any(),binary() | []) | char(),binary() | []),[],2048) breaks the contract ('Elixir.Path':t(),'line' | pos_integer(),[stream_mode()]) -> 'Elixir.File.Stream':t()

I'm not sure if there is a problem with the spec for File.stream? The "no local return" error is bubbling up to the callers (but without the additional details until figuring out that the Dialyzer error originates from File.stream in Tesla). This may be more widespread than Tesla, as any code using File.stream may be impacted.

Changing to the new File.stream/2 syntax resolves the Dialyzer error, but also appears to require Elixir 1.16.0, which a library may or may not want to do right away.

As a soft deprecation, code I've ran tests on continues to pass, but dialyzer is failing CI in multiple projects.

Expected behavior

No errors from Dialyzer.

It would be nice if the File.stream type spec didn't report an error when using the deprecated function signature. That way users could move to the new function signature at their own pace.

Apologies for not reporting this earlier. I was running my test suite on the release candidates, but I didn't try Dialyzer with Elixir 1.16 until today.

@jhogberg
Copy link

dialyzer maintainer here. The spec has been changed as if the old behavior was completely removed, even though it was just deprecated and the old stuff still works.

The spec has to support both uses for dialyzer to be happy with this.

@josevalim
Copy link
Member

Correct. When we deprecate an API, our typespecs reflect the latest version of said API. This is to avoid confusion when folks use typespecs as a reference/docs. We may change this policy (this happen every once in a while) but, at the moment, this is intentional. Thank you!

@josevalim
Copy link
Member

Also thank you for trying the RCs and thank you @jhogberg for the additional context!

@nathany-copia
Copy link
Author

This is to avoid confusion when folks use typespecs as a reference/docs.

That makes a lot of sense. 👍🏻

In the case of Tesla, the workaround we're implementing is as follows. We don't want to jump to requiring Elixir 1.16, but we do want to make Dialyzer happy. Leaving this here for anyone searching for the issue for other libraries:

  if Version.compare(System.version(), "1.16.0") in [:gt, :eq] do
    defp stream_file!(path, bytes), do: File.stream!(path, bytes)
  else
    defp stream_file!(path, bytes), do: File.stream!(path, [], bytes)
  end

nathany-copia added a commit to copia-wealth-studios/ex_aws_s3 that referenced this issue Dec 28, 2023
The following Dialyzer errors show up in Elixir 1.16, which then result in “has no local return” Dialyzer errors in clients of this library. This PR resolves them in a backwards compatible way.

```
lib/ex_aws/s3/upload.ex:70: Function stream_file/1 has no local return
lib/ex_aws/s3/upload.ex:70: Function stream_file/2 has no local return
lib/ex_aws/s3/upload.ex:71: The call 'Elixir.File':'stream!'(_path@1::any(),[],any()) breaks the contract ('Elixir.Path':t(),'line' | pos_integer(),[stream_mode()]) -> 'Elixir.File.Stream':t()
```

ref: elixir-lang/elixir#13212
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