-
Notifications
You must be signed in to change notification settings - Fork 829
Parser: Capture multiple block separators #18899
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
Parser: Capture multiple block separators #18899
Conversation
❗ Release notes required
|
# Conflicts: # tests/FSharp.Compiler.Service.Tests/FSharp.Compiler.Service.SurfaceArea.netstandard20.release.bsl
This is ready :) |
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 don't think yet that this is a correct way to model the tree. We should only capture the semicolons and the commas.
type BlockSeparator = | ||
| Semicolon of range: range * position: pos option | ||
| Comma of range: range * position: pos option | ||
| Offside of range: range * position: pos option |
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.
@edgarfgp I don't understand how these types are supposed to work in practice. The separator is wrapped in an option
in other places in the tree, e.g. in NamePatPairField
. Let's assume the value is Some
, i.e. there is a separator. How could it ever be that the position is None
in such case?
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.
Yeah this is fair. I wanted to preserve the shape type BlockSeparator = range * pos option
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.
Let me create a PR to make Semicolon position no optional
// A separator consisting of a newline | ||
/// range is the range of the newline | ||
/// position is the position of the newline (if available) | ||
| Offside of range: range * position: pos option |
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.
Why do we need to capture this case? And especially the new line? Are there any cases where we rely on it? If yes, let's review them, please. It seems very suspicious. It effectively captures implementation details of how things are parsed and we should not rely on it in any way.
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.
Because we need this to model OBLOCKSEP
seps_block:
| OBLOCKSEP
{ (rhs parseState 1), None }
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.
Why would we ever want to model it? If that's an implicit separator created by the unrelated subsequent tokens, we should not be relying on it in any way.
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.
And if OBLOCKSEP
can be a real token, this info should be carried in it from the LexFilter
to the parser, so we could distinguish these cases.
We should not model the tree for non-existent tokens that are generated by subsequent unrelated ones.
The more I think about this change the more I think it should've not been merged as is. Let's consider using the |
This reverts commit a276808.
This reverts commit a276808. # Conflicts: # docs/release-notes/.FSharp.Compiler.Service/10.0.100.md
Description
Capture multiple block separators: semicolon, comma, offside
This can be used in multiple places line Record, DU etc. Will also help to have better recovery and error messages.
Split #18881
Checklist