-
Notifications
You must be signed in to change notification settings - Fork 833
Add SynType.Paren #9241
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
Add SynType.Paren #9241
Conversation
TIHan
left a comment
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.
This seems fair, but Paren really isn't a "Type" and has no significance in the TypeChecker, which is why you simply skip over it. Should we add another union case in the syntax that is strictly for information purposes, namely for tooling?
I know we do not keep token information around, which is the problem. We don't have a mechanism to extract that information. If we were able to build something that would allow someone to query for tokens based on a range, would that be useful and potentially help solve your issue?
Yes, we should do this - the SyntaxTree type is slowly evolving to include all necessary syntax trivia (it should have always been like this, my fault way back at the start ) |
I guess nor
I think it's fine, since
We're mostly interested in the correct file structure w.r.t. ranges here, not the particular tokens info, so it'd probably not make it easier in our use case. |
Note I believe Fantomas has a phase that folds the token stream back into an extra-augmented version of the SyntaxTree, prior to source code formatting. However if the tooling built in this repo needs extra syntax information, we should just add it. |
There is no fault, it just is what it is. In a perfect world, I'm wondering how we would model the trivia; because I don't know.
SynExpr.Paren does seem to have a bit more significance for the TypeChecker. But, SynPat.Paren does not.
This makes sense; we already seem to do this in other places.
Makes sense as well. Sorry for being nit-picky. Whenever I see new cases added in the syntax tree, I always want to discuss it and make sure. I feel much better about SynType.Paren now :). Thank you all for explaining. |
TIHan
left a comment
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.
Marking approved, pending tests and minor nit.
|
there is a test failure |
|
@auduchinok , I think your code is correct, and the test baseline needs updating, I'm taking care of it. |
|
@KevinRansom Thanks, but please don't merge it yet, I'll need to add a test case before it goes in. |
|
K. I'll WIP it. |
6975ba7 to
04a51fb
Compare
|
It's ready. @KevinRansom Thank you for the baselines updates! |
* Add SynType.Paren * Review fixes: Skip -> Strip, remove `rec` * Add test for paren types ranges * Cleanup tests * Update neg04 test baseline * Cleanup Co-authored-by: Kevin Ransom <[email protected]>
Keeps parser knowledge about (possibly nested) parens found when parsing syntax types. It's needed when trying to get tree structures in cases like the following:
I'll add tests when existing ones are green.