Allow overriding types for other modules in prelude

[zuiderkwast]

With this change, we're also not using the remote spec syntax
which is not supported in OTP anymore.

[codecov-io]

Codecov Report

Merging #193 into master will decrease coverage by 0.37%.
The diff coverage is 28.57%.

@@            Coverage Diff             @@
##           master     #193      +/-   ##
==========================================
- Coverage   84.68%   84.31%   -0.38%     
==========================================
  Files          13       13              
  Lines        2253     2263      +10     
==========================================
  Hits         1908     1908              
- Misses        345      355      +10

Impacted Files	Coverage Δ
src/gradualizer_prelude_parse_trans.erl	0% <0%> (ø)	⬆️
src/gradualizer_db.erl	71.49% <100%> (-0.21%)	⬇️
src/typechecker.erl	85.71% <0%> (-0.06%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 58efb1c...5e7e592. Read the comment docs.

[zuiderkwast]

This is the follow-up to #191, as suggested by @gomoripeti in #187 (comment).

I used the syntax -override_module(Module), since it's not only specs anymore but also types and records. One known problem was fixed.

[gomoripeti]

great job 👍

[gomoripeti]

hah, nice

[josefs]

Having thought about this a little more, I'm not a huge fan of the file format here. Each spec cannot be understood in isolation. In order to understand what module a spec belongs to we have to work our way backwards in the file until we get to an override_module directive. This makes the file format stateful in a way that I think is unnecessary.
@Zalastax had a suggestion in #187 to rewrite the specs as atoms so erlang:apply would become 'erlang:apply'. I think that's clearer and more preferable. Each spec can be understood in isolation that way.

[zuiderkwast]

That's a good point @josefs.

The 'module:name' syntax would be a bit strange for overriding internal types though, e.g. deep_list below, a type which is not exported from lists.

-type 'lists:deep_list'(A) :: [A | deep_list(A)].
-spec 'lists:flatten'(deep_list(A))      -> [A].

Here, because of the 'lists:deep_list' representation of the name, these definitions would be treated as part of the lists module, so in the RHS, simply deep_lists is used. Also in the spec below, deep_lists is used without module prefix. This looks a bit strange. The alternative would be to write 'lists:deep_list' everywhere...

-type 'lists:deep_list'(A) :: [A | 'lists:deep_list'(A)].
-spec 'lists:flatten'('lists:deep_list'(A))      -> [A].

...but then we'd obviously have to handle this syntax in a lot more places.

The beauty of the -override_type(lists) syntax IMO is that it's clear that the code below this attribute is simply taken as part of the lists module, unchanged. (It was also easy to implement.)

---

An alternative would be to have one file per module we want to override, e.g. priv/overrides/lists.erl. I would put them under priv so that rebar doesn't think the're erl files to be compiled. Instead we can read these files in the prelude parse transform and let them be returned by the prelude module.

[josefs]

I like the idea of having one file per overridden module. We've already talked about splitting up this module anyway so that seems like a really nice solution actually!

[gomoripeti]

One more thing we could consider when defining the format of the override file is that in the future users could also define their own override files (eg for 3rd party libs). In that case I think its more convenient to allow a single file to override let's say 3 specs each from 3 different libs. Similar to a single dialyzer.ignore file. In that example it would be a bit less convenient to create 3 files for 3 modules each with only 1 function.

But I also agree the prelude file grew too large so it makes sense to split it up and support multiple override files.

[josefs]

@gomoripeti yes, if we allow users to override libraries in the future it'd be somewhat less convenient to have one file per overridden module. Are you arguing that this inconvenience is a killer and that we should allow multiple overridden modules per file?
For me it doesn't weigh particularly heavy and I think the problems that are solved by having multiple files clearly wins out in this case.

[Zalastax]

Making a feature easy to explain is a high priority for me so I want us to go for split files. Users won't complain over having to split their overrides and split files is more intuitive.

[zuiderkwast]

We can allow users so specify their own prelude directories,

$ gradualizer --prelude-dir ~/my/prelude -pa xyz -- files_to_check`

Such dir can have the same structure, i.e. one file per module to override stuff for (e.g. lists.erl). Many small files? Who cares? It's a simple interface. Maybe we should consider a different suffix than .erl though, as these files won't compile as Erlang modules. How about .erlspec or similar? Or .grl...

[josefs]

Having a separate file suffix sounds like a good idea. I don't have a strong opinion on the actual name

[josefs]

Feel free to merge once you've made dialyzer happy again.

[gomoripeti]

great work @zuiderkwast

Im not sure if this is the case but if gradualizer_prelude.beam is already compiled and one changes a specs file, rebar might not recompile the module as gradualizer_prelude.erl hasn't changed and rebar does not know there is a dependency between the specs file and the source file. If this is indeed a problem, maybe better to use a compile hook escript instead of the parse trans to conditionally regenerate the gradualizer_prelude.erl file. What do you all think?

[Zalastax]

I don't trust the recompilation process anyway so I always delete the _build folder. I'm happy with the current solution.

[zuiderkwast]

@gomoripeti

Im not sure if this is the case but if gradualizer_prelude.beam is already compiled and one changes a specs file, rebar might not recompile the module as gradualizer_prelude.erl hasn't changed and rebar does not know there is a dependency between the specs file and the source file.

Right.... In a Makefile (let's say erlang.mk) it would be so easy to just add a dependency, like ebin/gradualizer_prelude.beam: priv/prelude or so. (Why did people invent rebar anyway?)

If this is indeed a problem, maybe better to use a compile hook escript instead of the parse trans to conditionally regenerate the gradualizer_prelude.erl file. What do you all think?

I guess it needs to be addressed whenever someone adds or changes a prelude spec the next time. Otherwise users won't see any effect. Personally, I don't want to have hard dependencies on rebar, so I prefer a parse transform, but as long as rebar is used to build, a hook for at least invalidating the prelude would be good. (IMO gradualizer should be compatible with rebar as a plugin, etc. but preferably it should also be possible to build and use it with e.g. Mix or GNU Make or even Erlang make:all() as an alternative. It shouldn't be that hard since we don't have any dependencies. For dev stuff like running the tests, requiring rebar is fine.)

@Zalastax

I don't trust the recompilation process anyway so I always delete the _build folder. I'm happy with the current solution.

Wow, it really is a ridiculous build system then. IMO you can't expect people to make clean at every update.

[josefs]

I'm just going to add that I've lost a lot of time because the recompilation checker in rebar doesn't do the right thing. And I still have an issue when using dialyzer which I cannot seem to fix no matter how many files I delete. At this point I'm considering setting my laptop on fire to see if that helps.
I'm very tempted to replace rebar for the day-to-day development of gradualizer because it just slows me down right now. Though as @zuiderkwast says, we should still make sure we're compatible with rebar e.g. as a plugin. But we should have another build system which doesn't act as a roadblock.