-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
while variable closure #15529
Comments
In my view, you are mixing three things into one here:
|
@DavidArno Good points. I hadn't actually considered the In the The And I am of the mind that variables should not be able to leave the condition at all in If we made a pros and cons table for this scoping, I think there would be a heavy weighting to one side! |
Good point re |
/cc @dotnet/ldm |
Definitely something we could consider. I wouldn't have a problem with all the 'loop' constructs having a similar concept of a scope where their declared-variables would go. @gafter @MadsTorgersen I think it would be fine to change 'while' to behave in this manner. It would keep the loops consistent, while not impacting any of the scoping scenarios we cared about for the if/switch patterns. Note: i do think this code pattern would be rare. With foreach's and lambdas you were mixing two massively common constructs. To run into the problem with 'while', you'd need to have a 'while' with an 'out-param', and you'd need to be using a lambda which captured that out-param. That case seems so niche that i'm also ok not doing anything special here. Certainly, if we did nothing now, i would likely feel this would never warrant any work in the future. However, since we are touching scopes now, perhaps it's reasonable to just do this work. With that, i think our scope rules would be:
-- This seems reasonable to me. The core cases of To me, the important things with 'if' and 'expression-statements' was that we could see the valuable use cases in both narrow and wide scoping. I'm having trouble thinking of any valuable use cases for 'wide' 'while-loop' scoping. I'll do an examination of Roslyn itself to see if we've ever used this pattern. Note for Mads/Neal, if you didn't see #14697 (comment) Roughly 60% of out-var usage in Roslyn uses 'wide semantics'. i.e. we take advantage of spilling the variable into the outer scope. My gut tells me the number will be close to 0% for whiles. As such, if there are really no great use cases for splling while-variables, we should likely unify with the other loops. Thoughts? |
@Andrew-Hanlon Thanks for the feedback! |
Definitely second your thanks to @Andrew-Hanlon. After the storm of recent days over this cope change, I think Andrew may have turned up with a simple suggestion during the calm that could be a genuine compromise. If (though I reserve the right to remain miffed about |
Thanks for looking this over @CyrusNajmabadi and @gafter and all. While I agree that this scenario will be less common than the C# 5's I can easily see this being common enough and problematic:
P.S. In a related pain point about this variable closure, if I had switched the order of the |
Data time! As mentioned already, Roslyn has a 60/40 split between usages of statements that spill out-vars widely versus those that spill them narrowly. i.e. we use the: if (!TryWhatever(..., out var v))
{
...
v =
...
}
// use v pattern a lot. So now to while loops. In roslyn only 2% of all while loops even use an out or ref variable. That's higher that i expected! However, examinign all the usages shows that they're nearly all of the form: Task curTask;
while (tasks.TryPop(out curTask))
{
curTask.GetAwaiter().GetResult();
} I could only find one piece of code (and it was complex at that) where out was being used to write into an outer scope. And in that case it was writing into another variable produced by another out-var. A very uncommon code pattern indeed. From my sample size of one code base, i'm of the opinion that:
-- With the numbers collected, i think we're doing the right thing with if/switch/expression-statements. However, i think we have a better choice when it comes to loops. As we've discovered, and Andrew rightly reminds us, loop scoping is particularly problematic when it comes to capturing. Best to avoid the whole problem as there seems to be little (no?) data indicating that loops should spill. |
Note: i find andrew's pattern examples more compelling. And in those we'd also likely want to have narrow scopes. So i think we should make these scopes narrow since we're doing the work in this area right now anyways. |
Can you please run the same analysis for the negated is operator? I think that is also a rare case and the problem would be non-existent when we have let statements. Since is produces an unassigned variable in the else part I think it wouldn't be wise to spread the scope to the whole block. Thanks! |
I don't have the numbers handy anymore (should have really saved the excel spreadsheet :( ). But my findings were that it was common enough. While you are correct that some codepath would be unassigned, we have code that then wants to assign in the errant branch. i.e. it's common enough for us to do: if (x is string s) {
}
else {
s = something_else
}
// use s I think it really comes down to a few different camps/styles. I have a functional background. So i'm more used to immutable values and narrow scopes. But C# devs come in all stripes, and as we've seen from our own code, there are many how use a more mutable/imperative style. The former group is better served by narrow scoping. The latter group by wider scoping. However, to repeat myself (from teh other thread), our choice was between:
At the end of the day we chose '2'. Wide scoping means you can use the feature if you're in either camp. It does make things slightly less pleasant though if you want to use the same name for different types in different scopes. We felt like we could live with that restriction though. It was better than having people who wanted wide-semantics for their current constructs not be able to use them. |
Wow. I was all about narrow scoping for I still think that |
+1 on having 'match'. I recently converted expression based code over to using the new pattern matching |
@CyrusNajmabadi I like your style, greping real code is always important! But we also need to think about pattern matching vars - which I believe will become very prevalent. Another thing to consider is that the previous example could as easily apply to
|
To be clear, i'm 100% agreeing with you. I was collecting the data to make sure there weren't any sort of common patterns we would be potentially hurting. That turned out to be the case with 'if' and expression-statements, which is why we changed the scoping semantics. But, fortunately, (IMO) this looks like a very reasonable change we can make, with practically zero downsides. |
@CyrusNajmabadi That's totally reasonable. But I don't think that would be applicable to the ternary operator. var x = obj is T t ? (use t) : whatever;
// why would I want t here? |
Meh. The ternary operator is a single small expression. It wouldn't be astonishing if it didn't declare a scope unlike the |
So, in the interest of not starting another OMGHUEG thread, i ask that we take the discussion of ternary operators back to #14697 . If there is a concrete proposal, then let's open a new issue on that. I'm not trying to squash your ideas AT ALL. I just think we have a great proposal here that is very nice self contained. I would prefer to not have the discussion around it balloon into more than just Thanks much! |
The do/while loop is an oddball here. Any variables introduced couldn't be read anywhere else, neither within the loop block (use before declaration) nor afterwards (not definitely assigned). I'd concur that if |
I'm not advocating one way or the other about what the scope should be, but variables assigned (and in C# 7.0, introduced) in the condition of a int i;
do
{
} while((i = 42) == 0);
Console.WriteLine(i); This is perfectly valid code and Also note that using a |
Ah, yeah, you're right. Still would be odd to try to use the variable within the block both due to definite assignment and the fact that the declaration follows the block. |
I think I like the idea of tight scoping on while loops. A while loop today:
compiles the same as if it was written (and occasionally it is useful to refactor to this style):
so scoping this tightly reinforces the idea that this refactoring is precisely true. This:
should be the same as:
(btw, I like the wide scoped However do-while loops are odd. If said narrow scoping rule were introduced for them, the introduced variables would not be usable outside of the loop condition. I haven't entirely made up my mind on whether the narrow scoping is better or not for those. Also:
Any variable declared in the block of a do-while loop is not usable in the condition. In order for
|
Mads will have notes at some point. But one of the conclusions of today's LDM was that we would be revising the scoping for
There are many more things we intend to discuss. But we had to cut things a little short because our offices are being painted, so they needed us out of the building :) Thanks for your continued help and support, and we hope you like this news! |
@CyrusNajmabadi so basically variables scope is widened for all cases except while loops? what about expressions that lives outside to control blocks? as mentioned here #15538? got anything to share on that? |
Actually this should bring I'm curious about what remains. I seriously hope the LDM pays a little lip service as to how |
Surely what remains is everything covered by #15538 (which you created! 😉), namely the scope for all |
The LDM met yesterday and decided to approve this proposed change to the scoping of expression variables in a while loop. Specifically, a while loop written while (<cond>) <body> will have expression-variables declared in continueLabel:;
{
if (!<cond>) goto breakLabel;
{
<body>
}
goto continueLabel;
}
breakLabel:; We will also make a corresponding change to the for (<decl>; <cond>; <incr>) <body> will have expression-variables declared in the three parts scoped as if it were rewritten {
<decl>
while(<cond>)
{
<body>
continueLabel:;
{ <incr> }
}
} Note that |
I vote for discussion remaining here and #15630 stays a purely technical issue for managing the actual change. |
Thank you kindly @CyrusNajmabadi and @gafter for the update and information regarding the LDM. |
@bbarry There is nothing left to discuss. yesterday never comes. |
So what's the LDM decision for |
Keeping this alive in case there's further conversation, but given the merges, I'm getting this off the "panic" radar. |
Design notes for this are in #16694. Thanks again for the suggestion! |
|
In C# 5,
foreach
was explicitly changed - a breaking change no less - to ensure that loop variables were not closed over. At the time Eric Lippert wrote:With C# 7's current rules for
while
loop scoping, we again have the same problem where condition and loop variables will be closed over by default,Wouldn't it make sense to consider this ahead of time and ensure that out and pattern match vars do not fall into this trap?
while(GetNext(out T value)){...}
should follow in the footsteps offoreach
and produce something like:In general I do not agree with the new scoping rules for
if
andwhile
, but I see the later being not just confusing or an inconvenience, but a flaw that will be a "gotcha" for many.@ericlippert 's final quote in the link above is also quite poignant:
The C# 7 language team has a real opportunity to meet all of these principles before they become an issue.
Please reconsider the scoping rules for out and pattern match variables.
The text was updated successfully, but these errors were encountered: