-
Notifications
You must be signed in to change notification settings - Fork 359
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.
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'ACVM Benchmarks'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.
| Benchmark suite | Current: aea9f7f | Previous: c3261da | Ratio |
|---|---|---|---|
perfectly_parallel_batch_inversion_opcodes |
2793635 ns/iter (± 11247) |
2253375 ns/iter (± 8611) |
1.24 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Test Suite Duration'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.
| Benchmark suite | Current: aea9f7f | Previous: c3261da | Ratio |
|---|---|---|---|
test_report_zkpassport_noir-ecdsa_ |
3 s |
1 s |
3 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Execution Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.
| Benchmark suite | Current: aea9f7f | Previous: c3261da | Ratio |
|---|---|---|---|
rollup-block-root-single-tx |
0.003 s |
0.002 s |
1.50 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
| field: field.clone(), | ||
| struct_definition: struct_type.borrow().name.clone(), | ||
| }); | ||
| continue; |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
Description
Problem
No issue, just trying to prevent LSP from ever crashing.
Summary
This code would crash LSP:
for two reasons:
xin the struct pattern would crashxwould crashThe reason is that the struct field was registered for LSP when it didn't exist. Now we don't do that anymore.
Additional Context
User Documentation
Check one:
PR Checklist
cargo fmton default settings.