-
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
Added support for implicitly-typed out variables. #11493
Conversation
<value>out variable declaration</value> | ||
</data> | ||
<data name="ERR_OutVariableUsedInTheSameArgumentList" xml:space="preserve"> | ||
<value>Reference to variable '{0}' is not permitted in the same argument list.</value> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing in the spec supports this error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I'll add it. This error is reported only for an implicitly-typed local. I'll clarify the spec and adjust the message too.
Addressed feedback provided so far. |
var compilation = CreateCompilationWithMscorlib(text, options: TestOptions.ReleaseExe, parseOptions: TestOptions.Regular.WithOutVarFeature()); | ||
|
||
compilation.VerifyDiagnostics( | ||
// (6,15): error CS0841: Cannot use local variable 'x1' before it is declared |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error is not supported by the spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you suggesting to specially mention this error situation in the outvar.md? Sure, I'll do that in the next PR.
In reply to: 64250908 [](ancestors = 64250908)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm suggesting you deleted the only constraint on where the variable may be used, and then you implemented a constraint. The result is that the spec and implementation disagree (i.e. a bug). This can be fixed in one of four ways: fixing the spec, adding an issue in .work.md, opening an issue, or removing the constraint.
In reply to: 64252591 [](ancestors = 64252591,64250908)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please be specific, what constraint did I remove (pleas quote it). Please also explain how this error is related to the constraint that is being removed.
In reply to: 64253737 [](ancestors = 64253737,64252591,64250908)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, reporting of this error is not part of this PR. I simply added tests recommended for the previous PR.
In reply to: 64254529 [](ancestors = 64254529,64253737,64252591,64250908)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to other requested references, please, please point to a change in this PR that implements this constraint.
In reply to: 64255223 [](ancestors = 64255223,64254529,64253737,64252591,64250908)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no idea what is this "comment recalled" business. It looks like CodeFlow is going crazy.
In reply to: 64255531 [](ancestors = 64255531,64255223,64254529,64253737,64252591,64250908)
@dotnet/roslyn-compiler Please review. |
You may use the contextual keyword `var` for the variable's type. | ||
The scope will be the same as for a *pattern-variable* introduced via pattern-matching. | ||
Out variables are disallowed within constructor initializers. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You said that this is an open issue ("I think it is worth to take another look at the issue at LDM and confirm whether we still want to stick with the current decision."). Please place it on the open issue list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will do this separately. I am planning to move open issue in a special GitHub Issue. I think that would provide more flexibility.
In reply to: 64253175 [](ancestors = 64253175)
@gafter Please let me know when you are done with review pass. So that I could address all feedback at once. Thanks. |
case SyntaxKind.BaseConstructorInitializer: | ||
break; | ||
default: | ||
throw ExceptionUtilities.UnexpectedValue(node.Identifier.Parent.Parent.Parent.Kind()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It probably makes sense to have more detailed info for this exception.
We are intentionally crashing here if this code ends up being reachable (because of compile errors or some unforeseen reasons). More detailed message might make it easier to diagnose the crash than just unexpected Kind.
I mean - if we do work to detect the case, it makes sense to make message a bit more detailed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked through other places where we throw UnexpectedValue, not a single one of them uses more detailed message. I will add a comment instead.
In reply to: 64272964 [](ancestors = 64272964)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider extracting a local for node.Identifier.Parent.Parent.Parent.Kind()
, if for no other reason so the two uses of that expression cannot get out of sync.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AlekseyTs I am done with this review pass. |
{ | ||
x = 126; | ||
System.Console.WriteLine(x); | ||
return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a test where both overloads have second arg of the same type? (error case)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM |
1 similar comment
LGTM |
Also addressed feeback provided for the previous change.
@dotnet/roslyn-compiler I think I responded to all the feedback. |
It looks like failure of windows_debug_unit64_prtest is a known issue #11368 |
I have more changes blocked on this PR. It has been more that 24 hours without new feedback and I do have two sign-offs. I am going to merge, but additional feedback is welcome. |
Also addressed feedback provided for the previous change.
@dotnet/roslyn-compiler Please review.