Initial draft of SynMemberDefn.ReadWriteMember. #13292
Conversation
src/Compiler/pars.fsy
Outdated
| let attrs = $2 |> List.map (fun attrList -> { attrList with Attributes = attrList.Attributes |> List.map (fun a -> { a with AppliesToGetterAndSetter = true } ) }) | ||
| let mSet = rhs parseState 3 | ||
| let mEquals = rhs parseState 6 | ||
| let m = rhs2 parseState 1 7 |
There was a problem hiding this comment.
This range may include subsequent nodes when error happens inside typedSequentialExprBlock. A safer approach would be to get the range of the parsed expression and to join it with the start range. Here's an example of a similar range issue related to error recovery: #12224
|
|
||
| | Member of memberDefn: SynBinding * range: range | ||
|
|
||
| | ReadWriteMember of |
| xmlDoc: PreXmlDoc * | ||
| identifier: SynPat * | ||
| returnTypeOpt: SynReturnInfo option * | ||
| read: SynMemberDefnPropertyInfo * |
| IsInline: bool | ||
| Attributes: SynAttributes | ||
| /// 'get' or 'set | ||
| IsWrite: bool |
|
|
||
| /* The "with get, set" part of a member definition */ | ||
| classDefnMemberGetSet: | ||
| | WITH classDefnMemberGetSetElements |
There was a problem hiding this comment.
I think it would be best to avoid making any grammar changes as part of this. That should be possible?
There was a problem hiding this comment.
Perhaps, but I feel like this is a bit of a missed opportunity to separate some logic based on what the parser actually detects.
Having the rule:
/* The "get, set" part of a member definition */
classDefnMemberGetSetElements:
| classDefnMemberGetSetElement
{ [$1], None }
| classDefnMemberGetSetElement AND classDefnMemberGetSetElement
{ let mAnd = rhs parseState 2
[$1;$3], Some mAnd }
forces the consumer to deal with this in code. Both get and set or single get or single set need to be processed correctly where classDefnMemberGetSetElements is used.
| /* Properties with explicit get/set, also indexer properties */ | ||
| /* Properties with explicit get and set, also indexer properties */ | ||
| | opt_inline bindingPattern opt_topReturnTypeWithTypeConstraints classDefnMemberGetSet | ||
| { let mWith, (classDefnMemberGetSetElements, mAnd) = $4 |
There was a problem hiding this comment.
I'd recommend moving this entire helper to ParseHelpers.fs or SyntaxTreeOps.fs
There was a problem hiding this comment.
You could do this as pre-PR cleanup perhaps
We should move all such large sections of code out of pars.fsy
|
I'm in two minds about how to go about this. The current approach just doesn't feel right It's just odd that the GetSet case is captured so differently to the Get case and Set case. That said I'm sure you've thought of this too and I personally can't think of a much better way yet. |
|
Note you'll need to adjust all code matching on SynBinding, especially all tree-walking code. |
To some degree, I think that
To me, this feels like a construct that is being re-used for historical reasons but is different enough at the syntax level for us to start noticing it. The perhaps safer alternative was to just introduce a node that stores both SynBindings. |
|
Closing in favour of #13420 |
3 participants
This is a first attempt to address #13273.
So far I've only touched the untyped tree. However, these changes are already quite large.
I'd like to get some feedback from @dsyme, @auduchinok and others before trying to finish this.
Where the member has both
getandset, I've introducedSynMemberDefn.ReadWriteMember.If there is only
getorset, I'm constructing the same AST as before (using a helper function in SynTreeOps).Both grammar rules still return a list, I'd like to get rid of that as well.
Happy to hear your thoughts on this.