Skip to content

fix: don't track struct members with error for LSP #10709

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.

Sign up for GitHub

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

Open
asterite wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: don't track struct members with error for LSP #10709

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions compiler/noirc_frontend/src/elaborator/expressions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1072,6 +1072,7 @@ impl Elaborator<'_> {
field: field_name.clone(),
struct_definition: struct_type.borrow().name.clone(),
});
continue;
}

if let Some((index, visibility)) = expected_index_and_visibility {
Expand Down
1 change: 1 addition & 0 deletions compiler/noirc_frontend/src/elaborator/patterns.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -395,6 +395,7 @@ impl Elaborator<'_> {
field: field.clone(),
struct_definition: struct_type.borrow().name.clone(),
});
continue;
Copy link
Contributor

@vezenovm vezenovm Dec 1, 2025

Choose a reason for hiding this comment

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

It feels a little weird to only have continue here. This means that the specified field will no longer be part of the fields returned from this method (we will skip ret.push((field, resolved)) below). We are erroring so that seems ok, but when we error due to seen_fields.contains(&field) we still have the bad field as one of the constructor pattern fields. Does the LSP crash when we have duplicate fields too? Should we continue in the other error case as well? What if we just called add_struct_member_reference in the valid case above and did not continue at all? We would then have the accurate fields for the constructor pattern (even if they trigger errors).

Copy link
Collaborator Author

@asterite asterite Dec 1, 2025

Choose a reason for hiding this comment

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

The LSP crashes because later on add_struct_member_reference with an index that won't correspond to an actual field, so when we later call field_at it will panic. So LSP only crashes in the NoSuchField case because we still record those bad fields as some struct members at some index... which don't exist, really.

I initially fixed it in the inlay hint code, but then noticed it was also broken in hover, so then I decided to fix the root cause.

I also originally put continue in all error cases, but then I thought that if you hovered over the duplicate field you wouldn't get any hover, as we skipped that.

I'm not sure what's the best fix here.

}

ret.push((field, resolved));
Expand Down
Loading