Improve detection of invalid references for implicitly typed expression variables declared within implicit object creation expressions. #80546
Conversation
…ject creation should be expanded to an argument list immediately enclosing the object creation Fixes dotnet#80518.
|
Fix seems reasonable to me. I'm curious if this needs LDM check? Or does this fall out from the rules of the lang already spec'ed? -- Note: the above is not blocking. I think addressing the crash is important. Just not sure if this means we compile a subset of what the lang thinks should be legal. |
| while (forbiddenZoneOpt?.Parent is ImplicitObjectCreationExpressionSyntax { Parent: ArgumentSyntax { Parent: BaseArgumentListSyntax expanded } }) | ||
| { | ||
| forbiddenZoneOpt = expanded; | ||
| } |
There was a problem hiding this comment.
so, this seems reasonable to me. i'm curious what the right path is when we do with(...) elements. IN other words, we likely will want to expand this to cover [new(out var x), x]. This code seems fairly straightforward to update. But it seems a bit fragile in that i don't get a good sense of what the general rule should be for something being updated here.
There was a problem hiding this comment.
Thanks. I'll see if these scenarios need special treatment too.
| // (9,9): error CS0103: The name 'M1' does not exist in the current context | ||
| // M1(new(out var x), x); | ||
| Diagnostic(ErrorCode.ERR_NameNotInContext, "M1").WithArguments("M1").WithLocation(9, 9), | ||
| // (9,28): error CS8196: Reference to an implicitly-typed out variable 'x' is not permitted in the same argument list. |
There was a problem hiding this comment.
diagnostic is a bit incorrect now. but i suppose it's still clear enough?
it seems like it may need tweaking with collection expression work. perhaps make it vaguer as "reference to implicitly-typed out variable 'x' is not permitted here"?
There was a problem hiding this comment.
diagnostic is a bit incorrect now. but i suppose it's still clear enough?
Looks sufficiently good to me for the given scenarios.
I think it does. An implicit object creation expression is converted to its target type after overload resolution succeeds, types of any out vars declared within the object creation argument list cannot be inferred until then. Therefore, the language cannot know the type of the vars referenced in the same argument list (the type is needed to overload resolution before the |
That makes perfect sense. Thanks. |
…on variables declared within implicit object creation expressions.
out var used as an argument of an implicit object creation should be expanded to an argument list immediately enclosing the object creation| // | ||
| // For simplicity, we may assign nodes that themselves don't represent expressions to 'typeIsKnownAfterBinding'. | ||
|
|
||
| SyntaxNode? typeIsKnownAfterBinding = nodeEnclosingDeclaration; // Strictly speaking, when we are dealind with an argument list, |
There was a problem hiding this comment.
| SyntaxNode? typeIsKnownAfterBinding = nodeEnclosingDeclaration; // Strictly speaking, when we are dealind with an argument list, | |
| SyntaxNode? typeIsKnownAfterBinding = nodeEnclosingDeclaration; // Strictly speaking, when we are dealing with an argument list, | |
| ``` #Closed |
| // the type is known after an overload resolution for an operation | ||
| // owning the argument list is performed and the arguments are converted | ||
| // accordingly. But for our purposes simply pretending that this happens | ||
| // when we bind the argument list itself works pretty well. |
There was a problem hiding this comment.
the formatting of this comment leaves a lot to be desired. Not a fan of multiline done this way after the actual statement.
|
Can review fully tomorrow. Thanks. |
|
@dotnet/roslyn-compiler Please review |
|
@dotnet/roslyn-compiler Please review |
|
@jcouv, @dotnet/roslyn-compiler Please review |
| internal virtual ErrorCode ForbiddenDiagnostic => ErrorCode.ERR_VariableUsedBeforeDeclaration; | ||
| /// </param> | ||
| /// <returns></returns> | ||
| internal virtual bool IsForbiddenReference(SyntaxNode reference, out ErrorCode forbiddenDiagnostic) |
There was a problem hiding this comment.
nit: we only care about this for SourceLocalSymbol and all the call-sites already do such a type check. Consider moving this down into SourceLocalSymbol
Then the call-sites can do if (localSymbol is SourceLocalSymbol { IsVar : true } sourceLocal && sourceLocal.IsForbiddenReference(...)) #Closed
| // inside an argument of a 'new()' have known type before the 'new()' is converted. For example, | ||
| // the following code is legal because of that: | ||
| // | ||
| // M1(new() (M2(out var x)), x); |
There was a problem hiding this comment.
Are these extra parens intentional?
No. Will remove
| // | ||
| // For simplicity, we may assign nodes that themselves don't represent expressions to 'typeIsKnownAfterBinding'. | ||
|
|
||
| SyntaxNode? typeIsKnownAfterBinding = nodeEnclosingDeclaration; // Strictly speaking, when we are dealind with an argument list, |
| case BinaryExpressionSyntax { RawKind: (int)SyntaxKind.CoalesceExpression } coalesce when coalesce.Left == nodeEnclosingDeclaration: | ||
| case AssignmentExpressionSyntax assignment when assignment.Left == nodeEnclosingDeclaration: | ||
| case InitializerExpressionSyntax { RawKind: (int)SyntaxKind.CollectionInitializerExpression }: | ||
| // Ok to reference in this context given the curent binding behavior. |
| case InitializerExpressionSyntax { RawKind: (int)SyntaxKind.ArrayInitializerExpression }: // All initializer elements are bound first and only then they are converted to element type. | ||
| case ExpressionElementSyntax or CollectionExpressionSyntax: // All collection expression elements are bound first and only then they are converted to element type. | ||
| case TupleExpressionSyntax: // All tuple elements are bound first and only then they are converted. | ||
| // Untill conversion happens, the state of type knowledge doesn't change. |
| case ExpressionElementSyntax or CollectionExpressionSyntax: // All collection expression elements are bound first and only then they are converted to element type. | ||
| case TupleExpressionSyntax: // All tuple elements are bound first and only then they are converted. | ||
| // Untill conversion happens, the state of type knowledge doesn't change. | ||
| // For our puposes, we pretend that the parent node (quite possibly an inderect parent, depending on the nesting structure) initiates the conversion. |
| nodeEnclosingDeclaration = parent; | ||
| } | ||
|
|
||
| static bool typeMightBecomeUnknown(SyntaxNode nodeEnclosingDeclaration, SyntaxNode reference) |
There was a problem hiding this comment.
A comment would be useful as this is key function.
In a new(... <current> ...) ... <reference> situation, the type might become unknown because ... #Resolved
| break; | ||
|
|
||
| case ArgumentSyntax: // Argument node itself doesn't change the state of type knowledge. | ||
| case InitializerExpressionSyntax { RawKind: (int)SyntaxKind.ArrayInitializerExpression }: // All initializer elements are bound first and only then they are converted to element type. |
There was a problem hiding this comment.
It think this is clear from the context.
| // For our puposes, we pretend that the parent node (quite possibly an inderect parent, depending on the nesting structure) initiates the conversion. | ||
| break; | ||
|
|
||
| default: |
There was a problem hiding this comment.
I'm wondering how we'll remember to update this method when new constructs are added in the future
There was a problem hiding this comment.
I'm wondering how we'll remember to update this method when new constructs are added in the future
I assume this is a rhetorical question. Or do you have something specific in mind?
There was a problem hiding this comment.
Some options come to mind:
- comment in syntax.xml
- take note in compiler test plan
- add assertion about last syntax kind
- replace the
defaultwith an explicit list of nodes and throw for default case
There was a problem hiding this comment.
comment in syntax.xml
Where and about what?
take note in compiler test plan
What the note would say specifically? I think we already list out vars there.
add assertion about last syntax kind
replace the default with an explicit list of nodes and throw for default case
I do not think these are practical.
There was a problem hiding this comment.
I think we already list out vars there.
We can add a note to remind that every new feature should review the impact on forbidden zone.
I do not think these are practical.
What is impractical about adding an assertion with a comment?
There was a problem hiding this comment.
What is impractical about adding an assertion with a comment?
It is impractical to assert specific kinds that go through the default case, I am not claiming that the added test cover all possible scenarios.
| case BinaryExpressionSyntax { RawKind: (int)SyntaxKind.CoalesceExpression } coalesce when coalesce.Left == nodeEnclosingDeclaration: | ||
| case AssignmentExpressionSyntax assignment when assignment.Left == nodeEnclosingDeclaration: | ||
| case InitializerExpressionSyntax { RawKind: (int)SyntaxKind.CollectionInitializerExpression }: | ||
| // Ok to reference in this context given the curent binding behavior. |
There was a problem hiding this comment.
How do we determine all the syntax kinds that should be here? For example, I'm thinking of member access. If the reference is on the right, would it be okay too? #Resolved
There was a problem hiding this comment.
For example, I'm thinking of member access. If the reference is on the right, would it be okay too?
Member access has a member name on the right. If you have a specific scenario in mind, I'll be happy to add a test.
There was a problem hiding this comment.
It'd be something new(out var x).M(x) or M(new(out var x)).M2(x)
There was a problem hiding this comment.
I have a test for new(out var x)(x). I think the second scenario isn't really interesting because new() is inside an argument list for the receiver and the receiver will be bound first. I will add the tests.
Fixes #80518.