Skip to content

Conversation

@tsubery
Copy link
Contributor

@tsubery tsubery commented Dec 27, 2024

Many typespecs did not exactly match the underlying implementation. For example. File.read can throw an error atom that is not defined inside File.posix() type.

iex(2)> File.read("\x00")
{:error, :badarg}

This change does not have the expected effect of generating a warning on patterns that are unable to match such as

{:error, :non_existing_error} = File.read("path")

Because dialyzer is treating any union type with more than 13 members as the common type. When using File.posix() any atom is considered to be acceptable by dialyzer.
Even though type checking passes I think it makes sense to add the full specification in order to have complete documentation and make sure any implementation of type checking would be compatible.

The code is a bit repetitive because every instance of posix() is followed by :badarg. We could make a new type to encompass posix() + :badarg but it might be better to keep it the same as OTP implementation.

Somewhat related questions:
I noticed we don't have a matching implementation for the third options of copy function that uses a tuple of {filename :: binary, mode()} as arguments. Is it intentional?

Another question, since we wrap many of :file module functions in Elixir, does it make sense to wrap :file.format_error too and return a binary instead of char list?

@josevalim
Copy link
Member

I noticed we don't have a matching implementation for the third options of copy function that uses a tuple of {filename :: binary, mode()} as arguments. Is it intentional?

It is intentional in the sense we haven't had a use case for it thus far. :)

Another question, since we wrap many of :file module functions in Elixir, does it make sense to wrap :file.format_error too and return a binary instead of char list?

I wouldn't say there is a need but we should document you can call :file.format_error/1 if you need a string reason (probably document it in the @type posix declaration? You can of course get a string reason by using the bang variants too.

@tsubery
Copy link
Contributor Author

tsubery commented Dec 27, 2024

The documentation for File.read mentions it, even though it's common to other functions too.

                                 def read(path)                                 

  @spec read(Path.t()) :: {:ok, binary()} | {:error, posix()}

Returns {:ok, binary}, where binary is a binary data object that contains the
contents of path, or {:error, reason} if an error occurs.

Typical error reasons:

  • :enoent  - the file does not exist
  • :eacces  - missing permission for reading the file, or for searching
    one of the parent directories
  • :eisdir  - the named file is a directory
  • :enotdir - a component of the file name is not a directory; on some
    platforms, :enoent is returned instead
  • :enomem  - there is not enough memory for the contents of the file

You can use :file.format_error/1 to get a descriptive string of the error.

A similar explanation is repeated in a few places. A documentation for the type posix seems much better.

@josevalim
Copy link
Member

I believe it is because not all functions will return all posix codes. Which will probably need a specific distinction in the future anyway.

@josevalim josevalim merged commit 71b8529 into elixir-lang:main Dec 27, 2024
9 checks passed
@josevalim
Copy link
Member

💚 💙 💜 💛 ❤️

michallepicki added a commit to michallepicki/elixir that referenced this pull request Dec 27, 2024
josevalim pushed a commit that referenced this pull request Dec 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants