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

Reload project-specific files on every Graudalizer diagnostic run #1240

Merged

Conversation

erszcz
Copy link
Contributor

@erszcz erszcz commented Mar 8, 2022

The check to determine if project files should be imported into Gradualizer comes from the time when the integration relied on Gradualizer being available through ERL_LIBS and would only be started by the diagnostic being called.

Now that Gradualizer is a dependency of ErlangLS, it's running on each startup since ErlangLS itself. This means the check never succeeds, therefore the project specific files necessary for properly setting up the typechecker are never loaded. This leads to false positives being reported such as missing remote types or record definitions.

Reimporting the project files on each run might be a bit redundant, but it seems to work reasonably well in practice. Dogfooding this solution shows it doesn't lead to visible unnecessary load on the system.

The check to determine if project files should be imported into Gradualizer
comes from the time when the integration relied on Gradualizer being available
through ERL_LIBS and would only be started by the diagnostic being called.

Now that Gradualizer is a dependency of ErlangLS,
it's running on each startup since ErlangLS itself.
This means the check never succeeds, therefore the project specific files
necessary for properly setting up the typechecker are never loaded.
This leads to false positives being reported such as missing remote types or
record definitions.

Reimporting the project files on each run might be a bit redundant,
but it seems to work reasonably well in practice.
Dogfooding this solutions shows it doesn't lead to visible unnecessary
load on the system.
@robertoaloi
Copy link
Member

Hi @erszcz and thanks for this. I am a bit concerned about the performance implications of running the import at every save. While this could be doable for small repository, it can easily create problems at scale.
In #1148 there's an attempt at introducing an init callback for diagnostics. Would it make sense to use it here, so that the import is only executed once?

Copy link
Member

@robertoaloi robertoaloi left a comment

Choose a reason for hiding this comment

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

It would be great if we could add the init callback for diagnostics and only import files once.

@erszcz
Copy link
Contributor Author

erszcz commented Mar 10, 2022

Thanks for taking a look, @robertoaloi!

In #1148 there's an attempt at introducing an init callback for diagnostics. Would it make sense to use it here, so that the import is only executed once?

I saw the init concept mentioned in some other issue and considered using it, but it would introduce another problem. If we only read the files on init, then types added or specs changed as we edit wouldn't be taken into consideration on subsequent runs of the diagnostic.

There's definitely some work on ErlangLS and Gradualizer interop, but just using init for the logic which used to be in start_and_load doesn't solve the problem - the original idea of start_and_load was simply wrong. Reloading the files on each run carries a performance penalty, but a more sophisticated mechanism would probably require hashing files on each diagnostic run to check for changes and reload as necessary.

If it's not a problem, I'd opt for merging this PR as it solves a nasty bug. I think the performance issue is secondary to that, though it certainly has to be revisited at some point.

@robertoaloi
Copy link
Member

robertoaloi commented Mar 10, 2022

@erszcz Considering that the Gradualizer integration is disabled by default, I don't have a problem with that.

Long term, Erlang LS should have support for DidChangeWatchedFiles events. Once that is in place, we could revisit the diagnostic API to allow backends to register interest in changes.

Again, thanks for the contribution!

@robertoaloi robertoaloi merged commit 0f95fec into erlang-ls:main Mar 10, 2022
@erszcz erszcz deleted the gradualizer-reimport-context-on-every-run branch March 11, 2022 08:26
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.

2 participants