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

Allow ref assignment for switch expressions #3326

Open
Rabadash8820 opened this issue Apr 1, 2020 · 12 comments
Open

Allow ref assignment for switch expressions #3326

Rabadash8820 opened this issue Apr 1, 2020 · 12 comments
Assignees
Labels
Needs Approved Specification This issue needs an LDM-approved specification Proposal champion
Milestone

Comments

@Rabadash8820
Copy link

Rabadash8820 commented Apr 1, 2020

This idea was already brought up in #2507, but the conclusion there was that:

I think the switch expression should work the same way [as ternary operators] so I would close this for now

However, switch expressions definitely still do not work the same way as ternary operators with respect to ref assignment, as others have noted in the Roslyn repo:

Long story short, suppose I have a Direction enum and want to conditionally do a ref assignment by switching on a Direction instance, like so:

 switch (direction)
{
     case Direction.North: return ref NorthField;
     case Direction.South: return ref SouthField;
     case Direction.East: return ref EasstField;
     case Direction.West: return ref WestField;
     default: throw new NotImplementedException();
}

Currently, Visual Studio suggests converting the switch statement to an expression, generating the following invalid code:

return direction switch
{
     Direction.North => ref NorthField;
     Direction.South =>ref SouthField;
     Direction.East => ref EasstField;
     Direction.West => ref WestField;
     _ => throw new NotImplementedException();    
};

Each ref expression has a red squiggly saying Invalid expression term 'ref', even if I add ref after the return. If I keep the ref after the return and delete all the case-specific refs, then the whole switch block gets squiggled with the message An expression cannot be used in this context because it may not be passed or returned by reference.

I won't say "this should be easy to implement", cause I'm sure its not, but I see no reason why a ref assignment/return with a switch expression should be invalid syntax. Based on the conversation in #2507, I'm guessing the most likely implementation is to require ref after the return and in each case expression, but less typing is always better 🙂

@333fred
Copy link
Member

333fred commented Apr 1, 2020

@CyrusNajmabadi for the bad VS suggestion.

@CyrusNajmabadi
Copy link
Member

@alrz :)

@Rabadash8820
Copy link
Author

@333fred I believe the bad VS suggestion has already been addressed by dotnet/roslyn#40236. @CyrusNajmabadi recommended I open a new issue here for the actual language feature request.

@AJosephsen
Copy link

It seems inconsistent that you can do this with the ? operator but not with switch expressions

I am able to do the following with the ? operator
(lowerBound < upperBound ? ref lowerBound : ref upperBound) = mid;
But not with the switch expression

(lowerBound < upperBound ) switch
{
	(true) => ref lowerBound,
	(false) =>ref upperBound,
} = mid;

It just feels wrong that you are not able to do this in switch expressions

@333fred 333fred added this to the Any Time milestone Jul 13, 2020
@333fred 333fred moved this from TRIAGE NEEDED to Any Time in Language Version Planning Jul 13, 2020
@333fred 333fred added the Needs Approved Specification This issue needs an LDM-approved specification label Oct 15, 2020
@333fred 333fred removed this from Any Time in Language Version Planning Feb 6, 2021
@glenn-slayden
Copy link

(lowerBound < upperBound ) switch
{
	(true) => ref lowerBound,
	(false) =>ref upperBound,
} = mid;

To be clear, there would be few more ref mentions in the more maximal case of a ref-local assignment usage:

//   v----- these -----v 
    ref double r_mid = ref (lowerBound < upperBound ) switch
    {
        true => ref lowerBound,
        false => ref upperBound,
    };
    r_mid = mid;        

    // more later to amortize setting up 'r_mid', etc...
    r_mid = updated_mid;

This is of course for consistency with standard/existing ref-local syntax applied to your ternary example:

ref double r_mid = ref (lowerBound < upperBound ? ref lowerBound : ref upperBound);

So now the only question is, when might it happen? I was actually quite surprised to discover this lurking omission, having been spoiled, in general, by the always impossibly-stellar C# design enterprise.

@333fred
Copy link
Member

333fred commented Sep 28, 2021

So now the only question is, when might it happen?

This issue is the any time bucket, meaning that it is open for community contributions (and likely won't happen until then). Currently, it needs an approved specification.

@Rekkonnect
Copy link
Contributor

Rekkonnect commented Nov 3, 2021

Adding to this, should the ability to specify ref only once outside the body instead of in every case be considered? I can already foresee this being relatively repetitive in many cases. Something like,

return direction switch ref
{
     Direction.North => NorthField,
     Direction.South => SouthField,
     Direction.East => EastField,
     Direction.West => WestField,
     _ => throw new NotImplementedException(),
};

Obviously the ref would not actually apply to throw expressions, but their usage should still be permitted.

@Rekkonnect
Copy link
Contributor

Rekkonnect commented Nov 29, 2021

I've started creating a primitive spec for the current standing of the proposal to help this move forward.

@333fred
Copy link
Member

333fred commented Nov 29, 2021

A switch expression that returns a ref value does not need an extra ref in front of it when assigned/returned to a ref local.

FYI this is extremely unlikely to fly. The rules should be exactly the same as ref ternaries.

@Rekkonnect
Copy link
Contributor

Rekkonnect commented Nov 29, 2021

Now that I'm thinking about it, it makes sense to require that. I'll have it changed.

EDIT: Done

@CyrusNajmabadi
Copy link
Member

I'll champion this.

@CyrusNajmabadi
Copy link
Member

@Rekkonnect WRT to your spec, how close is it to the language/approach taken for ref-conditional expressions? Can you make a PR on this, and also link to the relevant existing ref-specs so we can compare?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Approved Specification This issue needs an LDM-approved specification Proposal champion
Projects
None yet
Development

No branches or pull requests

7 participants