-
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
Definite assignment versus pattern-matching when pattern always matches #14651
Comments
I'm not convinced that's a good idea. {} denotes scope. Should really that 'if' in the example introduce 'j' to the surrounding scope? I would have expected it only to be valid within the scope of that statement (i.e. inside the "{}" in this case). That would to me be the path of least surprise. If starting to release what at first glance seems to be scope-limited variable introductions into outer scopes, ... that seems like a very slippery slope (read: not a can but a whole barrel of worms). To quote wiser persons than myself: Here be dragons! |
I agree with your sentiment, but the scoping issue was already discussed and effectively decided upon: #12939 This issue specifically addresses definite assignment of |
Gotcha. OK, so it's (currently, and hopefully for a looong time) limited to 'out' variables, and implicit 'out' variables like 'j' in the scenario presented? While syntactically a bit confusing at first blush, I now see the merit and I'm less concerned. Provided scope doesn't expand, explicitly or implicitly (even by mistake, f.ex. with an unnamed tuple), I'm (reluctantly, from a syntactical purist POV) cool with it. |
"I told you so" is unprofessional, but... The team ignored lots of sensible advice and (a) introduced non-conditional |
@DavidArno This isn't a "problem", it is a place where the behavior has to be specified. The same "problem" exists no matter what the scope, due to operators such as |
That expression wouldn't be valid anyway, you're trying to declare |
Whether or not the |
@HaloFour: Again, that is not true. In fact, that is an almost exact replica of some code that @MadsTorgersen wrote in the exact same thread: |
He's using |
@HaloFour: Ok, remove the int then. The statement still stands, how does the following make any sense?
|
I'd agree that it doesn't and that the new scoping rules are a mistake, but it seems that the language design team has settled that decision. |
You are right. It isn't a "problem"; it's a problem. The way I see it, @HaloFour and I have really quite strongly disagreed with each other over a whole raft of ideas here. Occasionally, we've semi-agreed. On this issue though, we are as one: the new scoping rules are a mistake. If such often diverse views are so totally in agreement, the language team really ought to stop and take notice. Sadly, you won't though. |
The "problem" cited is the definite assignment state in a syntactic position where "definitely assigned when true" does not apply. Scope is a separate issue. |
@gafter: That is an inaccurate statement. If the scope didn't leak, the "problem" would not exist. Into the rabbit hole we go... |
Previously I was thinking "Well, to prevent leaking, just wrap it in curly braces". Then comes @Kaelum with a voice of sanity and an example (many thanks for it) displaying "Whooops, that won't work!". I wonder (just brainstorming here): Could it become "cleaner" if instead allowing a statement to not end in ';', but have a following {} (where introduced variables would be "live", but not after closing brace)? Edit: But then, how the heck would a result from such a compound statement be expressed? Should a "return" statement be mandatory? sigh this is a mess. Perhaps Mads should be included in this discussion? |
@Kaelum So raising up a counter example will help not flip the decision again. And you example can be workaround by using |
@qrli I do not accept this change, and never will, as long as it allows scope to leak in this manner. I do have a question for you, how do you think that "switch" can be used to replace the code in the example that I showed? My example is a single line of code, of which there are several when used in an actual application, and is a variation of code that is currently in use in several applications. This latest scope change needs to be rolled back to what we previously had, and then we can move forward. This "Option 3" is by far the worst of all the options, and will continue to spiral down the rabbit hole. I'd rather force the discussion now, than have people realize a year later that we are now totally f*****. |
Not true. In code such as
@Kaelum Can you please provide a specific example of code that used to compile, and now either does not compile, or has different behavior? |
@gafter: I don't know how you can make that statement, as "j" would only be in scope for the
@gafter: Yes I can, and I am so glad that you asked this exact question, but first, we need to go back a bit to look at the bigger picture. If you must, the example is at the bottom of this post. However, this entire mess has snowballed from a couple shortcomings, and the current methods for working around them. What are the shortcomings? The major one is, the inability to use Fields or Properties as To work-around the major shortcoming, you are required to create a temporary variable, so that you can do something similar to a So, what happened? Someone put in a request for a "nice to have feature". The ability to declare a variable in the method call with an With the horrendous changes that have been proposed as "Option 3", bringing us to the current proposed set of rules, we now have "hidden" variable declarations in places that can be extremely difficult to find, and there is no valid reason allowing this to happen. Remember, this is for "nice to have feature"! This is not required in any way. So why is there a proposal to forever screw up the scoping rules of an AWESOME language, simply to implement a "nice to have feature"? That is the $64K question! How did a simple "nice to have feature" turn into the "Scoping Rule Proposal from HELL"? All this being said, the example below exhibits how implementing this "nice to have feature" in existing code, will cause scoping issues with existing code:
You would not expect "i" or "t" (not sure what would happen with this) to have already been declared. You also have the following to consider:
The above code would become a reality. Due to poluted scoping, what do you think it would look like if we attempted the same thing using a DBNull test on a DataReader for a database record that has 30+ columns? The bottom line is that every single argument that I have seen to support this change, only covers pieces of the picture, not realizing that each small piece is destroying the landscape. This is why I am vehemently opposed to this scoping change, and everyone should be extremely concerned that this is even being considered. |
And one more thing. I don't know if the previous set of rules allowed this, but it would be in line with existing scoping:
Again, this is very useful, but not absolutely required. However, this would not be possible under the currently proposed scoping rules. |
BTW, if someone truly wants
The EDIT: This is "Option 4". |
@Kaelum None of your examples appear to be legal C# 6. You said there was a breaking change. Where is the C# 6 code that has changed behavior?
I can make that statement as the problem occurs in the argument to the call to |
@gafter, I have stated my position. If you don't understand my position, there is nothing more to say.
I just took another look at your code. How is that code even valid? |
In that code, M is never called except when j is definitely assigned. |
@gafter, you used || not &&. If you intended to use &&, and your statements stand, then your definition of the rules is completely different from @MadsTorgersen's definition of the rules as seen at #12939 (comment). |
@Kaelum Your examples are not good examples. I understand there is scary legacy code which has very long functions, but we are talking about new language features which will only be used in new code. When you try to use the new feature in legacy code, you will also do some refactor to extract some sub functions to simplify the beast. So it is not a problem to reuse short variable names.
No, you will not do that. Write a simple function to remove all that duplication: int? ExtractInt(object obj) => obj is int i || int.TryParse(obj, out i) ? i : null; |
What's the purpose of writing a pattern match which is guaranteed to succeed? Is it just to make the local variable readonly? I wonder if such a statement is more likely to be a mistake, in which case it might be better to leave it as an error. One could always write |
Pattern variables aren't readonly anymore. |
@HaloFour Thanks, I didn't realize that. (It's a bit of a shame, I liked how the original proposal with So that being the case, why would anyone write code like above, instead of just |
It is much more compelling when written to get the effect of Swift's |
Only if it explicitly stated that is coder meant, eg by making them explicitly state that intention. I think this is an more of an issue related to introducing variable into scoping. void TestFunc(int i)
{
if (i is int j) { j is in scope and assigned }
else { /* j is in scope and not assigned */ }
Console.WriteLine(j); /* j is in scope and assigned if the pattern succeeds, otherwise not assigned*/
} Inner void TestFunc(int i)
{
if (i is int ~j) { j is in scope and assigned }
else { /* j is not in scope and not assigned */ }
Console.WriteLine(j); /* j is not in scope and not assigned */
} void TestFunc(int i)
{
if (i isnot int ~j) { j is not in scope and not assigned }
else { /* j is in scope and assigned */ }
Console.WriteLine(j); /* j is not in scope and not assigned */
} |
…specified Also clean up some grammar in Syntax.xml Fixes dotnet#14651
This issue arises from a recent question by @uTeisT on pattern-matching.
Consider a pattern-matching operation that always succeeds:
This results in a definite assignment error on the use of j. That is because the current spec says that pattern variables are definitely assigned when true for the pattern-matching operation. The implication is that they are not definitely assigned when then pattern match fails (even though that cannot happen here), and so they are not definitely assigned after the if statement.
We could modify the specification so that a pattern-matching operation that always succeeds results in the pattern variables being definitely assigned. That is, a pattern-matching operation that is irrefutable (i.e. known to always succeed) would cause all variables to be definitely assigned when false.
That would allow this code to compile without error.
Note that the converse situation, a pattern-matching operation that cannot succeed (because, for example, of a type mismatch), is a compile-time error, so no special definite assignment rule is required.
The text was updated successfully, but these errors were encountered: