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

Improve some OTP type specs #39

Closed
josefs opened this issue Oct 3, 2018 · 11 comments
Closed

Improve some OTP type specs #39

josefs opened this issue Oct 3, 2018 · 11 comments

Comments

@josefs
Copy link
Owner

josefs commented Oct 3, 2018

There are modules in OTP which have really unfortunate type specs, from the point of view of Gradualizer. Take the function hd/1 for example. It has the following spec:

hd([term()]) -> term()

In Gradualizer, the type term() is the top of the subtyping hierarchy, which means that the code that consumes the output of hd/1 has to account for every kind of value there is! In practice it means that all uses of this function will signal a type error. That's no good! A better type would be:

hd([any()]) -> any()

One way to solve this problem is to provide our own type specs for problematic modules in OTP. We could instrument gradualizer_db to use its own set of specs instead of these modules.

A non-exhaustive list of problematic OTP modules

  • lists
  • proplists
  • maps
  • erlang
  • mnesia
  • ets
  • qlc
@zuiderkwast
Copy link
Collaborator

The long-term solution would be to update these in OTP with more specific types such as hd([A]) -> A. It would not impossible to get such a PR accepted, I think.

@josefs
Copy link
Owner Author

josefs commented Oct 4, 2018

@zuiderkwast, I agree. We should absolutely try to fix the types on OTP. The plan I outlined above is the short- to medium term solution.

@josefs josefs added this to the Beta release milestone Oct 5, 2018
@jachto
Copy link

jachto commented Oct 9, 2018

Where comes the otp code/types into Gradualizer?

@KennethL
Copy link

KennethL commented Oct 9, 2018

I encourage you to make a pull request with the specs you want to improve in OTP. Preferrably one per OTP application.
I think we will accept most of them without problem. For example hd([A]) -> A is something we have thought of as an improvement anyway.

@josefs
Copy link
Owner Author

josefs commented Oct 10, 2018

@jachto, the Gradualizer doesn't have any special mechanism for importing types from OTP. The file gradualizer_db handles the lookup of types in other modules, and if you want to dig into the details you can start with the function import_module/2.

@josefs
Copy link
Owner Author

josefs commented Oct 10, 2018

Thanks for the encouragement, @KennethL! It's good to know that you're open to improving the specs. We'll make sure to submit pull request in due course.

@tim2CF
Copy link
Contributor

tim2CF commented Nov 14, 2018

Hello! I think I have similar problems with standard Elixir libraries. Let's say I have a function

  @spec foo :: non_neg_integer()
  def foo do
    [:bar, :buz]
    |> Enum.count
  end

Then Gradualizer will say

The expression [bar,buz] on line 0 does not have type t()

Where actually t() is Enumerable.t() which is term(). What will be the best solution here? To create ./priv/elixir_spec_fix.erl? Or maybe we should develop mechanism to override types locally in context of project where Gradualizer is executed for type checks?

@gomoripeti
Copy link
Collaborator

hi @tim2CF
I think in the current example it should be a Gradualizer issue (why the list [:bar, :baz] is not a subtype of term())

But in general good point about overriding specs. Technically it should be no problem to override an Elixir standard lib spec even in otp_spec_fix.erl, although need to use Erlang syntax, eg

-spec 'Elixir.Enum':map('Elixir.Enumerable':t(), fun((any()) -> T) ) :: [T].

(Would be nice to have separation and store Elixir overrides in a separate module, even maybe in Elixir syntax)
It is also a good idea to allow user-specific spec files in the same format, this can be an additional (rebar3/mix) plugin option.

@zuiderkwast
Copy link
Collaborator

I consider this issue Done. Note that it contains "some" in the title.

This means that we have reached the goals for a beta version, so let's take 0.1.0! Shall we? (Before we make any new major changes...)

@josefs
Copy link
Owner Author

josefs commented Oct 20, 2019

I wouldn't be opposed to a 0.1.0 version. But I think it's important to point out that polymorphism doesn't work yet.

@zuiderkwast
Copy link
Collaborator

Sounds good! Let's finish up stuff in #43, e.g. describing what works and what not.

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

No branches or pull requests

6 participants