-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Ref assignment switch expressions #7352
Open
Rekkonnect
wants to merge
9
commits into
dotnet:main
Choose a base branch
from
Rekkonnect:ref-assignment-switch-expressions
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
7d2dc7d
Create spec for ref assignment on switch expressions
Rekkonnect a0eeaa1
Do include a ref in front of the expression
Rekkonnect 011f974
Include more details in the spec
Rekkonnect 827884d
Update rules about `ref readonly` + examples
Rekkonnect 1d66a5c
Update grammar adjustment information + fix words
Rekkonnect 24caf81
Promote ref discard to error
Rekkonnect 0783829
More rules
Rekkonnect f2bfc62
Clarify default switch arm
Rekkonnect ef422c4
Improve ref safety description
Rekkonnect File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,69 @@ | ||
# `ref` assignment on `switch` expressions | ||
|
||
* [x] Proposed | ||
* [ ] Prototype: None | ||
* [ ] Implementation: None | ||
* [ ] Specification: Started, below | ||
|
||
## Summary | ||
[summary]: #summary | ||
|
||
Provide the ability to return `ref` values in `switch` expressions. | ||
|
||
## Motivation | ||
[motivation]: #motivation | ||
|
||
The `switch` expression is meant to provide a quicker way to return values based on a given expression, but does not support returning `ref` expressions. This limitation doesn't prevent any harm, and can be lifted. | ||
|
||
## Detailed design | ||
[design]: #detailed-design | ||
|
||
A `switch` expression may return a `ref` value. The returning expression, if not a `throw` expression, must be a `ref` expression. | ||
|
||
A `switch` expression that returns a `ref` value needs an extra `ref` in front of it when assigned/returned to a `ref` local. The examples below cover this case too. This design aligns with the current design in the ternary operator. | ||
|
||
`throw` expressions can still be used normally. | ||
|
||
## Examples | ||
[examples]: #examples | ||
|
||
```csharp | ||
private ref int GetDirectionField(Direction direction) => ref direction switch | ||
{ | ||
Direction.North => ref NorthField, | ||
Direction.South => ref SouthField, | ||
Direction.East => ref EastField, | ||
Direction.West => ref WestField, | ||
_ => throw new NotImplementedException(), | ||
}; | ||
|
||
private void RefSwitchAssignLocal() | ||
{ | ||
ref int dimension = ref axis switch | ||
{ | ||
Axis.X => ref X, | ||
Axis.Y => ref Y, | ||
}; | ||
} | ||
Rekkonnect marked this conversation as resolved.
Show resolved
Hide resolved
|
||
``` | ||
|
||
## Drawbacks | ||
[drawbacks]: #drawbacks | ||
|
||
None. | ||
|
||
## Alternatives | ||
[alternatives]: #alternatives | ||
|
||
Currently, this may only be achieved with a `switch` statement, or a sequence of `if`-`else` statements. | ||
|
||
## Unresolved questions | ||
[unresolved]: #unresolved-questions | ||
|
||
- [ ] Requires LDM review | ||
- [ ] Should there be a quicker way to denote returning a `ref` to the provided expression by not requiring copying it over for each case? | ||
|
||
## Design meetings | ||
[meetings]: #design-meetings | ||
|
||
None. |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
One regret I have with conditional
ref
expressions is that the following compiles without a diagnostic:Clearly the user meant to take a
ref
here yet they forgot theref
beforecond
hence ended up with a value. Guessing many devs have spent time wondering what was going wrong here.Would not like to repeat that mistake here. Suggest diagnostics for the following scenarios:
switch
haveref
but the overall expression does not.ref
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.
You could(if it doesn't already exist) add an Analyzer for the
ref
expressions as well "while you're at it"(not that that's a good argument for a language decision)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.
Clearly Jared has a point here, 99% of the time you want the ref of the returned expression, so a warning is very reasonable.
Adding that warning for ref ternaries should probably be done in the future, but it's off the scope of this proposal. Probably a warning wave too, to avoid breaking existing code.
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 would simply make it a requirement for switches. WE can retcon ternaries independent of this.
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.
Added a simple advisory for causing a warning instead of an error that requires that the returned ref is directly used. If we settle on the error, I will update the spec accordingly.
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.
Update, after some thought and discussion I noticed that:
c
yields error CS8171 - Cannot initialize a by-value variable with a referenced
does not yield any errors and is completely legal, as if the value is immediately dereferencedEdit: the above examples are not very linked with each other. The initialization of
c
is an invalid intention, whereas the initialization ofd
simply discards a reference that is returned from the expression's result. It would be equally as legal to initialized
toa
orb
without a reference. The only dissonance is the discarding of the reference from the ternary.We should definitely strive for consistency with
ref
variables, and designingref
switch expressions should return an error if the value is immediately dereferenced.As for the ref ternary expressions, it could be a breaking change, via either a warning or an error that immediately applies to existing code, hinting that it was definitely a mistake in the first place. Though discussing this one goes out of scope of this PR.
I'll prepare spec changes for this new approach on the error, would love some more input on this decision to make sure we cover this case elegantly.