dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
19.05k stars 4.03k forks source link

Proposal: return, break and continue expressions #14239

Closed alrz closed 7 years ago

alrz commented 8 years ago

Since throw is now an expression, I think return , break and continue are also good candidates to be an expression. Obviously, return shouldn't be legal in expression bodied methods.

var c = obj as C ?? return;
// instead of
if(!(obj is C c)) return;
let C c = obj else return;

var o = F() ?? return;
// instead of
var o = F();
if (o == null) return;

foreach(var item in list) {
  var result = item match {
    case 1: "one",
    case 2: "two",
    case 3: break,
    default: continue,
  };
}

There is no straightforward alternative for the third example as you will need to use the switch statement.

dsaf commented 8 years ago

I am probably not used to expression syntax, but it looks a bit weird to me. I can understand how result can be "one" or "two", but I cannot understand how it can be equal to break or continue unless the latter two will suddenly behave like statements. How will inside of expression be aware of it's surroundings? This I can understand:

foreach(var item in list) {
  var result = item match {
    case 1: "one",
    case 2: "two",
    case 3: null,
    default: null,
  };

  if (result == null) break;
  else continue;
}

I guess this is why C# team insists on keeping and "upgrading" the statement switch form.

dsaf commented 8 years ago

I think it could only work if there was some new var x = foreach () match {}; construct utilising yield return and yield break.

alrz commented 8 years ago

I cannot understand how it can be equal to break or continue unless the latter two will suddenly behave like statements

Same applies to the throw expression. They all are kind of "jump" instructions so that they don't need to return any value because the control will be transferred to another place in any case. The fact that it's syntactically an expression doesn't affect its behavior.

Your alternative surely works for this particular example but in my opinion it's just a step away from being elegant.

I didn't quite get your second comment. I don't think anything else is needed. For example you could just yield return the result of the match expression right away.

I also note that return expression for example, means "immediate return" wherever it is being used. In Rust F(return x); is allowed but F will never be called, therefore, it will be flagged as unreachable code.

DavidArno commented 8 years ago

@alrz,

Consider a slight variation on your example:

string result;
foreach(var item in list) {
  result = item match {
    case 1: "one",
    case 2: "two",
    case 3: break,
    default: continue,
  };
}

What happens to result when case 3 or default are matched? Does it get set to null, or not changed? If the latter, then presumably this will need the compiler to flag a warning/error as result won't be set for some code paths?

dsaf commented 8 years ago

@alrz wasn't this already answered though? https://github.com/dotnet/roslyn/issues/5143#issuecomment-159113202

dsaf commented 8 years ago

Also, would any of the following be legal?

Action x = () => break;

foreach (var i in new [] {1,2,3})
{
    x();
}

x();
dsaf commented 8 years ago

Ok, this answers it: https://users.rust-lang.org/t/matching-error-outside-of-loop-when-replace-return-with-continue/506

I guess this feature is a matter of taste.

alrz commented 8 years ago

@DavidArno The same code rewritten with switch statement does produce an error as result is not definitely assigned. Note that case 3: break; is not equivalent to the case 3: break, in this context because it corresponds to the case label not the foreach statement. So, currently there is no way to break out of a loop in switch's case body.

@dsaf You cannot use break expression out of the context, same is true with a break statement. I'm thinking that ?? return; is a compelling use case because it is unfortunate that you'd just be able to use ?? only when you want to throw. PS: I don't think taking screenshots is necessary, you could just reference the comment. See https://help.github.com/articles/autolinked-references-and-urls/

dsaf commented 8 years ago

@alrz (PS) yeah I know, if only there was a way of embedding the posts instead of referencing. Updated the comment as per your wish.

MgSam commented 8 years ago

@alrz

So, currently there is no way to break out of a loop in switch's case body.

Yes there is.

foreach(var a in b)
{
  switch(a)
  {
    default: goto done; 
  }
}
done: 
alrz commented 8 years ago

@MgSam With goto, nothing is impossible. EDIT: My favorite is goto default;.

MgSam commented 8 years ago
goto InfinityAndBeyond;

Actually, in C#, the goto statement is extremely limited in what it can do compared to C or C++. It's only included for situations exactly as this.

Quoting the language spec:

If a label with the given name does not exist in the current function member, or if the goto statement is not within the scope of the label, a compile-time error occurs. This rule permits the use of a goto statement to transfer control out of a nested scope, but not into a nested scope.

Of course, most people are irrationally afraid of goto statements regardless of the fact that the C# devs constrained them to "safe" usage.

MgSam commented 8 years ago

@DavidArno

The prototypical example in C# is breaking out of nested loops.

foreach(var a in b)
{
    foreach(var c in a)
    {
        if(c == foo) goto exit;
    }
}
exit:

Now try and tell me how that goto there is harming your code in some way. You only dislike it because you've been reading dogma your whole life about how "bad" goto is. The "alternatives" people suggest of factoring out the inner loop to a separate method or using a marker variable for two breaks are both harder to read and maintain.

goto in C# is included specifically for this purpose and for use in switch statements. It cannot jump between methods, and thus can't do most of the horrible things you've heard about in tales from C.

In addition, I suspect you already use goto in your code- you just spell it break, continue, or return.

I digress, this is going off-topic from what this thread is about.

DavidArno commented 8 years ago

@MgSam,

You need to turn your "Now try and tell me how that goto there is harming your code in some way" on it's head, to make it: "If there were no goto, how would I solve this?" Two possible answers:

  1. Replace it with return, ie take those loops and put them in their own function. Which is OK, but isn't really that great a solution, or
  2. Get rid of the nested foreach's, eg by replacing them with a SelectMany...Any linq expression, with no loops and no nesting. The code will more often than not end up being better structured and more readable.

That's my point with "fearing" goto: it's presence likely indicates the code around it isn't "as nice as it could be".

You only dislike it because you've been reading dogma your whole life about how "bad" goto is

Nope, it's because I've spend decades working with crap code (lots of it my own from the past) and goto is a strong indication that I'm going near some pretty horrible code.

HaloFour commented 8 years ago

The Java alternative in those cases being labeled loops and break <label>/continue <label>:

loops:
for (A a : b)
{
    for(C c : a)
    {
        if(c == foo) break loops;
    }
}

Not that it has much bearing on this proposal, aside that even if C# were to adopt said syntax that it would also fit as an expression:

int tally = 0;
loops:
foreach (var a in b) {
    foreach (var c in a) {
       tally += (c != foo) ? 1 : break loops;
    }
}
cypressious commented 7 years ago

The Kotlin programming language has this feature and it turned out to be very valuable, especially in regards to null safety. Kotlin's ?: operator is equivalent to C#'s ?? and the following code is very idiomatic:

val firstItem = list.firstOrNull() ?: return

for (e in listOfNullableValues) {
    val nonNullableE = e ?: continue
}
BoasE commented 7 years ago

I agree with @cypressious . @DavidArno I think it is hard to argument with "cleancode" here. When I've a look at localfunctions which seems to me to be a big smell and they are in the next version.

And they will increase the complexity of the code as they are nesting methods now. The proposed operator avoids unnecessary nesting and therefore reduces complexity.

But thats just my point of view.

DavidArno commented 7 years ago

@Gentlehag,

Since witnessing the train wreck that the design team are making to variable scoping to satisfy their desire to be able to write:

if (!TryX(out var result)) return;
// use result here

I have become a big fan of @alrz's return, break and continue expressions proposal. The above could then be written as:

var result = TryX(out var x) ? x : return;
// use result here

The latter achieves the same result without causing scope leakage.

I was originally sceptical of this idea, but the irresponsible way that the language is being broken to make out var's more useful, coupled with good examples like that from @cypressious, have convinced me that this is a great proposal.

Sadly though, @MadsTorgersen (and especially @CyrusNajmabadi) seem hell-bent on ignoring sage advice on the topic so I doubt this proposal will happen (and variable scope leakage will happen). 😞

DavidArno commented 7 years ago

@Gentlehag,

Oh and for what it's worth, I 100% agree with you with regards to local functions too. We asked for implicitly typed lambdas and instead got given the nested method groups code smell. 😖

CyrusNajmabadi commented 7 years ago

Sadly though, @MadsTorgersen (and especially @CyrusNajmabadi) seem hell-bent on ignoring sage advice on the topic so I doubt this proposal will happen (and variable scope leakage will happen).

Nothing has been ignored. There is just a difference of opinion on the matter. :)

Thanks for your contributions here. We always find them valuable and helpful when we discuss the matters internally!

DavidArno commented 7 years ago

@CyrusNajmabadi

Difference of opinion:

Yes, we understand that this is a deeply unpopular decision that almost no one here agrees with, but we have received so many requests for out var that, in our opinion, it's worth compromising the integrity of the language in order to deliver a really useful implementation of this feature.

Ignoring sage advice:

Github community: "Hey guys, not so sure about this. How about you delay out var's to give everyone time to help you come up with a proper solution? Please? Please listen to us! Please don't ignore us!! 😢 " Language team: "Nothing has been ignored. There is just a difference of opinion on the matter. :) Thanks for your contributions here. We always find them valuable and helpful when we discuss the matters internally!

Spot the difference?

CyrusNajmabadi commented 7 years ago

I'm sorry we haven't been able to produce a solution you like :( . We've considered a lot of feedback (including many participants at our recent MVP summit), and we're more confident now that the set of trade-offs we've chosen are appropriate for the next version of the language.

Please? Please listen to us! Please don't ignore us!! 😢 "

You have been listened to, and you haven't been ignored. The language team discussed this issue at length and incorporated all our feedback avenues into the final design. Please don't mistake us deciding on something different as 'not listening' or 'ignoring'. We evaluated many possible designs and we weighed out the pros and cons. We brought in many voices into the discussion, and we evaluate the impact of our decisions on many styles of code. In the end, this imperfect solution won out over all the other imperfect solutions we were considering. :)

Yes, we understand that this is a deeply unpopular decision

This is your perspective. Our own investigations on the topic has not indicated that this is the case. Again, i'm sorry that we weren't able to come up with a design that you like. We really do appreciate the feedback and will continue to use it when shaping future changes. Thanks!

CyrusNajmabadi commented 7 years ago

Now, if possible, can we get back on topic? Having a meta-discussion about how people feel when the LDM makes decisions they don't like isn't conducive to the topic at hand. Let's try to be respectful and accepting that not everything will go the way one always wants it to (lord knows that's how i've felt with every programming language a lot of the time :-)), and allow the discussion to focus on the future and not the past. Thanks!

DavidArno commented 7 years ago

@CyrusNajmabadi

I'm sorry we haven't been able to produce a solution you like :(

Really? That seems odd. @MadsTorgersen turned up and said "right, listen up, this is what we are doing", then later on rocked up and said "Scrap that, we are doing this instead". Then stormed up in a huff and said "La, la; la, la. I'm not listening. I've made my decision. Like it or lump it". So at what point did you feel sorry that this amazingly interactive process failed to result in a solution that people liked?

CyrusNajmabadi commented 7 years ago

Please keep the discussion on topic. If you want to talk more about your concerns here, you are welcome to reach me at cyrusn@microsoft.com.

DavidArno commented 7 years ago

@CyrusNajmabadi,

As far as I'm aware, we are discussing why @alrz's excellent proposal of allowing "return, break and continue expressions" couldn't be used as a way of avoiding variable scope leakage and thus is completely on topic IMO. However, I'm happy to defer to @alrz and will of course shut up and move on if he feels it is off-topic.

And no, I absolutely will not contact you via email. Microsoft make great claims that C# is now "developed in the open". That means we discuss language design decisions in the open. Not at MVC conferences; not via email; but right here on GitHub.

CyrusNajmabadi commented 7 years ago

@DavidArno Discussing the topic is fine. Attempting to derail with comments like:

Sadly though, @MadsTorgersen (and especially @CyrusNajmabadi) seem hell-bent on ignoring sage advice on the topic so I doubt this proposal will happen (and variable scope leakage will happen).

is not.

Neither is:

Then stormed up in a huff and said "La, la; la, la. I'm not listening. I've made my decision. Like it or lump it

Please, let's all be respectful, courteous, and professional here. Thanks!

And no, I absolutely will not contact you via email. Microsoft make great claims that C# is now "developed in the open". That means we discuss language design decisions in the open. Not at MVC conferences; not via email; but right here on GitHub.

We also discuss things over email, and any of the many mediums that allow one to converse without muddying up another topic. This is the last i'm going to say about this. Please keep the discussion on the issue at hand. That means actually discussing the language feature and not your feelings about how your feedback is being treated. If you want to discuss that elsewhere, i welcome that. It's just not appropriate here.

lostmsu commented 7 years ago

I'm going to vouch +1 for at least return expression. It is quite useful for early breaks on nulls similar to throw expression. E.g. someDict[key] ?? return KeyNotFound;

alrz commented 7 years ago

Moved to https://github.com/dotnet/csharplang/issues/176