-
Notifications
You must be signed in to change notification settings - Fork 830
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
Changes from all commits
1799aaf
55507e9
1738018
65f5bb6
0c97b9d
6f2b706
e8f1bb0
75d8f5e
4f2e97e
63be5d5
4248f2a
cc96217
e270b88
e0cc65a
1e29d58
1470bf9
8988215
74712e8
967c4a9
a30cef4
5fa0480
15e3d34
b7ffcf8
5bde641
0f7c23c
6a6843c
d11dd4a
30fc9a2
815c51b
7536249
5b1a5aa
0cac8f6
9bd11e8
9018065
f23dc6d
908b292
2b5685c
4324335
4d19e95
73d6e60
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -354,9 +354,26 @@ type SeqExprOnly = | |
/// Indicates if a for loop is 'for x in e1 -> e2', only valid in sequence expressions | ||
| SeqExprOnly of bool | ||
|
||
/// Represents the location of the separator block + optional position | ||
/// of the semicolon (used for tooling support) | ||
type BlockSeparator = range * pos option | ||
/// Represents the location of the separator block and optional position of the semicolon (used for tooling support) | ||
[<NoEquality; NoComparison; RequireQualifiedAccess>] | ||
type BlockSeparator = | ||
/// A separator consisting of a semicolon ';' | ||
/// range is the range of the semicolon | ||
/// position is the position of the semicolon (if available) | ||
| Semicolon of range: range * position: pos option | ||
/// A separator consisting of a comma ',' | ||
/// range is the range of the comma | ||
/// position is the position of the comma (if available) | ||
| Comma of range: range * position: pos option | ||
|
||
// 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 | ||
Comment on lines
+369
to
+372
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Because we need this to model
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. And if We should not model the tree for non-existent tokens that are generated by subsequent unrelated ones. |
||
|
||
member Range: range | ||
|
||
member Position: pos option | ||
|
||
/// Represents a record field name plus a flag indicating if given record field name is syntactically | ||
/// correct and can be used in name resolution. | ||
|
Uh oh!
There was an error while loading. Please reload this page.
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. inNamePatPairField
. Let's assume the value isSome
, i.e. there is a separator. How could it ever be that the position isNone
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