-
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
Fixup IOperation Test Hook #49534
Fixup IOperation Test Hook #49534
Conversation
…ing variable declaration.
… as fields or class methods when referenced in other methods.
…re is nested flow (say in the arguments to a method), the region can keep extending while the capture isn't actually used further.
…ey do not have one.
VerifyOperationTreeAndDiagnosticsForTest<InitializerExpressionSyntax>(comp, expectedOperationTree, expectedDiagnostics); | ||
|
||
var m = comp.SyntaxTrees[0].GetRoot().DescendantNodes().OfType<MethodDeclarationSyntax>().Single(m => m.Identifier.ValueText == "M"); | ||
VerifyFlowGraph(comp, m, @" |
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.
VerifyFlowGraph [](start = 12, length = 15)
It looks like all tests adjusted in the second commit are using target-typed new
. I am assuming regular new
had the same issue. Please confirm. #Closed
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 would be good to add verification for a couple of interesting scenarios with a regular new
.
In reply to: 528940856 [](ancestors = 528940856)
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 looks like it doesn't, because the existing code uses a syntax check rather than IOperation check, which I missed on my first pass. I'll update the syntax check instead, we already have coverage of these scenarios for regular object creation.
In reply to: 528943004 [](ancestors = 528943004,528940856)
@@ -567,7 +567,8 @@ private void VisitMemberReference(IMemberReferenceOperation operation, IEnumerab | |||
if (operation.Member.IsStatic && | |||
operation.Instance is IInstanceReferenceOperation) | |||
{ | |||
Assert.False(operation.Instance.IsImplicit, $"Implicit {nameof(IInstanceReferenceOperation)} on {operation.Syntax}"); | |||
Assert.True(!operation.Instance.IsImplicit || operation.Instance.HasErrors(operation.SemanticModel.Compilation), |
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.
|| operation.Instance.HasErrors(operation.SemanticModel.Compilation) [](start = 62, length = 69)
It feels like this breaks one of the invariants that we have. Why the original condition was fine for a regular member access and a problem for patterns? #Closed
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 feels to me that we should fix the product instead and maintain the invariant.
In reply to: 528948352 [](ancestors = 528948352)
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.
Why the original condition was fine for a regular member access and a problem for patterns?
Patterns include the implicit instance reference on the object being pattern matched against, even though it's a static member. I can adjust the implementation to instead not include that reference, but it may be useful for things introspecting the operation tree?
In reply to: 528952013 [](ancestors = 528952013,528948352)
var isPattern = tree.GetRoot().DescendantNodes().OfType<IsPatternExpressionSyntax>().Single(); | ||
var model = compilation.GetSemanticModel(tree); | ||
var instanceReceiver = model.GetOperation(isPattern).Descendants().OfType<IInstanceReferenceOperation>().Single(); | ||
TestOperationVisitor.Singleton.Visit(instanceReceiver); |
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.
TestOperationVisitor.Singleton.Visit(instanceReceiver); [](start = 12, length = 55)
It is not clear to me what is this doing which VerifyOperationTreeForTest wouldn't already do. Could you please elaborate? #Closed
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.
VerifyOperationTreeForTest
does not call the TestOperationVisitor
on operation nodes. Perhaps it should, but that is what this is accomplishing.
In reply to: 528951526 [](ancestors = 528951526)
@@ -1802,7 +1802,7 @@ public System.Threading.Tasks.Task<bool> MoveNextAsync() | |||
); | |||
} | |||
|
|||
[Fact] | |||
[ConditionalFact(typeof(NoIOperationValidation), Reason = "https://github.com/dotnet/roslyn/issues/49267")] |
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.
} | ||
|
||
[Fact] | ||
public void Foreach_ExtensionGetEnumeratorWithParams() |
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.
Foreach_ExtensionGetEnumeratorWithParams [](start = 20, length = 40)
Do we have a similar test for VB? #Closed
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.
We didn't add extension foreach support to VB, did it already have support for the feature?
In reply to: 529091269 [](ancestors = 529091269)
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.
We didn't add extension foreach support to VB, did it already have support for the feature?
Extension methods were always considered for patterns like that in VB:
Module Ext
<System.Runtime.CompilerServices.Extension>
Function GetEnumerator(this As MyEnumerable, ParamArray array As Integer()) As System.Collections.Generic.IEnumerator(Of Integer)
End Function
End Module
Class MyEnumerable
Sub Test()
For Each x In New MyEnumerable
Next
End Sub
End Class
In reply to: 533002410 [](ancestors = 533002410,529091269)
@@ -1671,7 +1671,7 @@ private IForLoopOperation CreateBoundForStatementOperation(BoundForStatement bou | |||
argumentsToParametersOpt: default, | |||
defaultArguments: default, | |||
argumentRefKindsOpt: default, | |||
expanded: false, | |||
expanded: enumeratorMethod.Parameters.Any(p => p.IsParams), |
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.
enumeratorMethod.Parameters.Any(p => p.IsParams) [](start = 74, length = 48)
This calculation feels fragile and possibly incorrect in a general case. Presence of params
modifier doesn't actually imply that the argument always has an expanded form. For example, the parameter might not have the right type. Or params
, which is just an attribute in metadata, can be on a "wrong" parameter. Etc. See, OverloadResolution.IsValidParams
for example. Rather than trying to re-infer this information, I think we should find a way to thread it through from the binding. Also, with recent work to move creation of default values to binding, I would expect that would also take care of param arrays, when necessary.
#Closed
} | ||
public static class Extensions | ||
{ | ||
public static C.Enumerator GetEnumerator(this C self, params int[] x) => throw 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.
params int[] x [](start = 58, length = 14)
Consider testing scenario when this parameter is optional in metadata and doesn't have an array type. Or, for example, has the right type, but it is not the last parameter, there is also another optional parameter on the method. #Closed
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.
Some error scenarios from source could be interesting too.
In reply to: 529094019 [](ancestors = 529094019)
@@ -2914,6 +2914,8 @@ private static bool AreCloseEnough(Symbol original, Symbol updated) | |||
{ | |||
(LambdaSymbol l, NamedTypeSymbol n) _ when n.IsDelegateType() => AreLambdaAndNewDelegateSimilar(l, n), | |||
(FieldSymbol { ContainingType: { IsTupleType: true }, TupleElementIndex: var oi } originalField, FieldSymbol { ContainingType: { IsTupleType: true }, TupleElementIndex: var ui } updatedField) => | |||
// For tuples with their indexes being private fields, such as in `InferredName_ConditionalOperator_UseSite`, when we do reinference we don't end up inferring the resulting type correctly, |
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.
indexes [](start = 45, length = 7)
I am not sure what "indexes" is referring to. Could you please elaborate? #Closed
case OperationKind.LocalReference: | ||
referencedLocalsAndMethods.Add(((ILocalReferenceOperation)node).Local); | ||
case ILocalReferenceOperation localReference: | ||
if (localReference.Local.ContainingSymbol.IsTopLevelMainMethod() && !associatedSymbol.IsTopLevelMainMethod()) |
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.
!associatedSymbol.IsTopLevelMainMethod() [](start = 96, length = 40)
(object)localReference.Local.ContainingSymbol !=associatedSymbol
? #Closed
case OperationKind.LocalReference: | ||
referencedLocalsAndMethods.Add(((ILocalReferenceOperation)node).Local); | ||
case ILocalReferenceOperation localReference: | ||
if (localReference.Local.ContainingSymbol.IsTopLevelMainMethod() && !associatedSymbol.IsTopLevelMainMethod()) |
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.
associatedSymbol [](start = 97, length = 16)
Perhaps we should assert somewhere that associatedSymbol
is not a local function or a lambda, otherwise the condition might not be accurate. #Closed
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's fine if it's a local function or lambda, but we likely want to skip only if that local function or lambda isn't itself part of the top-level main.
In reply to: 529792485 [](ancestors = 529792485)
@@ -1045,6 +1097,12 @@ bool isLongLivedCaptureReferenceSyntax(SyntaxNode captureReferenceSyntax) | |||
} | |||
|
|||
CSharpSyntaxNode parent = syntax.Parent; | |||
while (parent is PostfixUnaryExpressionSyntax { OperatorToken: { RawKind: (int)CSharp.SyntaxKind.ExclamationToken } }) |
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.
while (parent is PostfixUnaryExpressionSyntax { OperatorToken: { RawKind: (int)CSharp.SyntaxKind.ExclamationToken } }) [](start = 28, length = 118)
I think this logic should be incorporated into applyParenthesizedIfAnyCS
and the helper renamed accordingly #Closed
@@ -1102,6 +1160,13 @@ bool isLongLivedCaptureReferenceSyntax(SyntaxNode captureReferenceSyntax) | |||
return true; | |||
} | |||
break; | |||
|
|||
case CSharp.SyntaxKind.ConditionalAccessExpression: | |||
if (((CSharp.Syntax.ConditionalAccessExpressionSyntax)syntax.Parent).Expression == syntax) |
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.
if (((CSharp.Syntax.ConditionalAccessExpressionSyntax)syntax.Parent).Expression == syntax) [](start = 36, length = 90)
I think we should see if we want to fix the builder instead. Looking at the added NestedConditionalAccess
unit-test, the life-time of capture 0 is extended unnecessarily. #Closed
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 any case I do not think we should be treating it as long lived (i.e. something that is known to span multiple statements). So the relaxation should probably be applied elsewhere
In reply to: 529818891 [](ancestors = 529818891)
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.
After much offline investigation and discussion, I will be tightening the assertion, but we will be keeping the builder as is. It's not ideal behavior, but would require significant effort to make ideal with significant maintenance burden, and is not worth it for this case.
In reply to: 530039773 [](ancestors = 530039773,529818891)
.locals {R2} | ||
{ | ||
CaptureIds: [0] [2] [4] | ||
Block[B1] - Block |
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.
Block[B1] - Block [](start = 8, length = 17)
I think we actually should have a nested region here. it should enclose B1, B2 and own capture [0]. #Closed
@@ -349,7 +349,7 @@ static void F(IEnumerable<int[]> x) | |||
comp.VerifyDiagnostics(); | |||
} | |||
|
|||
[Fact] | |||
[ConditionalFact(typeof(NoIOperationValidation), Reason = "Timeouts")] |
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.
Timeouts [](start = 67, length = 8)
Should we have an issue to track disable tests like that? #Closed
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 don't think that's particularly useful, as I'm not currently thinking that we'll be attempting to renable them in the future.
In reply to: 529820501 [](ancestors = 529820501)
// If the arguments didn't have a natural type, they're not constants, and so would already have diagnostics from the rest of binding. | ||
var ignoredDiagnostics = new DiagnosticBag(); | ||
ImmutableArray<BoundExpression> boundConstructorArguments = constructorArguments.Arguments.SelectAsArray( | ||
(arg, thisAndDiagnostics) => thisAndDiagnostics.@this.BindToNaturalType(arg, thisAndDiagnostics.ignoredDiagnostics), |
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.
BindToNaturalType [](start = 70, length = 17)
BindToTypeForErrorRecovery
?
#Closed
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.
We also have a BuildArgumentsForErrorRecovery
, see if we can use that helper.
In reply to: 529823025 [](ancestors = 529823025)
ImmutableArray<string> boundConstructorArgumentNamesOpt = constructorArguments.GetNames(); | ||
ImmutableArray<BoundExpression> boundNamedArguments = analyzedArguments.NamedArguments; | ||
ImmutableArray<BoundExpression> boundNamedArguments = analyzedArguments.NamedArguments.SelectAsArray( | ||
(namedArg, thisAndDiagnostics) => thisAndDiagnostics.@this.BindToNaturalType(namedArg, thisAndDiagnostics.ignoredDiagnostics), |
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.
BindToNaturalType [](start = 75, length = 17)
BindToTypeForErrorRecovery
? #Closed
|
||
// If the arguments didn't have a natural type, they're not constants, and so would already have diagnostics from the rest of binding. | ||
var ignoredDiagnostics = new DiagnosticBag(); | ||
ImmutableArray<BoundExpression> boundConstructorArguments = constructorArguments.Arguments.SelectAsArray( |
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.
constructorArguments.Arguments [](start = 72, length = 30)
Are we leaking this builder? #Closed
ImmutableArray<string> boundConstructorArgumentNamesOpt = constructorArguments.GetNames(); | ||
ImmutableArray<BoundExpression> boundNamedArguments = analyzedArguments.NamedArguments; | ||
ImmutableArray<BoundExpression> boundNamedArguments = analyzedArguments.NamedArguments.SelectAsArray( |
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.
analyzedArguments.NamedArguments [](start = 66, length = 32)
See if we want to change BindAttributeArguments
to keep it as a builder since we are going to allocate a new array here. #Closed
if (enumeratorMethod.ParameterCount > 1) | ||
{ | ||
var ignoredDiagnostics = new DiagnosticBag(); | ||
enumeratorInfoOpt.Binder.BindDefaultArguments(boundForEachStatement.Expression.Syntax, enumeratorMethod.Parameters, argumentsBuilder, argumentRefKindsBuilder: null, ref argsToParams, out defaultArguments, expanded: true, enableCallerInfo: true, ignoredDiagnostics); |
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.
true [](start = 239, length = 4)
Why is this true
? #Closed
@@ -1646,6 +1646,40 @@ private IForLoopOperation CreateBoundForStatementOperation(BoundForStatement bou | |||
HashSet<DiagnosticInfo>? useSiteDiagnostics = null; | |||
var compilation = (CSharpCompilation)_semanticModel.Compilation; | |||
|
|||
ImmutableArray<IArgumentOperation> getEnumeratorArguments; | |||
if (enumeratorInfoOpt.GetEnumeratorMethod is { IsExtensionMethod: true } enumeratorMethod) |
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.
true [](start = 82, length = 4)
Can we run into an instance method with default values and/or param array? #Closed
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 commit is no longer necessary, dropping from the updated PR.
In reply to: 529836543 [](ancestors = 529836543)
argumentsBuilder.ToImmutableAndFree(), | ||
argumentNamesOpt: default, | ||
argsToParams, | ||
defaultArguments, |
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.
defaultArguments [](start = 28, length = 16)
Is there a test where we can observe the difference? #Closed
break; | ||
case CSharp.SyntaxKind.SimpleAssignmentExpression: | ||
if (((CSharp.Syntax.AssignmentExpressionSyntax)syntax.Parent).Left == syntax | ||
&& syntax.Parent.Parent is InitializerExpressionSyntax { Parent: CSharp.Syntax.ObjectCreationExpressionSyntax }) |
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.
&& syntax.Parent.Parent is InitializerExpressionSyntax { Parent: CSharp.Syntax.ObjectCreationExpressionSyntax }) [](start = 40, length = 112)
Would it be possible to tighten the condition by also checking something other than syntax? Perhaps check symbols involved? #Closed
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 do not think we should recognize this as long lived, it doesn't escape the statement.
In reply to: 530036537 [](ancestors = 530036537)
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.
Could isObjectOrArrayInitializerInitializedObjectTarget help for this scenario?
In reply to: 530040111 [](ancestors = 530040111,530036537)
@@ -612,7 +612,8 @@ private BoundExpression UnwrapCollectionExpressionIfNullable(BoundExpression col | |||
return BoundCall.Synthesized( | |||
syntax: exprSyntax, | |||
receiverOpt: collectionExpr, | |||
method: nullableValueGetter); | |||
method: nullableValueGetter, | |||
binder: this); |
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.
binder: this [](start = 24, length = 12)
Is the problem that we didn't pass a binder when it was needed? Can we add an assert somewhere that we got null binder when non-null is expected to be passed? #Closed
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 the problem that we didn't pass a binder when it was needed?
Yes, it's needed for DeriveArguments
.
Can we add an assert somewhere that we got null binder when non-null is expected to be passed?
It's actually an assertion in DeriveArguments
that caught this, we just didn't have a test case that verified the IOperation tree for this code path. I'm going to need to incorporate this change into the branch changes. We still have the bug in master, but the assertions I added when nullable enabling are blocking my ability to run benchmarks.
In reply to: 530043147 [](ancestors = 530043147)
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.
Done with review pass (commit 17) |
Subsumed by #51206. |
This fixes up a number of issues discovered by running the IOperation test hook on the compiler-layer tests. Commit-by-commit is highly recommended here: each commit is one fix, and tests to accompany that fix. After Rikki's work to restructure runtests goes into master, I intend to enable a test leg that actually runs these tests on the compiler layer as part of regular CI, which will hopefully help us catch regressions.