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
18.97k stars 4.03k forks source link

C# Design Notes for Jul 15, 2016 #12939

Closed MadsTorgersen closed 7 years ago

MadsTorgersen commented 8 years ago

C# Design Language Notes for Jul 15, 2016

Agenda

In this meeting we took a look at what the scope rules should be for variables introduced by patterns and out vars.

Scope of locals introduced in expressions

So far in C#, local variables have only been:

  1. Introduced by specific kinds of statements, and
  2. Scoped by specific kinds of statements

for, foreach and using statements are all able to introduce locals, but at the same time also constitute their scope. Declaration statements can introduce local variables into their immediate surroundings, but those surroundings are prevented by grammar from being anything other than a block {...}. So for most statement forms, questions of scope are irrelevant.

Well, not anymore! Expressions can now contain patterns and out arguments that introduce fresh variables. For any statement form that can contain expressions, we therefore need to decide how it relates to the scope of such variables.

Current design

Our default approach has been fairly restrictive:

This approach caters to the "positive" scenarios of is expressions with patterns and invocations of Try... style methods with out parameters:

if (o is bool b) ...b...; // b is in scope
else if (o is byte b) ...b...; // fine because bool b is out of scope
...; // no b's in scope here

It doesn't handle unconditional uses of out vars, though:

GetCoordinates(out var x, out var y);
...; // x and y not in scope :-(

It also fits poorly with the "negative" scenarios embodied by what is sometimes called the "bouncer pattern", where a method body starts out with a bunch of tests (of parameters etc.) and jumps out if the tests fail. At the end of the test you can write code at the highest level of indentation that can assume that all the tests succeeded:

void M(object o)
{
  if (o == null) throw new ArgumentNullException(nameof(o));
  ...; // o is known not to be null
}

However, the strict scope rules above make it intractable to extend the bouncer pattern to use patterns and out vars:

void M(object o)
{
  if (!(o is int i)) throw new ArgumentException("Not an int", nameof(o));
  ...; // we know o is int, but i is out of scope :-(
}

Guard statements

In Swift, this scenario was found so important that it earned its own language feature, guard, that acts like an inverted if, except that a) variables introduced in the conditions are in scope after the guard statement, and b) there must be an else branch that leaves the enclosing scope. In C# it might look something like this:

void M(object o)
{
  guard (o is int i) else throw new ArgumentException("Not an int", nameof(o)); // else must leave scope
  ...i...; // i is in scope because the guard statement is specially leaky
}

A new statement form seems like a heavy price to pay. And a guard statement wouldn't deal with non-error bouncers that correct the problem instead of bailing out:

void M(object o)
{
  if (!(o is int i)) i = 0;
  ...; // would be nice if i was in scope and definitely assigned here
}

(In the bouncer analogy I guess this is equivalent to the bouncer lending the non-conforming customer a tie instead of throwing them out for not wearing one).

Looser scope rules

It would seem better to address the scenarios and avoid a new statement form by adopting more relaxed scoping rules for these variables.

How relaxed, though?

Option 1: Expression variables are only scoped by blocks

This is as lenient as it gets. It would create some odd situations, though:

for (int i = foo(out int j);;); 
// j in scope but not i?

It seems that these new variables should at least be scoped by the same statements as old ones:

Option 2: Expression variables are scoped by blocks, for, foreach and using statements, just like other locals:

This seems more sane. However, it still leads to possibly confusing and rarely useful situations where a variable "bleeds" out many levels:

if (...)
  if (...)
    if (o is int i) ...i...
...; // i is in scope but almost certainly not definitely assigned 

It is unlikely that the inner if intended i to bleed out so aggressively, since it would almost certainly not be useful at the outer level, and would just pollute the scope.

One could say that this can easily be avoided by the guidance of using curly brace blocks in all branches and bodies, but it is unfortunate if that changes from being a style guideline to having semantic meaning.

Option 3: Expression variables are scoped by blocks, for, foreach and using statements, as well as all embedded statements:

What is meant by an embedded statement here, is one that is used as a nested statement in another statement - except inside a block. Thus the branches of an if statement, the bodies of while, foreach, etc. would all be considered embedded.

The consequence is that variables would always escape the condition of an if, but never its branches. It's as if you put curlies in all the places you were "supposed to".

Conclusion

While a little subtle, we will adopt option 3. It strikes a good balance:

It does mean that you will get more variables in scope than the current restrictive regime. This does not seem dangerous, because definite assignment analysis will prevent uninitialized use. However, it prevents the variable names from being reused, and leads to more names showing in completion lists. This seems like a reasonable tradeoff.

HaloFour commented 8 years ago

Yay, design notes!

Boo, changed scoping rules! I'm beating a dead horse, but I still think that it introduces inconsistencies into the language that aren't worth it. Why should an out declaration in a while leak but not an out declaration in a for? Even if the spec is written to be explicit and technically correct I guarantee that it will still feel unintuitive and unexpected.

If we kept with the previous scoping rules for out declaration variables there wouldn't need to be any new syntax introduced. We already have syntax to explicitly allow that variable to be defined in the enclosing scope:

int value;
if (int.TryParse(s, out value)) {
}

Now I do recognize that variable/type-switch patterns are a different case since they are implicitly readonly and must be declared where they are assigned. I don't have a good answer to this, short of saying just treat the pattern variable like any run-of-the-mill out variable (or use the out keyword to assign that value to said variable). But I think that something better can be proposed than deciding that half of the loop statements do one thing and the other half do something completely different.

And, to reiterate, yay design notes! 🎉 🍻

AdamSpeight2008 commented 8 years ago

The default should be that it is available in the surround scope. unless explicitly changed by the user. eg default

int value;
if (int.TryParse(s, out value)) {
 // value is in scope
} else
{
 // value is in scope
}
// value is in scope

local enclosing scope

if (int.TryParse(s, local out value)) {
 // value is in scope
} else
{
 // value is not in scope
}
// value is not in scope
int value;
if (int.TryParse(s, local out value)) { // error value already exists.
 // value is in scope
} else
{
 // value is not in scope
}
// value is not in scope

using @MadsTorgersen example

for (int i = foo(local out int j);;); 
// j in scope
// i in scope
}

guard

if (int.TryParse(s, guard out value)) {
 // value isnot in scope
} else
{
 // value is in scope
}
// value is not in scope
HaloFour commented 8 years ago

@AdamSpeight2008

I don't see how that is an improvement. It's still internally inconsistent, but now adds the complication of all of these new keywords. Can you apply local or guard to variables declared within a for loop? Does that even make sense? I don't think so.

AdamSpeight2008 commented 8 years ago

@HaloFour They are contextual only applying to out var, saying which scope to introduce the variable into. The default makes sense as it simplifies the existing pattern.

if( int.tryparse( text, out value ) 

If you're going change the scope the make it explicit rather than implicit.

MadsTorgersen commented 8 years ago

@AdamSpeight2008, @HaloFour:

A principle that the new design manages to maintain is that when it comes to scopes, all local variables are the same. It doesn't matter how they are introduced, only where they are introduced. This is very valuable in order for folks to build intuition: You can visualize the scopes as physical boundaries in the code.

It then merely becomes a question of "what establishes a scope". And yes, the weakest part of the chosen approach probably is the intuition around the fact that an if or while statement does not establish a scope for variables in its condition. Believe me, we had looong arguments around it! :-)

However, at the end of the day, the current proposal wins on having full expressiveness for the important scenarios, while having only slightly surprising scopes.

HaloFour commented 8 years ago

@MadsTorgersen

I don't know, I imagine more complaints here about people not being able to reuse their variable names. Especially if they need to convert from a switch statement to a series of if/else statements and all of a sudden, again, the rules change. Which is even weirder given that case labels don't introduce scope.

Believe me, we had looong arguments around it! :-)

Not the first time: https://roslyn.codeplex.com/discussions/565640

That's probably the most jarring aspect. This has been argued out before, long since settled, and all of a sudden we get this big 180. And as this decision came out of a particularly quiet time from the team it feels even more out of the blue.

DerpMcDerp commented 8 years ago

I had a prior proposal (#10083) suggesting that semi-colons should be able to appear in the boolean-condition part of an if statement which gives the programmer explicit over which variables get to leak outside the condition expression. The proposal didn't support the "bouncer pattern" (because I think it's a bad idea for a mainstream programming language to leak scope like that. e.g. Pre-Standard C++'s for (int i = 0; used to leak i's scope to outside of the for loop and most people hated it) but it could be tweaked slightly to support it. i.e. the proposal could be modified to have the following semantics instead:

if (foo(out int one)) {
} else {
}

// insert a dummy "if (false) {}" to suppress scope leakage outside the if chain
if (false) {
} else if (foo(out int two)) {
} else {
    two; // two's scope ends here
}

// insert a dummy "if (true;" to suppress scope leakage to other branches
if (true; foo(out int three)) {
    three; // three's scope ends here
} else {
}

one; // one's scope ends here

I'll mention it here as a way to enhance "Option 3" with granular control of how scopes are leaked.

bradphelan commented 8 years ago

The canonical examples always seem to be a bool return and an out. This is much better handled by an Option type and a pattern match

if(foo() is Some x){
   Console.WriteLine(x);
}else{
   Console.WriteLine("Got nothing");
}

though I'm not sure if the current pattern matching proposal for C# can handle the above type of patterns.

The guard pattern as described above.

void M(object o)
{
  guard (o is int i) else throw new ArgumentException("Not an int", nameof(o)); // else must leave scope
  ...i...; // i is in scope because the guard statement is specially leaky
}

again works better as an extension method on Nullable

T IfNull(this T? o, Action a){
    if(o == null) a();
    return o.Value;
}

used liked

void M(object o)
{
    int i = (o as int ).IfNull(()=>throw new ArgumentException("Not an int", nameof(o));
}

and a language extension like ruby blocks would get rid of the lambda

T IfNull(this T? o, Block &a){
    if(o == null) a();
    return o.Value;
}

void M(object o)
{
    int i = (o as int ).IfNull {
        throw new ArgumentException("Not an int", nameof(o))
    }
}

No need for weird scoping rules.

MadsTorgersen commented 8 years ago

@HaloFour: Thanks for digging out the design notes where the previous design came from. We went back and looked at those arguments and found that we no longer believed in them. I acknowledge that this came "out of the blue" in the sense that it's a late design change; in fact (knock on wood) the last one for C# 7.0.

The fact is that we've been mulling it for a couple of months, but only had the extreme options on the table (the restrictive design vs. what is "Option 1" above). Both were really unacceptable. Only a couple of weeks ago did we come up with the compromise that is option 3. I think it retains the full upside of option 1 (supporting meaningful use of out vars in top level expression statements, supporting the bouncer pattern for patterns and out vars). At the same time it limits "variable spilling" to a reasonably intuitive set that are declared "near the top level".

Sometimes you want those variables in scope, sometimes you don't.

While this isn't a slam-dunk trade-off, in the end we think a and b are lesser evils than c and d.

@DerpMcDerp: It's an explicit goal not to allow granular control of the scope of a given variable. A given construct should be a scope, and all variables contained inside it are scoped by it, regardless of how they are declared.

(This principle is violated by dubious legacy behavior inside switch statements that unfortunately cannot be fixed in a non-breaking fashion. We'll consider this a wart and live with it).

@bradphelan: If we could start over we might have avoided the TryFoo(out ...) pattern, or maybe even out parameters altogether. But the state of the world is that there are lots of out parameters out there, and as long as we're introducing variables in expressions anyway (with patterns) it seems good and right to use that to also improve the consumption experience of those out parameters. For Try methods, you can now think of them almost as a way to write your own active pattern.

bradphelan commented 8 years ago

@madstorgesson yeah that's a nice way to think of out params as being active pattern. Thanks for the insight.

On Fri, 5 Aug 2016, 18:20 Mads Torgersen, notifications@github.com wrote:

@HaloFour https://github.com/HaloFour: Thanks for digging out the design notes where the previous design came from. We went back and looked at those arguments and found that we no longer believed in them. I acknowledge that this came "out of the blue" in the sense that it's a late design change; in fact (knock on wood) the last one for C# 7.0.

The fact is that we've been mulling it for a couple of months, but only had the extreme options on the table (the restrictive design vs. what is "Option 1" above). Both were really unacceptable. Only a couple of weeks ago did we come up with the compromise that is option 3. I think it retains the full upside of option 1 (supporting meaningful use of out vars in top level expression statements, supporting the bouncer pattern for patterns and out vars). At the same time it limits "variable spilling" to a reasonably intuitive set that are declared "near the top level".

Sometimes you want those variables in scope, sometimes you don't.

  • If you don't and they are there anyway, what's the harm? a) some variable names are taken and not available, and b) it may be a bit confusing until you get the hang of it
  • If you do and they aren't there? c) Significant and clunky rewriting of your code or even d) you have no way of using the new features.

While this isn't a slam-dunk trade-off, in the end we think a and b are lesser evils than c and d.

@DerpMcDerp https://github.com/DerpMcDerp: It's an explicit goal not to allow granular control of the scope of a given variable. A given construct should be a scope, and all variables contained inside it are scoped by it, regardless of how they are declared.

(This principle is violated by dubious legacy behavior inside switch statements that unfortunately cannot be fixed in a non-breaking fashion. We'll consider this a wart and live with it).

@bradphelan https://github.com/bradphelan: If we could start over we might have avoided the TryFoo(out ...) pattern, or maybe even out parameters altogether. But the state of the world is that there are lots of out parameters out there, and as long as we're introducing variables in expressions anyway (with patterns) it seems good and right to use that to also improve the consumption experience of those out parameters. For Try methods, you can now think of them almost as a way to write your own active pattern.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dotnet/roslyn/issues/12939#issuecomment-237895131, or mute the thread https://github.com/notifications/unsubscribe-auth/AABE8gP2mrMuByixMNg8ypD61ZypD_cKks5qc2LMgaJpZM4JdLzM .

mungojam commented 8 years ago

I'm confused how option 3 supports the bouncer pattern. In your example. isn't i still out of scope because of the implicit braces around the if branch?

void M(object o)
{
  if (!(o is int i)) throw new ArgumentException("Not an int", nameof(o));
  ...; // we know o is int, but i is still out of scope with option 3 isn't it?
}

seems to be equivalent to:

void M(object o)
{
  if (!(o is int i)) 
  { 
      throw new ArgumentException("Not an int", nameof(o)); 
  }
  ...; // i is still out of scope due to block?
}
Eyas commented 8 years ago

@mungojam Implicit braces around a branch affect patterns, declarations, etc. inside of the branch (between the implicit braces), the declaration inside of the condition part of the if statement continues to 'bleed'.

Since C# provides us with definite assignment analysis, I agree this sounds like a clean approach.

mungojam commented 8 years ago

Thanks @Eyas, yes, I follow now, the variables are escaping to outside the if block, not just into each branch, and definite assignment analysis is doing its magic.

Sounds like a very reasonable compromise to me, in current code the variable had to be declared outside the if block anyway, e.g. when using Try...(out result)

Shiney commented 8 years ago

This might be enough of an edge case for the bouncer pattern that it probably doesn't matter, but what happens if boolean operators that cause short circuiting are used in the conditional in the if block?

E.g.

if(dictionary != null  && dictionary.TryGetValue("key", out int j)
{  
    //Stuff
}
// Is j in scope here? What happens if the dictionary was null?
stepanbenes commented 8 years ago

@Shiney I understand that the variable j will be in scope after if, but it won't be definitely assigned.

HaloFour commented 8 years ago

@Shiney

It's no different than the following that you can write today:

int j;
if(dictionary != null  && dictionary.TryGetValue("key", out j)
{  
    // j is in scope and is definitely assigned
}
// j is in scope but is not definitely assigned
HaloFour commented 8 years ago

@MadsTorgersen

What about the scope of out declaration variables within LINQ queries? Allowing them to leak throughout the LINQ query would be very useful, like an automatic range variable. But if they leak to the enclosing scope they could potentially produce concurrency issues if the LINQ query is parallel.

I imagine that people will expect the following to "Just Work"™:

IEnumerable<string> strings = GetSomeStrings();
var query = from s in strings
    where int.TryParse(s, out int i)
    select i;

// is i in scope here?
HaloFour commented 8 years ago

@MadsTorgersen

Actually, thinking about it some more, specifically with pattern variables, the scope changes make little sense. As long as pattern variables remain implicitly readonly, the scope is leaking into scopes where the identifier could never be definitely assigned. As such, it's just a wasted identifier.

Eyas commented 8 years ago

@HaloFour doesn't seem wasted in general-- as you said in the different thread:

It seems much of the point of these scoping rules is to avoid the tons of extra syntax that Swift requires in order to influence the scope.

That seems worth having a wasted identifier in some cases, while allowing the identifier to be useful in the opposite case:

if (!x is Type t) throw new IllegalArugmentException(nameof(x));
// can still use t

Are you saying it is worth having special statements or special scoping rules just to get this set of use cases to work?

HaloFour commented 8 years ago

@Eyas

For the bouncer case it's probably fine. t would be definitely assigned at least where you'd expect to use it. But in any other situation you'd be left with t where 2 out of the 3 scopes where it exists it cannot be used nor can it be updated. At least out variables can be updated so you can assign some fallback value.

MadsTorgersen commented 8 years ago

@HaloFour and others,

I should clarify a few things that weren't in the notes above.

_Pattern variables are mutable_. For instance you can write:

if (o is int i || (o is string s && int.TryParse(s, out i)))
{
    // i is definitely assigned and o is either an int or a string with an int in it
}

There are a couple of things that also already establish scopes, that I forgot to mention, because they aren't statements:

_lambda expressions already establish scopes_ to carry their parameters. These scopes also contain any variables defined inside the body of even expression bodied lambdas.

_Query clauses already establish scopes_ because query expressions are defined in terms of syntactic translation to lambda expressions which constitute scopes.

It is true that it would be nice to allow them the same scope as that of variables introduced with from and let clauses (which turn into separate lambda parameters for each clause in the translation). We don't know how to do that in practice, though.

_Catch clauses already establish scopes_ for the exception specifiers, and any filter expression on the catch clause is contained within that scope.

Hope that helps!

Mads

svick commented 8 years ago

@HaloFour What would that query compile into? A combination of Select() and Where(), like this?

var query = strings
    .Select(s => { int i; bool res = int.TryParse(s, out i); return new { s, i, res }; })
    .Where(x => x.res)
    .Select(x => x.i);

I'm not sure this kind of translation would be expected. How the translation looks like is especially important for IQueryable and custom LINQ providers.

HaloFour commented 8 years ago

@svick

In short, yes. The compiler could emit tuples to make it more efficient, which was/is on the board for LINQ expressions which project temporary anonymous types for range variables today like let. The translation above should work fine with expression trees, although I agree that most queryable providers probably wouldn't understand it, but how many of them would understand methods with out parameters today regardless of what you did with the scope?

Basically an extension of #6877 where the implementation concerns were already mentioned. But with let seeming less and less necessary there likely wouldn't be another opportunity to add said functionality to LINQ, which means that pattern matching wouldn't have a place in stream processing, which in my opinion would be a pretty big fail.

paulomorgado commented 8 years ago

I really think that it's a bad idea that a variable introduced in the condition of a if or while leak that statement.

In Try... methods, if the user wants the variable to leak, then just write the code the way it can be written today.

Patterns are another issue easily solved by using another variable declared outside the statement:

int j;
if (o is int i || (o is string s && int.TryParse(s, out i)))
{
    j = I;
}
else
{
    j = -1;
}
// I should not be in scope here!!!
tumtumtum commented 8 years ago

@paulomorgado The less variable declarations the more declarative the code is.....which leads to fewer bugs.

paulomorgado commented 8 years ago

@tumtumtum, I'm sure you have hard data on that. Specially on cases like this one.

timgoodman commented 8 years ago

It's as if you put curlies in all the places you were "supposed to".

This is a good rule. I certainly would expect that the use of curlies in single-line if branches would be purely a stylistic choice.

Although I guess it's not quite true even today, since if (condition) { var x = 5; } is legal (albeit pointless) but if (condition) var x = 5; gives error CS1023. Which is probably a good thing, since the programmer presumably meant something like int x; if(condition) x = 5; With this change, will that still be an error?

In other words, do the invisible curlies apply only to expression variables, but not to ordinary variable declarations in the same location? (If so, I'm not objecting to that small amount of inconsistency -- just curious.)

qrli commented 8 years ago

@paulomorgado Falling back to old syntax always works. But then the usefulness of our var would be much less, to an extent which I think not worth it. Leaking a few variable names, in practice, does not increase risk of bug. And it does not make it harder to name variables, as long as the function is not too long. The only bad thing is it does not feel good. I think it is a good compromise.

alrz commented 8 years ago

I believe the best way to implement method contracts (#119) is to use some kind of guard statement instead of drowning method signature with oneliner checks (see https://github.com/dotnet/roslyn/issues/11308#issuecomment-219213073). So I'd go with guard statement here to do double duty in the future. As others said, it seems these scoping changes help in one case and get in the way in the rest.

HaloFour commented 8 years ago

@Kaelum

You can specify the type name as well, e.g. bool success = int.TryParse(text, out int result);

Sounds like style rules will allow you to enforce that var can't be used in any situation where the variable could be explicitly defined. Some people like the terseness of var. C++ has auto. Java is likely getting var, val and/or let. The trend is more implicit strong typing, and I mostly welcome it.

lorcanmooney commented 8 years ago

Preview 5 has only been out a few days, and I've been burned by the new out var scoping rules already. It went something like this:

var text = "5.5";

if (int.TryParse(text, out var resultI4))
{
    Console.WriteLine(resultI4);
}
else if (double.TryParse(text, out var resultR8))
{
    Console.WriteLine(resultI4);    // Typo!
}

This is not the pit of success I was hoping for.

mungojam commented 8 years ago

@lorcanmooney I would say that isn't so different from many other scenarios where that could happen given a typo, one being the way you would currently have to write it in C# where you'd have to declare both types before the if statement.

For me the trade-offs are worth it, and in your example that would hopefully raise a compiler warning for the unused resultR8 variable, although maybe it doesn't do that for out variables? Resharper hopefully would if you have that.

lorcanmooney commented 8 years ago

I would say that isn't so different from many other scenarios where that could happen given a typo...

You are of course correct, but my hope was that we could use out var to reduce such problems.

tamlin-mike commented 7 years ago

@Kaelum

At first I was thinking like you - this is not good design. Scope is scope. Obey it.

Then I saw the 'out' var example, and took a step back. F.ex. how else could you both define a new variable and get it to live after a function call when ';' is encountered?

Yeah, it's a convenience feature, but if we consider it like this:

// explicit
int i;
blahblah(..., out i);

and

// implicit
blahblah(..., out int i);

I think it makes sense. You'd have to wrap both examples in {} to keep scope.

I'm not saying that I've yet come to terms with the thinking, but expressed like this it started to seem reasonable to me.

tamlin-mike commented 7 years ago

@Kaelum: Having spent some time letting this issue sink in, I think we're on the same page. Your final statement stands out, to the point I repeat it, in hopes someone in the group defining the language takes this to heart:

"As it stands, the rules are so convoluted, and polluted, its going to be impossible to know what variables are in scope."

Whether or not that is correct (though currently it seems not incorrect), just the fact it is claimed should be enough of a wake-up call.

I consider this a major, disruptive, language change, and as such it should not be allowed to go "live" unless all outstanding issues are addressed - and solved.

CyrusNajmabadi commented 7 years ago

@Kaelum

Revert the rules to the previously proposed set and allow the developer to decide if they want the functionality that is being proposed here, by simply doing the following:

There is a downside to that. Now, as a developer i have to declare my variable before hand. This is a negative for me on two counts:

  1. i now have to have both the declaration and the initialization separate. I would far prefer to not do that.
  2. i have to do declare the type out fully. As such, i might end up with:
Dictionary<string, ImmutableArray<(ISymbol symbol, double matchWeight)>> symbolWeightMap;
if (!somethingElse.TryGetSymbolWeights(out symbolWeightMap)) {
     return;
}

symbolWeightMap.yadda yadda

I would far prefer just writing:

if (!somethingElse.TryGetSymbolWeights(out var symbolWeightMap)) {
     return;
}

symbolWeightMap.yadda yadda

Indeed, as a primary proponent of the new scoping rules, i wanted very much to have a system whereby i could take nearly all cases today where 'out' is used, and be able to switch them over to 'out var'.

The limited-scoping rules ended up being precisely that: very limiting.

As such, pulling the variable into the enclosing scope was both necessary for some cases (like the one i showed above), and (IMO) not that impactful for others.

To be clear, i recognize, and i'm cognizant of the desire to not have those variables be in scope. The examples given by many here were echoed internally and absolutely have merit. The question was between:

  1. Have limited scoping. Good for some constructs, but eliminates others. Or
  2. Have wider scoping. Enables all constructs, but means slightly more vigilence with naming and using variables.

In the end '2' won out. It was unpleasant to have a new feature that existing code couldn't move to naturally. Bog standard simple patterns (like guard clauses) were not possible, and would require an entirely new language construct to solve them. It seemed unfortunate that we'd add a feature like out-var, and would immediately need to introduce another feature to make up for such limitations.

CyrusNajmabadi commented 7 years ago

And, again, the complaints about this current scoping decision are absolutely true and recognized. variables are put into a broader scope than some would like. The difficulty is that there are two groups. One that wants the variables in the wider scope, and one that wants the variables in the narrower scope.

This is a case though where one approach would make the feature non-usable for some users (because the code would literally not compile), whereas the other makes the feature less pleasant (but the feature is still available and works).

IMO, i'd rather us go with an approach that allows both groups to at least have a solution that uses this feature, as opposed to picking the narrower approach that is more ideal for one group, but which makes the feature literally not available for the other.

I really wish there was a solution that would allow this single language feature to be great for both groups. One that allowed brevity and simple usage of var, while also making variables scoped mor widely for those who want that (like me), and one that allowed out-var to be used, but also be scoped very narrowly for those who like that. But with the desire to only have a single feature, and to keep it simple (i.e. no new forms like wide out var whatever), the decision was made to have somehting usable by both camps, even if not as ideal as possible for the 'narrow' camp.

CyrusNajmabadi commented 7 years ago

Preview 5 has only been out a few days, and I've been burned by the new out var scoping rules already. It went something like this:

Yes. This is the unfortunate situation where there is no perfect solution. On the other hand, i've been able to update a bunch of code to use out-var in cases where i want wide-scoping. So the new feature has been a boon to me. Six of one, half dozen of another, and all that :-/

CyrusNajmabadi commented 7 years ago

To those who do not want the variables in scope. Would you ok with an intellisense option (or code-style option) that flagged these wide-scope cases as problems in your code? You could then catch these cases where you unintentionally referred to a variable you would have preferred not be in scope.

I ask because i'm largely responsible for a lot of the intellisense/code-style type features in VS. As such, i'd be happy to put such a feature on our backlog for inclusion in the future. It's also something we'd likely welcome as a PR if any of you want to pick it up :)

alrz commented 7 years ago

@CyrusNajmabadi Alternatively we could allow variable shadowing with an option to flag them as warning. It's proposed before and has its own use cases specially with recursive patterns. To not let it get out of hand I think it'd help if we restrict which and where variables can be shadowed.

HaloFour commented 7 years ago

@CyrusNajmabadi

The scoping by itself is bad enough but what makes it absolutely horrible is the fact that the behavior is inconsistent. You get wide with if but you get narrow with switch. You get wide with while but you get narrow with for. It creates a slew of new rules that developers have to (and won't) remember in order to do their job and it will make refactoring a pain. Imagine just tweaking one loop and half of the rest of that function breaks?

What's worse is that the desire to make out variables slightly easier to use now pollutes the entirety of pattern matching. Now it's impossible to use it in any meaningful way without spraying a ton of extra identifiers all over the place.

I'd rather have guard and let. Sure, that's a lot of new language constructs but they enabled the developer to explicitly declare their intent and achieve their goals without having to adopt old-JavaScript-style variable scoping.

alrz commented 7 years ago

Just to mention, it is possible to use let with TryX to widen var scope to the enclosing block; assuming previous scoping rules:

// match return value against a constant pattern 
let true = TryX(out var result) else return;
// `result` in scope

Not as pleasant as guard but the same functionality.

DavidArno commented 7 years ago

@CyrusNajmabadi

Would you ok with an intellisense option (or code-style option) that flagged these wide-scope cases as problems in your code?

No, that would not be OK, as the scope leakage problem would still be out there in the wild, and the genie can never be put back in the bottle. You say:

This is a case though where one approach would make the feature non-usable for some users (because the code would literally not compile), whereas the other makes the feature less pleasant (but the feature is still available and works).

In reality, the one approach would see a version of the C# compiler supporting scope leakage. People would use this and from then on, we'd all be stuck with it, due to the team's strict rule of avoiding breaking changes. No matter how unpopular this proves to be with the wider user-base, C# would, forever more, leak variable scope. The other approach would be to temporarily make out var and is <type> slightly less pleasant to use for some users whilst a proper solution is created for C# 7.1+.

Letting C# 7 ship with this problem would be a truly irresponsible act by the compiler team.

DavidArno commented 7 years ago

@CyrusNajmabadi,

I think the most frustrating thing about this scope leakage plan is that the use-cases for doing it are such weak ones. We have:

GetCoordinates(out var x, out var y);
// I want to access x & y here

which is fixed by using a little feature also added to C# 7, tuples:

var (x, y) = GetCoordinates();
// I can access x & y here, without scope leakage

And we have:

if (!(o is int i)) throw new ArgumentException("Not an int", nameof(o));
  ...; // we know o is int, but i is out of scope :-(

which is fixed by using a ternary operator, and the new throw expression:

var i = o is int j ? j : throw new ArgumentException("Not an int", nameof(o));
// i is in accessible here, again without scope leakage
// and, importantly, j is not in scope, as it's job is now done. Win, win situation!!!

Which leaves me wondering how the decision was ever made to make such a massive compromise to scoping for such poorly thought out reasons?

alrz commented 7 years ago

@DavidArno There is another one, if (!TryX(out var result)) return; which guard was proposed for.

DavidArno commented 7 years ago

@alrz,

Your own #14239 proposal could be a solution to that, by allowing return to be an expression:

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

but that aside, really is enabling the saving of one line, by turning:

SomeType result;
if (!TyX(out result)) return;

into "your" version, really worth introducing a fundamental change to variable scope?

alrz commented 7 years ago

@DavidArno I'm not saying "yes" to your question but I think the whole point of out var feature is to save that very line. Roslyn codebase uses this pattern a lot so I'd expect this to be an important use case to deal with. I'd agree that this change is not the perfect solution though.

iam3yal commented 7 years ago

@CyrusNajmabadi

I'll be honest with you, I prefer this:

Dictionary<string, ImmutableArray<(ISymbol symbol, double matchWeight)>> symbolWeightMap;
if (!somethingElse.TryGetSymbolWeights(out symbolWeightMap)) {
     return;
}

symbolWeightMap.yadda yadda

Over this:

if (!somethingElse.TryGetSymbolWeights(out var symbolWeightMap)) {
     return;
}

symbolWeightMap.yadda yadda

Simply because the first has consistent scoping rules and the second doesn't, I don't like to trade readability for consistency and what I already know, most of the time.

If you REALLY want to solve the readability problem solve it at its root, allow var to be inferred based on the argument and then this would be completely possible.

var symbolWeightMap;
if (!somethingElse.TryGetSymbolWeights(out symbolWeightMap)) {
     return;
}

symbolWeightMap.yadda yadda

And then allow people to do the following if they want a narrowed scoping rules:

if (!somethingElse.TryGetSymbolWeights(out var symbolWeightMap)) {
     // Do something with symbolWeightMap
     return;
}

In short do it right.

DavidArno commented 7 years ago

@alrz,

It would be really sad if the team were fundamentally damaging variable scoping rules just to save themselves a tiny bit of typing. :cry:

Eyas commented 7 years ago

I've been kind of disappointed by the kind of arguments I'm hearing against this the scope changes which I think are fundamentally a good idea:

First, there is no formal contract between the return value of a function and its out parameters. out parameters are used for two patterns which are both, in a sense, obsolete:

  1. TryXXX methods for conditional returns. Can use nullable return values instead.
  2. Multiple return values. Can use tuple return values instead.

In either case, libraries that write methods using 1. and 2. will continue to exist. Therefore I'm not particularly convinced by @DavidArno's argument in this comment that we don't need to worry about the multiple return cases (the GetCoordinates example).

Let me repeat. There is no formal contract between the return value of a function and its out parameters. In fact, C# already forces us to define the value of out parameters.

Second, for pattern variables, there is absolutely no problem or tradeoff. Scope is not the only thing that governs how you write your code. The C# compiler will fail with a compile error if it detects a use of a variable that is not definitely assigned.

Thus, the scoping rules are only potentially problematic for out variables. My view:

Finally, with regards to this:

It would be really sad if the team were fundamentally damaging variable scope just to save themselves a tiny bit of typing.

I question that variable scope is being "damaged". Scope just tells you which variable names are available to you for use/reuse. In C#, scope is not damaged as easily due to definite assignment checks. Further, most advancements in programming languages (ever?) could be said to stem from "sav[ing] themselves a tiny bit of typing".