Fix LSP regression #1583
Merged
Fix LSP regression #1583
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
After the bidirectional refactoring (better separating the rules between `infer` and `check`), the LSP was experiencing regressions, not showing hover information in some cases, in particular within statically typed blocks. It turns out the separation between `infer` and `check` makes less clear which function is responsible for adding the term to the linearizer: sometimes it's `check`, sometimes it's `infer`, and it really depends on the call graph and where each function is called from. It turns out the previous implementation was adding many terms in double, and this caused the initial regression observed. Morever, fixing is made hard by the fact that `infer` now doesn't have a type to provide right now for the linearizer: it first need to, well, infer it. But the linearizer currently relies on AST nodes being visited in order, so we can't either call to it whenever we want. The situation seemed intricate enough that it was easier to separate "visiting the term" from "amending the type of the term". This commit thus adds a `retype` method to the `Linearizer` trait, plus some id-related signature changes. Now, the typechecking code can add a term to the linearizer as soon as it visits it, and set the type later. More discipline has been introduced so that `check` and `infer` don't repeat the same term visits anymore. Doing so, this commit also cleans a bit the LSP implementation which was very hard to change getting it right: `IdGen` in particular is gone (which was maintaining a counter by hand in parallel to the linearization's size, which was very error-prone, and if any mismatch occurred, anything could happen). The truth is, the refactor should be much more involved to make the code maintainable, but this commit is just a fix for the 1.2.0 release to get out, and the linearizer is a legacy component that will be scraped soon when the AST-based LSP will reach feature parity with the linearizer.
yannham
force-pushed
the
refactor/linearizer
branch
from
fbce1dc
to
4aa12ff
Compare
yannham
force-pushed
the
refactor/linearizer
branch
from
4aa12ff
to
f888e9a
Compare
yannham
commented
Sep 8, 2023
vkleen
approved these changes
Sep 8, 2023
yannham
commented
Sep 8, 2023
Co-authored-by: Viktor Kleen <[email protected]>
jneem
approved these changes
Sep 8, 2023
|
|
||
| /// Variant of [infer] taking an additional `item_id` to indicate that inference is done on a term | ||
| /// that has already been visited by the linearizer, for example when coming directly from [check]. | ||
| /// When `item_id` is defined, the linearizer isn't called. [infer] is just `infer_visited` called |
There was a problem hiding this comment.
Suggested change
| /// When `item_id` is defined, the linearizer isn't called. [infer] is just `infer_visited` called | |
| /// When `item_id` is defined, the term `rt` will not be added to the linearizer. [infer] is just `infer_visited` called |
suimong
pushed a commit
to suimong/nickel
that referenced
this pull request
Sep 17, 2023
* Fix LSP not showing hover information After the bidirectional refactoring (better separating the rules between `infer` and `check`), the LSP was experiencing regressions, not showing hover information in some cases, in particular within statically typed blocks. It turns out the separation between `infer` and `check` makes less clear which function is responsible for adding the term to the linearizer: sometimes it's `check`, sometimes it's `infer`, and it really depends on the call graph and where each function is called from. It turns out the previous implementation was adding many terms in double, and this caused the initial regression observed. Morever, fixing is made hard by the fact that `infer` now doesn't have a type to provide right now for the linearizer: it first need to, well, infer it. But the linearizer currently relies on AST nodes being visited in order, so we can't either call to it whenever we want. The situation seemed intricate enough that it was easier to separate "visiting the term" from "amending the type of the term". This commit thus adds a `retype` method to the `Linearizer` trait, plus some id-related signature changes. Now, the typechecking code can add a term to the linearizer as soon as it visits it, and set the type later. More discipline has been introduced so that `check` and `infer` don't repeat the same term visits anymore. Doing so, this commit also cleans a bit the LSP implementation which was very hard to change getting it right: `IdGen` in particular is gone (which was maintaining a counter by hand in parallel to the linearization's size, which was very error-prone, and if any mismatch occurred, anything could happen). The truth is, the refactor should be much more involved to make the code maintainable, but this commit is just a fix for the 1.2.0 release to get out, and the linearizer is a legacy component that will be scraped soon when the AST-based LSP will reach feature parity with the linearizer. * Add Hover capability to tests * Add regression test for hover after bidir pull request * Formatting and code clean-up * Apply suggestions from code review Co-authored-by: Viktor Kleen <[email protected]> * Make retype calls less verbose * Fix documentation dangling link --------- Co-authored-by: Viktor Kleen <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #1574.
Cause
After the bidirectional refactoring (better separating the rules between
inferandcheck), the LSP was experiencing regressions, not showing hover information in some cases, in particular within statically typed blocks.The separation between
inferandcheckmakes less clear which function is responsible for adding the term to the linearizer: sometimes it'scheck, sometimes it'sinfer, and it really depends on the call graph and where each function is called from.It turns out the implementation was adding many terms in double, and this caused the initial regression observed.
Content
Fixing the double visit is made harder by the fact that
inferdoesn't have a type to provide right now to the linearizer: it first need to, well, infer it. But the linearizer currently relies on AST nodes being visited in order, so we can't either call to it whenever we want.The situation seemed intricate enough that it was easier to split "visiting the term" and "amending the type of the term" in two operations. This PR thus adds a
retypemethod to theLinearizertrait, plus some id-related signature changes. Now, the typechecking code can add a term to the linearizer as soon as it visits it, and update the type later. More discipline has been introduced so thatcheckandinferdon't repeat the same term visits anymore, fixing the original issue.Doing so, this PR also cleans a bit the LSP implementation, as changing it was very hard to get right.
IdGenin particular is gone, as it amounted to maintain a parallel insertion index by hand instead of just relying on the current linearization's size, which was very error-prone and caused strange errors if any off-by-one error happened.The truth is, a refactor to make the code actually maintainable should be much more involved, but this commit is just a fix for the 1.2.0 release to get out. I did the minimum changes to be able to make the patch without losing mental sanity. The linearizer is a legacy component that will be scraped as soon as the AST-based LSP reaches feature parity with the linearizer, hence it's probably not worth a heavy rework.