-
Notifications
You must be signed in to change notification settings - Fork 832
Make typechecking indexed setters with tuples on the right more consistent #17017
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
Conversation
* In the scenarios where assigning an unparenthesized tuple using an
indexed setter already works, `MakeDelayedSet` is actually being
called twice — once before `TcIndexingThen` is called, and once inside
of it. For `SynExpr.NamedIndexedPropertySet`, `SynExpr.DotIndexedSet`,
and, `SynExpr.DotNamedIndexedPropertySet`, we don't actually want to
delay it; it just needs an extra set of parens (whether it should
really need double parens is a different question). In this commit,
I am simply adding the extra parens inline. I could alternatively
have updated `MakeDelayedSet` itself to always wrap in double
parens, although that would mean that in the codepaths where that
function is itself already being called twice, we'd be adding extra
parens parens unnecessarily. While extra parens do not affect
typechecking (unlike insufficient parens), it seems undesirable to
allocate more nodes than we really need, only to strip them later
anyway.
❗ Release notes required
|
…ll/fsharp into indexed-set-tuples
|
Gathering mental energy for this, hope to review this tomorrow ;) |
psfinaki
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.
Okay this wasn't that hard to understand in the end 🤦
Thanks for the fix and for the thorough testing, consistency in one of the focus areas for F# these days.
Head branch was pushed to by a user without write access
Description
Fixes #16987.
Wrap the value expression in indexed setters in extra parentheses in 3 more cases.
In the scenarios where assigning an unparenthesized tuple using an indexed setter already works,
MakeDelayedSetis actually being called twice — once beforeTcIndexingThenis called, and once inside of it. ForSynExpr.NamedIndexedPropertySet,SynExpr.DotIndexedSet, and,SynExpr.DotNamedIndexedPropertySet, it was not being called twice, because we don't actually want to delay it; the expression does, however, need an extra set of parens in order to receive the same treatment later on as the value expression fromSynExpr.Set. Whether double parens should really be needed there is a different question (that I did not investigate).Extra parens were already added around the value expr for
SynExpr.Setfsharp/src/Compiler/Checking/CheckExpressions.fs
Lines 5822 to 5825 in 6b6cca9
Extra parens were not added for other indexed setters
fsharp/src/Compiler/Checking/CheckExpressions.fs
Lines 5836 to 5839 in 6b6cca9
Extra parens are (also) always added here, which is reached from both of the above paths
fsharp/src/Compiler/Checking/CheckExpressions.fs
Lines 6601 to 6606 in 6b6cca9
In this PR, I am simply adding the extra parens inline when
SynExpr.NamedIndexedPropertySet,SynExpr.DotIndexedSet, and,SynExpr.DotNamedIndexedPropertySetare encountered during typechecking. I could alternatively have updatedMakeDelayedSetitself to always wrap in double parens, although that would mean that, in the codepaths where that function is already being called twice, we'd be adding extra parens unnecessarily. While excess parens do not affect typechecking in this case (unlike insufficient parens), it seems undesirable to allocate more nodes than we really need, only to strip them later anyway.Checklist