dotnet / csharplang

The official repo for the design of the C# programming language
11.5k stars 1.03k forks source link

Discussion: Conditional branches, `return when (...) with (...)` #453

Closed lachbaer closed 7 years ago

lachbaer commented 7 years ago

Idea

Many consider it a bad habit to break the code flow with jump instructions like return, break and continue. One valid reason is, that the points of the jump are easily overseen.

A very common scenario are conditional returns, one e.g. being the check for null.

I want to discuss a syntax, that would allow for a cleaner appearance of the conditional 'jumps'.

See the following (made-up) code example:

public static IEnumerable<T> GetValueTypeItems<T>(
  IList<T?> collection, params int[] indexes)
  where T : struct
{
  if (collection == null)
  {
    yield break;
  }

  foreach (int index in indexes)
  {
    T? item = collection[index];
    if (item != null) yield return (T)item;
  }
}

With a syntax like

the above code could be written like

public static IEnumerable<T> GetValueTypeItems<T>(
  IList<T?> collection, params int[] indexes)
  where T : struct
{
  yield break when (collection == null);

  foreach (int index in indexes)
  {
    T? item = collection[index];
    yield return when (item != null)
                 with ((T)item);
  }
}

when and with can be interchanged.

    yield return with ((T)item)
                 when (item != null);

Syntax

I chose when and with, because they are more contextual than if. with sounds quite verbose to me and does in this context not conflict with the with for record types.

Practical example

In theory the idea might look unnecessary. But lease look at the following code, copied from https://github.com/dotnet/roslyn/blob/master/src/Compilers/CSharp/Portable/Binder/Binder_Operators.cs#L1415

I have rewritten that function (below) using the proposed syntax. I think that the expression of the code is much better there!

private ConstantValue FoldBinaryOperator(
    CSharpSyntaxNode syntax,
    BinaryOperatorKind kind,
    BoundExpression left,
    BoundExpression right,
    SpecialType resultType,
    DiagnosticBag diagnostics,
    ref int compoundStringLength)
{
    Debug.Assert(left != null);
    Debug.Assert(right != null);

    if (left.HasAnyErrors || right.HasAnyErrors)
    {
        return null;
    }

    // SPEC VIOLATION: see method definition for details
    ConstantValue nullableEqualityResult = TryFoldingNullableEquality(kind, left, right);
    if (nullableEqualityResult != null)
    {
        return nullableEqualityResult;
    }

    var valueLeft = left.ConstantValue;
    var valueRight = right.ConstantValue;
    if (valueLeft == null || valueRight == null)
    {
        return null;
    }

    if (valueLeft.IsBad || valueRight.IsBad)
    {
        return ConstantValue.Bad;
    }

    if (kind.IsEnum() && !kind.IsLifted())
    {
        return FoldEnumBinaryOperator(syntax, kind, left, right, diagnostics);
    }

    // Divisions by zero on integral types and decimal always fail even in an unchecked context.
    if (IsDivisionByZero(kind, valueRight))
    {
        Error(diagnostics, ErrorCode.ERR_IntDivByZero, syntax);
        return ConstantValue.Bad;
    }

    object newValue = null;

    // Certain binary operations never fail; bool & bool, for example. If we are in one of those
    // cases, simply fold the operation and return.
    //
    // Although remainder and division always overflow at runtime with arguments int.MinValue/long.MinValue and -1 
    // (regardless of checked context) the constant folding behavior is different. 
    // Remainder never overflows at compile time while division does.
    newValue = FoldNeverOverflowBinaryOperators(kind, valueLeft, valueRight);
    if (newValue != null)
    {
        return ConstantValue.Create(newValue, resultType);
    }

    ConstantValue concatResult = FoldStringConcatenation(kind, valueLeft, valueRight, ref compoundStringLength);
    if (concatResult != null)
    {
        if (concatResult.IsBad)
        {
            Error(diagnostics, ErrorCode.ERR_ConstantStringTooLong, syntax);
        }

        return concatResult;
    }

    // Certain binary operations always fail if they overflow even when in an unchecked context;
    // decimal + decimal, for example. If we are in one of those cases, make the attempt. If it
    // succeeds, return the result. If not, give a compile-time error regardless of context.
    try
    {
        newValue = FoldDecimalBinaryOperators(kind, valueLeft, valueRight);
    }
    catch (OverflowException)
    {
        Error(diagnostics, ErrorCode.ERR_DecConstError, syntax);
        return ConstantValue.Bad;
    }

    if (newValue != null)
    {
        return ConstantValue.Create(newValue, resultType);
    }

    if (CheckOverflowAtCompileTime)
    {
        try
        {
            newValue = FoldCheckedIntegralBinaryOperator(kind, valueLeft, valueRight);
        }
        catch (OverflowException)
        {
            Error(diagnostics, ErrorCode.ERR_CheckedOverflow, syntax);
            return ConstantValue.Bad;
        }
    }
    else
    {
        newValue = FoldUncheckedIntegralBinaryOperator(kind, valueLeft, valueRight);
    }

    if (newValue != null)
    {
        return ConstantValue.Create(newValue, resultType);
    }

    return null;
}

Rewritten with return when () while ():

private ConstantValue FoldBinaryOperator(
    CSharpSyntaxNode syntax,
    BinaryOperatorKind kind,
    BoundExpression left,
    BoundExpression right,
    SpecialType resultType,
    DiagnosticBag diagnostics,
    ref int compoundStringLength)
{
    Debug.Assert(left != null);
    Debug.Assert(right != null);

    return when (left.HasAnyErrors || right.HasAnyErrors) with (null);

    // SPEC VIOLATION: see method definition for details
    ConstantValue nullableEqualityResult = TryFoldingNullableEquality(kind, left, right);
    return when (nullableEqualityResult != null) with (nullableEqualityResult);

    var valueLeft = left.ConstantValue;
    var valueRight = right.ConstantValue;
    return when (valueLeft == null || valueRight == null) with (default);

    return when (valueLeft.IsBad || valueRight.IsBad) with (ConstantValue.Bad);

    return when (kind.IsEnum() && !kind.IsLifted())
           with (FoldEnumBinaryOperator(syntax, kind, left, right, diagnostics));

    // Divisions by zero on integral types and decimal always fail even in an unchecked context.
    return when (IsDivisionByZero(kind, valueRight)) with (ConstantValue.Bad)
        Error(diagnostics, ErrorCode.ERR_IntDivByZero, syntax);

    object newValue = null;

    // Certain binary operations never fail; bool & bool, for example. If we are in one of those
    // cases, simply fold the operation and return.
    //
    // Although remainder and division always overflow at runtime with arguments int.MinValue/long.MinValue and -1 
    // (regardless of checked context) the constant folding behavior is different. 
    // Remainder never overflows at compile time while division does.
    newValue = FoldNeverOverflowBinaryOperators(kind, valueLeft, valueRight);
    return when (newValue != null)
           with (ConstantValue.Create(newValue, resultType));

    ConstantValue concatResult = FoldStringConcatenation(kind, valueLeft, valueRight, ref compoundStringLength);
    return when (concatResult != null) with (concatResult)
    {
        if (concatResult.IsBad)
        {
            Error(diagnostics, ErrorCode.ERR_ConstantStringTooLong, syntax);
        }
    }

    // Certain binary operations always fail if they overflow even when in an unchecked context;
    // decimal + decimal, for example. If we are in one of those cases, make the attempt. If it
    // succeeds, return the result. If not, give a compile-time error regardless of context.
    try
    {
        newValue = FoldDecimalBinaryOperators(kind, valueLeft, valueRight);
    }
    catch (OverflowException)
    {
        Error(diagnostics, ErrorCode.ERR_DecConstError, syntax);
        return ConstantValue.Bad;
    }

    return with (ConstantValue.Create(newValue, resultType))
           when (newValue != null);

    if (CheckOverflowAtCompileTime)
    {
        try
        {
            newValue = FoldCheckedIntegralBinaryOperator(kind, valueLeft, valueRight);
        }
        catch (OverflowException)
        {
            Error(diagnostics, ErrorCode.ERR_CheckedOverflow, syntax);
            return ConstantValue.Bad;
        }
    }
    else
    {
        newValue = FoldUncheckedIntegralBinaryOperator(kind, valueLeft, valueRight);
    }

    return when (newValue != null)
           with (ConstantValue.Create(newValue, resultType));

    return null;
}
HaloFour commented 7 years ago

You aren't eliminating any jumps, you're just disguising them. The control flow logic is exactly the same.

lachbaer commented 7 years ago

@HaloFour

You aren't eliminating any jumps, you're just disguising them

It's not my intent to eleminate them. And I would rather say that this syntax dis- disguises the jumps!

And to me, I love the appearance of

    return when (valueLeft == null || valueRight == null) 
           with (default);
    return when (valueLeft.IsBad || valueRight.IsBad)
           with (ConstantValue.Bad);
    return when (kind.IsEnum() && !kind.IsLifted())
           with (FoldEnumBinaryOperator(syntax, kind, left, right, diagnostics));

    return when (IsDivisionByZero(kind, valueRight)) with (ConstantValue.Bad)
        Error(diagnostics, ErrorCode.ERR_IntDivByZero, syntax);

or

    return with (default)
           when (valueLeft == null || valueRight == null);
    return with (ConstantValue.Bad)
           when (valueLeft.IsBad || valueRight.IsBad);
    return with (FoldEnumBinaryOperator(syntax, kind, left, right, diagnostics))
           when (kind.IsEnum() && !kind.IsLifted());

    return with (ConstantValue.Bad) when (IsDivisionByZero(kind, valueRight))
        Error(diagnostics, ErrorCode.ERR_IntDivByZero, syntax);

instead of

    if (valueLeft == null || valueRight == null)
    {
        return null;
    }
    if (valueLeft.IsBad || valueRight.IsBad)
    {
        return ConstantValue.Bad;
    }
    if (kind.IsEnum() && !kind.IsLifted())
    {
        return FoldEnumBinaryOperator(syntax, kind, left, right, diagnostics);
    }

    if (IsDivisionByZero(kind, valueRight))
    {
        Error(diagnostics, ErrorCode.ERR_IntDivByZero, syntax);
        return ConstantValue.Bad;
    }
HaloFour commented 7 years ago

I don't. It's a lot of extra syntax for zero benefit. You can already achieve a terser form by simply omitting the blocks:

if (valueLeft == null || valueRight == null) return null;
if (valueLeft.IsBad || valueRight.IsBad) return ConstantValue.Bad;
if (kind.IsEnum() && !kind.IsLifted()) return FoldEnumBinaryOperator(syntax, kind, left, right, diagnostics);

if (IsDivisionByZero(kind, valueRight)) {
    Error(diagnostics, ErrorCode.ERR_IntDivByZero, syntax);
    return ConstantValue.Bad;
}

Your last "statement" doesn't even make sense as you're implying that some kind of embedded-statement or block can follow the return? Your code jumps around more than the code with actual jumps.

DavidArno commented 7 years ago

If you reduced down the syntax to:

return <value> when <guard expression>;

then this proposal is effectively adding when guards, which we have in catch and case statements already, to return and yield.

However, as such guards can already be expressed as if (<guard expression>) return <value>;, one has to ask: what's the point, beyond "lov[ing] the appearance" of one syntactic form over another?

lucamorelli commented 7 years ago

I have a similar problem while doing model validation at the beginning of method calls, when If have to add several preconditions checks. Obviously there are a lot of possible way, but often ends with several

if ([condition1])
  return (code1);
if ([condition1])
  return (code2);
....

this lead often to have a beginning of the code that is a bit confused, long, and hard to share between method calls with similar model. One scenario is the validation of parameters of method calls with several parameters.

In this situation it's something annoying because you need at least 2/3 rows of code ofr every check.

some sample (it's depending on the type of data) could be:

if (!Preconditions.IsValidVatCode(vatCodeParam))
  return false;
if (!Preconditions.IsValidZipCode(zipCodeParam))
  return false;
if (!Preconditions.IsValidCity(cityCodeParam))
  return false;

In my case it depends of data values

lachbaer commented 7 years ago

@lucamorelli In your case this could be rewritten like

return when (!Preconditions.IsValidVatCode(vatCodeParam)
          || !Preconditions.IsValidZipCode(zipCodeParam)
          || !Preconditions.IsValidCity(cityCodeParam))
       with (false);

@DavidArno

what's the point, [...] of one syntactic form over another

THE point is actually to put the relevant code part - namely the branch/return - at the beginning of the code line!

See, I'm talking about the looks now:

if (obj == null) return;
// or
if (list.Count == 0) return null;

This primarily looks like an if clause that, when hiding the following statement, looks as if it is part of the calculatoric algorithm. Following the if can be anything, a variable definition, a method call, etc.

return when (obj == null);
// or
return with (null) when (list.Count == 0);

This is actually the same statement, but this time you see immediately that the whole line is about the branching/returning. The focus is shift towards then return! Same for

if (list.Count == 0) {
    Console.WriteLine("List is empty, nothing to be done...");
    return null;
}

Here again, the purpose of the whole if-block is to return with a log message. But on the first sight it does not really distinguish from any other if that is used for the actual calculations the method is doing. The real purpose is more visible from this:

return when (list.Count == 0) with (null) {
    Console.WriteLine("List is empty, nothing to be done...");
}    

(no, it's not really shorter, but that's not the idea anyway). The intention of the programmer here is to return at this point - and just issue a log message additionally.

Of course you can violate this construct to irritate the reader. But you could do that with other constructs as well, so this doesn't count as an argument.

Besides, what is the purpose of LINQ queries? Every query can be expressed as good old plain C# method calls. Null-respecting operators don't add real value either. They, too, can be expressed in two lines of code. [I don't mean that for real!]

This whole idea is not about creating new ways, it is about making the expression, the real purpose of that lines of code much more expressive - that's all.

@HaloFour

return with (ConstantValue.Bad) when (IsDivisionByZero(kind, valueRight))
     Error(diagnostics, ErrorCode.ERR_IntDivByZero, syntax);

Your last "statement" doesn't even make sense as you're implying that some kind of embedded-statement or block can follow the return? Your code jumps around more than the code with actual jumps.

Yes, it absolutely makes sense. Then you haven't understood the meaning of this idea - sorry! I wouldn't go for the way to write the statement without braces. I just did it that one time to show that it could also be done that way, like with standard if-clauses.

The purpose of allowing statement(s) behind the return when(...) is to "unwind" the return statement from the inner block, so that anyone can see at once, that the only purpose of this block is to return (or break, yield, goto, maybe even continue).

HaloFour commented 7 years ago

@lachbaer

Yes, it absolutely makes sense. Then you haven't understood the meaning of this idea - sorry!

No, I got it. It just doesn't make sense. The idea that this "construct" can be followed by an embedded-statement which is executed and then it performs the return is crazy. It completely violates the expected flow to control code in C# (or frankly any imperative programming language).

There's a reason you write if before return, the if happens first, and is more important.

jnm2 commented 7 years ago

@lachbaer

return when (!Preconditions.IsValidVatCode(vatCodeParam)
          || !Preconditions.IsValidZipCode(zipCodeParam)
          || !Preconditions.IsValidCity(cityCodeParam))
       with (false);

I think this looks cleaner:

if (!Preconditions.IsValidVatCode(vatCodeParam)
 || !Preconditions.IsValidZipCode(zipCodeParam)
 || !Preconditions.IsValidCity(cityCodeParam))
{
    return false;
}
lucamorelli commented 7 years ago

@jnm2 it depends on data. In this example could work becase you have the same return type, but if you have

if (!Preconditions.IsValidVatCode(vatCodeParam)) return Error.Vat; if (!Preconditions.IsValidZipCode(zipCodeParam)) return Error.Zip if (!Preconditions.IsValidCity(cityCodeParam)) return Error.City;

this doesn't work

lachbaer commented 7 years ago

@HaloFour

the if happens first, and is more important.

I totally do not agree on being more important! Different people, different culture, different view...

It completely violates the expected flow to control code in C#

A bit of, yes. But I didn't see any other possibility to put the return at the beginning of the construct and still having the opportunity to execute some side-code.

lachbaer commented 7 years ago

@lucamorelli

return when (!Preconditions.IsValidVatCode(vatCodeParam)) with (Error.Vat);
return when (!Preconditions.IsValidZipCode(zipCodeParam)) with (Error.Zip);
return when (!Preconditions.IsValidCity(cityCodeParam)) with (Error.City);

PS: you can highlight the code by using ```cs at the beginning instead of just ```.

HaloFour commented 7 years ago

@lucamorelli

It works just fine:

if (!Preconditions.IsValidVatCode(vatCodeParam)) return Error.Vat;
if (!Preconditions.IsValidZipCode(zipCodeParam)) return Error.Zip;
if (!Preconditions.IsValidCity(cityCodeParam)) return Error.City;

@lachbaer

I totally do not agree on being more important! Different people, different culture, different view...

C# is a programming language. It's syntax and order has already been decided. To introduce more dialects just to make it more comfortable to non-English speakers would only make it unnecessarily complicated.

A bit of, yes. But I didn't see any other possibility to put the return at the beginning of the construct and still having the opportunity to execute some side-code.

There's a reason for this.

lucamorelli commented 7 years ago

@HaloFour I know, just I don't like write code this way because to keep everything in one line you can have a series of rows with different length. This Thing that make harder to find the main parts of the control while reading. Even worst is the idea of align all the returns to the same column.

lachbaer commented 7 years ago

@HaloFour Are you reading from left-to-right? I guess so. :wink: Close your right eye (symbolically speaking) and tell me what is going to happen:

if (!Preconditions.IsValidVatCode(vatCodeParam)) [...can't see this]
if (!Preconditions.IsValidZipCode(zipCodeParam)) [...can't see this]
if (!Preconditions.IsValidCity(cityCodeParam)) [...can't see this]

What is going to happen...?

Now again:

return when (!Preconditions.IsValidVatCode(vatCodeParam)) [...can't see this]
return when (!Preconditions.IsValidZipCode(zipCodeParam)) [...can't see this]
return when (!Preconditions.IsValidCity(cityCodeParam)) [...can't see this]

And again:

return with(Error.Vat) [...can't see this]
return with(Error.Zip) [...can't see this]
return with(Error.City) [...can't see this]

Because of return with you know a mandatory condition is comming. Because of return when you know a return value must come (when method isn't void).

Also, return when gives you both, the essence of the condition and the condition at the beginning:

  // oh, an `if`. Must be of importance to the algorithm
if (!Preconditions.IsValidVatCode(vatCodeParam)
  // where is the statement...? Ah, another condition is coming:
 || !Preconditions.IsValidZipCode(zipCodeParam)
  // but now I expect to see what `if` is doing - ah, no, another condition...
 || !Preconditions.IsValidCity(cityCodeParam))
  // certainly there is a further condition here?
  // Oh yes, a block begins, but now I'm already too tired to keep reading,
  // I'm more interested in the actual algorithm
{
    return false;
}

// vs.
return with (false) // oh, this is only a "jump out" - not interested in reading now, 
                   // I'm skipping this part for now
       when (!Preconditions.IsValidVatCode(vatCodeParam) ...

In case you are not interested in premature returns (as a reader) the algorithms are more easily to see for you.

Same, when you are interested in the returns.

if (!Preconditions.IsValidVatCode(vatCodeParam))
{
    return false;
}

What's your reading flow now? You will search for the return, and then go upwards to see the condition. The reading "completely violates the expected flow to control code in C#" :wink:

HaloFour commented 7 years ago

@lachbaer

Close your right eye (symbolically speaking) and tell me what is going to happen

Tells me that nothing is going to happen unless the condition is met. That's more important than what might actually happen.

Solves no new problems. More verbose than existing solution. Completely redundant.

lachbaer commented 7 years ago

@HaloFour I think we both will not agree upon this finally. But I would be happy if you had some constructive arguments that will make me think. I think I have already given enough.

jnm2 commented 7 years ago

@lachbaer This doesn't do much to make returns easier to find because it only handles the case of non-nested if statements. There are many other ways to nest return statements, such as switches and loops and try blocks. It won't stop us from having to scan the whole method to find all the exit points. What does help with that is the IDE. Put your cursor on one and it highlights all the exit points.

lachbaer commented 7 years ago

@jnm2 I also (somewhere) proposed to highlight branching keywords differently in the IDE, e.g. violet instead of standard blue. No, it doesn't solve the problem completely. But when you look at your code, I bet the most occurences of intra-method returns would profit in readability. Just look at the Roslyn code in the very first post.

@HaloFour

To introduce more dialects just to make it more comfortable to non-English speakers would only make it unnecessarily complicated.

Yes, please tell that to the translators of Visual Studio! 🀣 I always install English IDEs, because the German translations make me sick! πŸ€’ But then, what is the point of having translations anyway...?! Sorry to say, but I think that your opinion on that is a bit illiberal. In an open world a modern programming language should be open to other cultures as well, as long as it stays understandable for all cultures.

HaloFour commented 7 years ago

@lachbaer

In an open world a modern programming language should be open to other cultures as well, as long as it stays understandable for all cultures.

A programming language should have simple concise rules and, preferably, exactly one way to accomplish something. Additional ways should only be introduced if they provide a significant benefit. Programming languages should not bother to approach the huge variety of nuances and grammatical differences between the litany of possible spoken languages. Attempting to chase dialects, SVO, conjugation, alternate vocabularies, etc. would only result in making the language near impossible to learn and understand.

lachbaer commented 7 years ago

@HaloFour

Additional ways should only be introduced if they provide a significant benefit.

And that is what I see in this proposal. We can argue about the syntax or the one or other point, but not on the issue of putting branching keywords at the beginning of a statement.

As just a quick idea, an intermediate way could be to not using the return keyword as the first one.

when (!Preconditions.IsValidVatCode(vatCodeParam))
    return Err.VAT;

It is actually equivalent to if with two major differences:

  1. when has that different appearance as a keyword, introducing a branch
  2. the following statement (or block) must end with a branch

Especially 1. is important to me! (And it needn't to be when, e.g. could be a new branch.)

When you work in a team and have others read your code, e.g. in an open source project, or when you return to your own code after a while, don't you think that there are enough premature returns that can be seen easier with a different construct? Have a thoughtful look at your code. πŸ˜ƒ

HaloFour commented 7 years ago

@lachbaer

Solves no new problems. More verbose than existing solution. Completely redundant.

This proposal provides no benefits except to make you feel better about the order of grammar in your code. If you really want it I suggest forking C# and making your own language.

When you work in a team and have others read your code, e.g. in an open source project, or when you return to your own code after a while, don't you think that ...

I work on a team of well over several hundred developers, within an organization of several dozen teams. We maintain our own products as well as dozens of open source projects. What I like, above all, is consistency. One way to do something. Not 50 based on subjective ideas of in which order grammatical structures should happen to appear. One syntax to rule them all and in the language specification bind them.

lachbaer commented 7 years ago

@HaloFour

If you really want it I suggest forking C# and making your own language

That's gross! 😞

I work on a team of well over several hundred developers, within an organization of several dozen teams.

And that is what is giving you probably blinkers, no offense!

What I like, above all, is consistency. One way to do something

That's what guidelines are for, no matter what structures the language offers.

Just imagine for a second - besides all current ways - that since years in all your company's code all conditional returns had the appearance of this proposal, together with the guideline to strictly use it when (conditionally) jumping out of a method prematurely. How would your code look like then? Wouldn't you already have gotten used to it? Maybe even meanwhile love that construct, because it gives the consistency you long for?

HaloFour commented 7 years ago

@lachbaer

That's an irrelevant hypothetical. This proposal isn't about whether or not to use an existing redundant language feature. It's about allocating the resources to modify the compiler and language specification in order to incorporate a new redundant language feature.

If C# always had a completely different syntax for performing conditionals and/or breaking from the current method than I would argue against adding another syntax, even if the proposed syntax was the current syntax. Other languages have their own syntax, and I don't/won't argue that they should incorporate the syntax from C#. If they want to waste their time doing so that's their business.

Do you also argue that C# should adopt VSO syntax? Let's stick the method before the instance. Some language families work like that. Sometimes the O comes first, so let's stick the arguments before the method. In some languages negation is a post-fix modifier, so let's support ! as a suffix operator. Why not make every keyword translatable? Use whatever brackets are easiest for whichever keyboard we're dealing with at the moment?

Programming languages aren't supposed to be these things. I pity the programming language that tries.

lachbaer commented 7 years ago

@HaloFour Now you are overreacting. What a pity, I thought we could have a productive discussion here.

I never ever went the way to support multiple cultures in whatever way. But at least they must be respected somehow when taking about changes, which so ever.

The conditional returns are one of the most used patterns. The argumentation against premature returns is just theoretical, because in C# at least that is common practice all over the place, and though it never ends because of the importance of the concept. The appearance of that "special" construct, that does not differ from any other ordinary if, alone should be argument enough to undergo a review.

DavidArno commented 7 years ago

@lachbaer,

Serious question here: have you tried coding using ruby? The mass of proposals that you have created around if, null and expression order are all - as far as I can remember - addressed by that language. Give it a try; you might love it.

HaloFour commented 7 years ago

@lachbaer

But at least they must be respected somehow when taking about changes, which so ever.

The language should remain consistent with itself above all. What other cultures do doesn't matter; the C# culture matters.

The conditional returns are one of the most used patterns.

Yes it is.

The argumentation against premature returns is just theoretical, because in C# at least that is common practice all over the place, and though it never ends because of the importance of the concept.

I'm not sure why you bring this up. The people making such arguments would be just as opposed to your syntax as it is the fact that the control flow exits the method early. They're also generally opposed to break and continue within loops, throwing exceptions, etc.

For the rest of us that have no problems exiting methods early where warranted, we already have perfectly good syntax to do so. Your syntax does nothing but invert the condition and the action because you think it looks nicer that way, despite being more verbose. That's far from reason enough to bother with the time and effort required to modify the compiler and the language specification, in my opinion.

lachbaer commented 7 years ago

@DavidArno

have you tried coding using ruby

Actually never had a look into it yet.

@HaloFour

I'm not sure why you bring this up. The people making such arguments would be just as opposed to your syntax as it is the fact that the control flow exits the method early.

I expressed myself badly. I meant to say that despite their reasoning is understood by most programmers, nevertheless the contrary has become common practice in C style languages. One of their main arguments, as far as I remember, is that the jumps can easily be overseen. An argument I agree upon.

This proposal is an attempt to add a construct to C# that in no means introduces a new concept, but instead "reformats" a common and heavily used pattern in a way that pays its importance justice - finally in a way that fits the language of course.

That's far from reason enough to bother with the time and effort required to modify the compiler and the language specification

That's not our call to make, thankfully ;-)

lachbaer commented 7 years ago

@HaloFour I wonder, if the following approach suits you πŸ˜ƒ

It makes use of attributes on statements/blocks, a non-available feature by now. But adding that would allow for many other future uses. The attributes are there for compile-time only, so no CLR change is needed.

// This is a conditional return, just ensure the following statement returns
[return: Return]
if (obj == null) return null;

// Ensure the following statement returns null
[return: Return.Null]
if (valueLeft == null || valueRight == null) return null;

// Ensures the following statement returns the type
[return: Return.Type(typeof(ErrorEnum))]
if (!Preconditions.IsValidVatCode(vatCodeParam)) return Error.Vat;

// Ensures the following statement returns the value
[return: Return.Value(0)]
if (!Preconditions.IsValidVatCode(vatCodeParam)) return 0;

// The following is a "precondition validation block"
// The block itself __must not__ have *direct* return statement
// but in case of returns, the corresponding type must be met
[return: Return.NoDirectReturn, Return.Type(typeof(ErrorEnum))]
{
    if (!Preconditions.IsValidVatCode(vatCodeParam)) return Error.Vat;
    if (!Preconditions.IsValidZipCode(zipCodeParam)) return Error.Zip;
    if (!Preconditions.IsValidCity(cityCodeParam)) return Error.City;
}

// the following switch must return, and it must not return "null"
[return: Return.NotNullOrDefault]
switch (index) {
    case 1: return object1; break;
    case 2: return object2; break;
    default: return defaultobject; break;
}

// this is just a hint, compiler issues error if no return is found at all
[return: Return.HasReturn]
switch (index) {
    case 1: return object1; break;
    case 2: return object2; break;
    default: continue; break;
}

The used attribute classes (e.g. Return.ValueAttribute) would be nested within the System.ReturnAttribute class.

The [return: ...] target can be reused for this purpose (to make the construct stand out more, maybe it even helps the compiler).

This approach would actually serve the same purpose of this proposal in a really typical C# style. Additionally it would add compile-time-attributes on statements/blocks, that can be used in many other upcoming scenarios.

A further advantage of this syntax is that it can loosen the guideline of always surrounding if-blocks in {}, even in case of a simple return, because the initial [return: ...] attribute is sounding enough, uncluttering the code from braces.

CyrusNajmabadi commented 7 years ago

I have no idea what this code means:

return with (ConstantValue.Bad) when (IsDivisionByZero(kind, valueRight))
     Error(diagnostics, ErrorCode.ERR_IntDivByZero, syntax);

... it's... returning "ConstantValue.Bad"... if "IsDivisionByZero(...)" returns true itself... and it also errors (maybe) before return ConstantValue.Bad?

Is that it?

What if i wrote:

return with (ConstantValue.Bad) when (IsDivisionByZero(kind, valueRight))
Error(diagnostics, ErrorCode.ERR_IntDivByZero, syntax);

But i meant:

return with (ConstantValue.Bad) when (IsDivisionByZero(kind, valueRight));
Error(diagnostics, ErrorCode.ERR_IntDivByZero, syntax);

That's exceptionally hard to see the difference between the two...

yaakov-h commented 7 years ago

@CyrusNajmabadi It's taken me about 40 seconds to spot the difference between those two examples. 😞

lachbaer commented 7 years ago

@CyrusNajmabadi @yaakov-h First, me myself I do not like that either. But I put it there for the sake of language consistency.

But for a good argumentation, I will dig in it, though this post is not about this proposal. It is nearly all based on human factors, you'll see πŸ˜ƒ

See the following code fragment. What does it do?

String line;
var lines = new List<string>();
do
{
    line = Console.ReadLine();
    if (!line.StartsWith("#"));
        lines.Add(line);
}
while (!line.StartsWith("$"));

There are some issues here. Let's get startet.

  1. It is valid C#
  2. Have you immediately seen then ! in the conditions? Try to find the answer, why you did or didn't.
  3. Have you immediately seen the ; at the end of the if condition? Does the if have to do anything with lines.Add()?
  4. Is it immediately clear to you that it reads lines up to the next one beginning with "$"?

There is no the answer to the questions above. Because we are all humans, with any cultural and environmental background whatsoever, even within the same country.

ad 1) Well, there are most probably ways this could be written better. Nevertheless we are talking about existing specifications here. But I have to play advocate for the "minority" of the programmers "who buy C# books" (according to the argumentations of some participants in this forum that must be so little, poor authers; it's a wonder that they still sell books... [PS: this is not about books πŸ˜‰ ]).

ad 2) You might have seen the !. In this forum there are mainly experienced programmers. You are accustomed to look out for these kind of tokens. But even then, ask yourself if you really understood immediately the negating sense of the expression or if that, too, took you some seconds to realize.

ad 3) I guess this one is a bit harder to notice. Of course this is only a typo and is warned upon by the compiler. But you had to look twice and a bit harder, hadn't you? The following lines.Add() line is indented and just appears as to belong to the if-statement. But it doesn't.

And even if this is a no brainer for you, it shows how accustomed you are to those constructs. If anything like

return with (ConstantValue.Bad) when (IsDivisionByZero(kind, valueRight))
     Error(diagnostics, ErrorCode.ERR_IntDivByZero, syntax);

would already have been in the language for years, you will most probably be accustomed to the apearances, too, and it will also be a no-brainer to you!

ad 4) Again, did you have to think twice?

Now, look at the following code.

do
    ifnot ((line = Console.ReadLine()).StartsWith("#"))
        lines.Add(line);
until (line.StartsWith("$"));

Of course ifnot and until are no valid keywords. But first, ask yourself if it could be valid C# - in the most basic meaning - an whether that above code, though being badly formed, is immediately better understandable what ever your background is.

This proposal here is simply based upon the (non-Android) intent to make a commonly used pattern stand out more and therefore being less prone to accidantial errors as well as being immediately understandable to all people.

CyrusNajmabadi commented 7 years ago

See the following code fragment. What does it do?

It warns:

Warning CS0642  Possible mistaken empty statement

That's because it's really easy to make that sort of mistake. However, with the syntax you have suggested the common case would be the one you warn on.

CyrusNajmabadi commented 7 years ago

would already have been in the language for years, you will most probably be accustomed to the apearances, too, and it will also be a no-brainer to you!

We've avoided such constructs precisely because they can be so easily misinterpreted. We have inherited semicolons and statements from our C background. But we detect and warn in this case because we recognize that it's hard to see and very likely to be a problem. We would not want to slide back here by introducing a new construct that by default has two forms which are very hard to distinguish, but which are vastly different in meaning.

CyrusNajmabadi commented 7 years ago

Of course ifnot and until are no valid keywords. But first, ask yourself if it could be valid C# - in the most basic meaning - an whether that above code, though being badly formed, is immediately better understandable what ever your background is.

This is not necessarily true. We have to consider that the background of many of our customers is precisely the C heritage of families.

as well as being immediately understandable to all people.

This is again, not necessarily true. For example, for the existing millions of people using C# today, this code may now be less understandable. Precisely because it doesn't use the core language constructs they've been familiar with for 10+ years now.

I mean, if we're just going to go with syntax that any person could read and understand, then we should just have:

keep .ReadLine from Console until $it .StartsWith "$" and if $it does not .StartWith "#" .Add $it to lines;

:)

lachbaer commented 7 years ago

@CyrusNajmabadi Actually I'd rather see unbraced statements behind constructs completely vanished! It's not worth a proposal, but can't you introduce a compiler switch that makes them an error?

What do you think of the attribute approach? It doesn't have to be that comprehensive, as I did it above. And statement attributes can be used in other scenarios as well, e.g.

[Condition.CStyle]
for (int i=10; i; i--) { Console.WriteLine(i); }

(That's nonsense, but just came into my mind)

CyrusNajmabadi commented 7 years ago

Sometimes it can be better to just stick with a core few ways of doing things, which you can then teach to people. People learn, they understand, and they move on.

Continuing to try to make the language as intuitively understandable as possible to the novice that comes along is an admirable goal, but can really lead to a mess of a language.

CyrusNajmabadi commented 7 years ago

but can't you introduce a compiler switch that makes them an error?

I'm not sure what you mean. Can you show an example of what you would like us to make an error?

What do you think of the attribute approach?

I really don't understand what is accomplishes.

for (int i=10; i; i--) { Console.WriteLine(i); }

Is this just so you don't have to write "i > 0" in the middle of your for-loop? If so, i don't see the value in us providing that. We think it's valuable to have to be explicit about your condition and to ensure that it's actually a boolean to help prevent mistakes. We're unlikely to change that any time soon.

lachbaer commented 7 years ago

but can't you introduce a compiler switch that makes them an error?

I'm not sure what you mean. Can you show an example of what you would like us to make an error?

Here you go.

/* Errors with "unbraced statement" */
for (int i=10; i > 0; i--) Console.WriteLine(i);

for (int i=10; i > 0; i--)
    Console.WriteLine(i);

/* this is fine */
for (int i=10; i > 0; i--) { Console.WriteLine(i); }
for (int i=10; i > 0; i--) { 
    Console.WriteLine(i);
    }

Not having the braces is one of the major error sources, admittedly! A switch to force it for newly written code would be fine.

Is this just so you don't have to write "i > 0" in the middle of your for-loop?

It was just a syntax sample of statement attributes with no further meaning. Though that one could help to copy-paste legacy C-code algorithms into an unsafe block of a C# program without having to spend too much time for language agnostic adoptions (thus making the 'C' in C# a bit prouder of itself ;-)

yaakov-h commented 7 years ago

Continuing to try to make the language as intuitively understandable as possible to the novice that comes along is an admirable goal, but can really lead to a mess of a language.

You could always try write a variant of AppleScript that runs on the CLR πŸ˜„

CyrusNajmabadi commented 7 years ago

Here you go.

You can get that by writing an analyzer that reports an error when any of these constructs contain an embedded statement that is not a block. Roslyn has an entire supported API for doing this sort of thing.

CyrusNajmabadi commented 7 years ago

It was just a syntax sample of statement attributes with no further meaning.

Then i don't know what purpose you want it to serve, or how it applies to this general discussion. You asked

What do you think of the attribute approach?

You haven't given an example of code where this would be valuable or why I would want this. So, currently, i don't think anything of it :)

lachbaer commented 7 years ago

What do you think of the attribute approach?

I really don't understand what is accomplishes. You haven't given an example of code where this would be valuable or why I would want this.

Please see above

In this concrete proposal marking a statement or block with [return: Return] would serve two purposes.

  1. Unwinding the "deeply hidden" return statement to the beginning of the statement. That way it is much more easy to spot return points.
  2. Assuring that the block actually returns at appropriate places - kind of a built-in test. With if (...) { /* many other statements */ return; } you cannot accomplish a code-analysation for a return without marking the statement itself.
lachbaer commented 7 years ago

You can get that by writing an analyzer that reports an error when any of these constructs contain an embedded statement that is not a block. Roslyn has an entire supported API for doing this sort of thing.

Argh, ... :wink: I don't want to write an analyzer myself, or play around with an actual quite comprehensive API! I want it to be forced to newly written code by everyone - unless they do not switch the switch off.

And I want it to be "forced", because it led, is leading and will lead to errors. When C# shall be that language that makes efforts to stabilize code while already writing it, then this is the way.

CyrusNajmabadi commented 7 years ago

I want it to be forced to newly written code by everyone - unless they do not switch the switch off.

Sorry, our philosophy is to not break user code :)

CyrusNajmabadi commented 7 years ago

I really honestly don't get it. What does this mean:

// This is a conditional return, just ensure the following statement returns
[return: Return]
if (obj == null) return null;

What does the attribute add? Why do i need it? What is it doing?

lachbaer commented 7 years ago

Sorry, our philosophy is to not break user code :)

That's what a switch is for... πŸ₯‡

CyrusNajmabadi commented 7 years ago

You said you wanted it forced on everyone. That means we're breaking their code. We do not do that. If you want an opt-in mechanism, that's fine. That's what analyzers do. People opt into precisely the set of checks they feel are appropriate for them.

CyrusNajmabadi commented 7 years ago
// Ensures the following statement returns the type
[return: Return.Type(typeof(ErrorEnum))]

I don't understand this. The compiler already ensures that types are correct. What does this attribute actually do?

lachbaer commented 7 years ago

In that special case the return type of the function shall be int, but the type returned by the block must be an ErrorEnum. As I said, it needn't be that comprehensive. This is a discussion and not a very concrete proposal. It should only show a way that could be gone.

lachbaer commented 7 years ago

You said you wanted it forced on everyone. That means we're breaking their code.

I said I want it forced on everyone for newly written code. That will break nothing. That could be achieved e.g. by starting every file with #pragma braced. A checkbox in the project settings will do that automatically for you on new files.