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

Allow giving include path to erl files imported into db #344

Merged

Conversation

erszcz
Copy link
Collaborator

@erszcz erszcz commented Sep 25, 2021

Copy link
Collaborator

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the use case?

I considered gradualizer_db an internal module. If it's useful to call it from other apps, maybe we should add some wrapper funtions in the gradualizer module or document that db is not internal. WDYT?

@erszcz
Copy link
Collaborator Author

erszcz commented Sep 27, 2021

To properly import source files we have to resolve macros. Otherwise, we're not going to get the full information. Without correct include paths we might not be able to find all header files included by a given source file. Records are traditionally defined in headers when intended for sharing between modules or apps, so this is important for real-world usage. Other macros might be needed as well, like constants, "custom guards", etc, but records are the big deal.

In more detail, it's about integration with Erlang Language Server. The server supports configuring project-wide settings like include and ebin paths, etc, and can pass these down to, so called, diagnostics. These are plugins implementing various kinds of checks - on an internal hackathon early this year we (@garazdawi, me and another colleague) created a diagnostic bridging Gradualizer and ErlangLS. The whole setup in a nutshell is:

I'm going to describe it in more detail sooner or later.

Here are some screenshots of what that looks like in practice:

Exhaustiveness checking on reasonably complex types

One more example of exhaustiveness checking

self-gradualize

@zuiderkwast
Copy link
Collaborator

That's really cool!

No more question about how useful erlang_ls is. :-) I've heard about it before and it's really impressive.

I suppose you can't use the same include paths as supplied the CLI using -I and to gradualizer using {i, Dir}?

Perhaps we can add a separate option such as {import_include, Dir} to make it's accessable from the CLI and gradualizer too? That only works if you want to use the same include path for all erl files, which I assume you don't want.

Inferring the include paths from a complex nested directory structure full of Makefiles and makefile includes (such as OTP itself) is pretty hard, so we just used beam the files for imports.

How about adding a gradualizer:import/2 wrapper for gradualizer_db? Or do you prefer using gradualizer_db directly?

@erszcz
Copy link
Collaborator Author

erszcz commented Sep 27, 2021

That's really cool!

It is! The Erlang LS team has done a great job and I'm happy to add a bit from myself.

I suppose you can't use the same include paths as supplied the CLI using -I and to gradualizer using {i, Dir}?

The include paths passed on the command line are not used by gradualizer_db:import_erl_files/1 at all - only the paths guessed by guess_include_dirs are, and now the paths passed in explicitly to gradualizer_db:import_erl_files/2. They could be, but they weren't and we went for, admittedly, the easiest fix.

Perhaps we can add a separate option such as {import_include, Dir} to make it's accessable from the CLI and gradualizer too? That only works if you want to use the same include path for all erl files, which I assume you don't want.

I'm not sure what the best approach is. The proposed approach allows to control the set of include files per each set of .erl files we're including. I'm not sure if this level of control is necessary, but so far it didn't hurt.

Inferring the include paths from a complex nested directory structure full of Makefiles and makefile includes (such as OTP itself) is pretty hard,

Indeed and I think no inference method can give 100% correct results for all possible project layouts. That's why it's really handy to use the settings provided by Erlang LS from its config - they only have to be set there once per project. Their team explores this topic further and looks into https://build-server-protocol.github.io/ - this could probably allow for sharing settings between Erlang LS, Rebar3, and other such tools, but I think it's still a thing of the future.

[...] so we just used beam the files for imports.

That's indeed a good alternative, but would require a recompile on each change, which is slower if we're considering the in-editor experience.

How about adding a gradualizer:import/2 wrapper for gradualizer_db? Or do you prefer using gradualizer_db directly?

Given Gradualizer is still not completely stable, I imagine there might be changes on this front. The API proposed here is fine for Erlang LS integration purpose.

Copy link
Collaborator

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I can merge this then, if you're happy with it. Keep in mind that you're the most active contributor right now, so you may want to take responsibility for how it evolves. :-)

src/gradualizer_db.erl Outdated Show resolved Hide resolved
src/gradualizer_db.erl Outdated Show resolved Hide resolved
src/gradualizer_db.erl Outdated Show resolved Hide resolved
@erszcz
Copy link
Collaborator Author

erszcz commented Sep 27, 2021

Closing and reopening to update CI status.

@erszcz erszcz closed this Sep 27, 2021
@erszcz erszcz reopened this Sep 27, 2021
src/gradualizer_db.erl Outdated Show resolved Hide resolved
Co-authored-by: Viktor Söderqvist <viktor@zuiderkwast.se>
@erszcz
Copy link
Collaborator Author

erszcz commented Sep 30, 2021

I think this one is ready to merge with your suggestions @zuiderkwast included 👍

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

Successfully merging this pull request may close these issues.

3 participants