Skip to content
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
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

Rekkonnect
Copy link
Contributor

@Rekkonnect Rekkonnect commented Jul 18, 2023

Contributes to #3326

This is a somewhat old spec that was missed from being opened as a PR. Might need to take another iteration on it after two years.

@Rekkonnect Rekkonnect requested a review from a team as a code owner July 18, 2023 18:15
@Rekkonnect
Copy link
Contributor Author

CC @CyrusNajmabadi

@CyrusNajmabadi
Copy link
Member

I'd recommend including relevant parts from https://github.com/dotnet/csharplang/blob/main/proposals/csharp-7.2/conditional-ref.md. For example, safe-to-return, as well as this being an LValue.

@333fred
Copy link
Member

333fred commented Jul 18, 2023

Modified the original comment to not say closes

@333fred
Copy link
Member

333fred commented Jul 18, 2023

A decent example of the type of thing we're looking for in a spec is https://github.com/dotnet/csharplang/blob/main/proposals/csharp-9.0/target-typed-conditional-expression.md or https://github.com/dotnet/csharplang/blob/main/proposals/csharp-9.0/extension-getenumerator.md. It is going to be harder to get to that level of detail, as there's no checked-in version of the spec. You can base it off the proposal specification here: https://github.com/dotnet/csharplang/blob/main/proposals/csharp-8.0/patterns.md#switch-expression.


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.
Copy link
Member

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:

cond ? ref a : ref b

Clearly the user meant to take a ref here yet they forgot the ref before cond 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:

  1. If all arms of the switch have ref but the overall expression does not.
  2. If there is disagreement in the arms about ref

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)

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

@Rekkonnect Rekkonnect Jul 22, 2023

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:

int a = 4;
ref int b = ref a;
int c = ref a;
int d = a == b ? ref a : ref b;
  • c yields error CS8171 - Cannot initialize a by-value variable with a reference
  • d does not yield any errors and is completely legal, as if the value is immediately dereferenced

Edit: the above examples are not very linked with each other. The initialization of c is an invalid intention, whereas the initialization of d simply discards a reference that is returned from the expression's result. It would be equally as legal to initialize d to a or b 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 designing ref 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants