-
Notifications
You must be signed in to change notification settings - Fork 782
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
Fix Internal error when analysing incomplete inherit member #17905
base: main
Are you sure you want to change the base?
Conversation
❗ Release notes required
|
@@ -37,7 +37,7 @@ type SynLongIdent = | |||
|
|||
member this.Range = | |||
match this with | |||
| SynLongIdent([], _, _) -> failwith "rangeOfLidwd" | |||
| SynLongIdent([], _, _) -> Range.Zero |
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.
I think this may be a not good fix here, because it affects to many different places where this type is used. The exception immediately shows that something is really wrong, and a wrong range may go unnoticed for a long time.
It's probably much better to fix it more systematically (e.g. use SynLongIdent option
instead of SynLongIdent
in places of known recovery) and/or check it in the outer code.
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.
Range.Zero
is commonly used in similar scenarios in the SyntaxTree
file. But I agree we could use option
here
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.
Range.Zero is commonly used in similar scenarios in the SyntaxTree file.
Well, that's wrong because it means some node with a range like (10,20)-(10,30)
can now contain a node with range (0, 0)-(0, 0)
which is outside of its parent node range. Anyway, we need to fix this more systematically in future.
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.
Style nit: I generally think keeping pattern matching flat where possible (instead of adding nested match
expressions) makes the top-level match easier to understand, but feel free to ignore if you disagree.
Co-authored-by: Brian Rourke Boll <[email protected]>
Co-authored-by: Brian Rourke Boll <[email protected]>
Co-authored-by: Brian Rourke Boll <[email protected]>
Co-authored-by: Brian Rourke Boll <[email protected]>
878597e
to
362bdfc
Compare
@psfinaki Sorry to bother you again. Could you help me update the last salsa test https://dev.azure.com/dnceng-public/public/_build/results?buildId=850191&view=ms.vss-test-web.build-test-results-tab&runId=22075436&resultId=108303&paneView=debug. Trying to do it using the CI is really painful :( |
Description
Fixes #17902
Checklist