x/tools/gopls: investigate if 'fixed' syntax needs more accurate positions #64335
Comments
findleyr
added
gopls
|
Change https://go.dev/cl/546655 mentions this issue: |
In golang/go#64488, we observe how a seemingly innocuous change to postfix completion tests led to significant flakiness in our integration tests, which started to encounter a new bug.Report for out-of-bounds positions on type checker errors. The root cause is that the new syntax of the test data triggered AST fixes (i.e. parsego.fixAST) that overflowed the original file. The failure was not deterministic because the postfix snippet tests do not wait for diagnostics: adding an env.AfterChange() to the end of the test body made the failure deterministic. Additionally, while investigating I encountered and fixed a clear bug that fixes (and therefore parsego.File.Fixed()) were not correctly counted. This was incidental, and did not contribute to the test failures. We don't actually use Fixed() very much, though we probably should consider it more. To fix the underlying bug, clamp positions to the token.File. This is definitely unsatisfactory, but after an hour of tracing through the fix logic, I am hesitant to commit to a more principled yet risky change. This logic is rather tricky and clearly contains a lot of embedded knowledge. It's probably best to leave it alone and redirect efforts to improved parser recovery. This is strong evidence that at some point we do need to carefully scrutinize the AST fixes (see also golang/go#64335). Updates golang/go#64335 Fixes golang/go#64488 Change-Id: I70a33c0c9aae66baae78e6474ee56cdaa25e45f4 Reviewed-on: https://go-review.googlesource.com/c/tools/+/546655 Auto-Submit: Robert Findley <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]> Reviewed-by: Alan Donovan <[email protected]>
|
Hi @findleyr , could you elaberate more about the test cases for fixed syntax in the todo comment "we should have many more tests for fixed syntax.". I can help to add some test cases to track it. |
|
Change https://go.dev/cl/664095 mentions this issue: |
|
Thanks, @xieyuschen. I think the test you added in that CL is good. There are very few documented invariants that we can check (currently), but enforicing that Pos() is in the source makes sense to me. |
This CL adds a new test case to verify the fixed syntax in parsego.Parse, which helps us better understand its behavior. Updates: golang/go#64335 Change-Id: I8bf93e43a1bb67853ca02e32232aef48159295d7 Reviewed-on: https://go-review.googlesource.com/c/tools/+/664095 LUCI-TryBot-Result: Go LUCI <[email protected]> Reviewed-by: Alan Donovan <[email protected]> Reviewed-by: Robert Findley <[email protected]> Auto-Submit: Alan Donovan <[email protected]>
|
Change https://go.dev/cl/664435 mentions this issue: |
|
Change https://go.dev/cl/664876 mentions this issue: |
The implementation of file-based versions (https://go.dev/issue/62605) uses positions to associate syntax with a per-file go version.
gopls has logic to fix syntax before type checking which likely invalidates some positions. For example, I think this manifests as #64320 (comment), where we can't position a type checker error.
This is a reminder issue that this logic may need to be more careful about preserving valid positions, such as by setting the position of synthetic syntax to the file base. Otherwise, we may produce inaccurate type checking results in the future.
Not super urgent, because at the moment there is no file version that can affect type checker rules. You can't downgrade a file to before 1.21, when the semantics of file versions changed.
Putting this in [email protected], though if this slips to v0.17 that's OK.
CC @adonovan @griesemer for awareness of this type of subtlety.
The text was updated successfully, but these errors were encountered: