-
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
Improved nullable '=='/'!=' analysis #53198
Improved nullable '=='/'!=' analysis #53198
Conversation
d586329
to
fd6536a
Compare
@@ -3805,6 +3893,37 @@ private void AfterLeftChildHasBeenVisited(BoundExpression leftOperand, Conversio | |||
var inferredResult = InferResultNullability(binary.OperatorKind, method, binary.Type, leftType, rightType); | |||
SetResult(binary, inferredResult, inferredResult.ToTypeWithAnnotations(compilation)); | |||
|
|||
TypeSymbol? getTypeIfContainingType(TypeSymbol baseType, TypeSymbol? derivedType) |
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 is just a move.
VisitRvalue(rightOperand); | ||
|
||
var rightType = ResultType; | ||
ReinferBinaryOperatorAndSetResult(leftOperand, leftConversion, leftType, rightOperand, rightConversion, rightType, binary); |
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 extracted the middle "chunk" of this method and left in the parts where we visit the right operand and learn from null tests in the operator.
Looking |
src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs
Show resolved
Hide resolved
Short meeting notes (5/18/2021): Aleksey, Fred, Rikki and I thoroughly explored options, including an approach tracking changed slots. We're thinking that the double-visit approach is still likely the best trade-off (usability/complexity/performance), and we'll do some measurement to validate performance behavior. |
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 (iteration 1)
src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs
Show resolved
Hide resolved
Have briefly investigated the performance with various complex right-hand sides. Benchmark source[SimpleJob(RuntimeMoniker.NetCoreApp50)]
public class ComplexCondAccessEquals
{
private MetadataReference[] coreRefs = null!;
[GlobalSetup]
public void Setup()
{
coreRefs = new[] { MetadataReference.CreateFromFile(typeof(object).Assembly.Location) };
}
[Params(10, 12, 15, 18)]
public int Depth { get; set; }
[Params("enable", "disable")]
public string NullableContext { get; set; } = null!;
[Benchmark]
public void CondAccess_ComplexRightSide()
{
var source1 = @"
#nullable " + NullableContext + @"
object? x = null;
C? c = null;
if (
";
var source2 = @"
)
{
}
class C
{
public bool M(object? obj) => false;
}
";
var sourceBuilder = new StringBuilder();
sourceBuilder.Append(source1);
for (var i = 0; i < Depth; i++)
{
sourceBuilder.AppendLine($" c?.M(x = {i}) == (");
}
sourceBuilder.AppendLine(" c!.M(x)");
sourceBuilder.Append(" ");
for (var i = 0; i < Depth; i++)
{
sourceBuilder.Append(")");
}
sourceBuilder.Append(source2);
var tree = SyntaxFactory.ParseSyntaxTree(sourceBuilder.ToString(), path: "bench.cs");
var comp = CSharpCompilation.Create(
"Benchmark",
new[] { tree },
coreRefs);
var diags = comp.GetDiagnostics();
}
} The scenario looks like: #nullable enable
object? x = null;
C? c = null;
if (
c?.M(x = 0) == (
c?.M(x = 1) == (
c?.M(x = 2) == (
c?.M(x = 3) == (
c?.M(x = 4) == (
c?.M(x = 5) == (
c?.M(x = 6) == (
c?.M(x = 7) == (
c?.M(x = 8) == (
c?.M(x = 9) == (
c!.M(x)
))))))))))
)
{
}
class C
{
public bool M(object? obj) => false;
} BenchmarkDotNet=v0.12.1, OS=Windows 10.0.19042 Job=.NET Core 5.0 Runtime=.NET Core 5.0
It seems to me like the level of performance we're seeing here is adequate. It certainly starts to get bad as the depth increases, but that's approaching the point where we expect only machine-generated code to reach, and machine-generated code like this should probably disable the nullable context regardless. /cc @jcouv @AlekseyTs @333fred |
{ | ||
A a2; | ||
B? b2 = null; | ||
var c2 = b2 ?? a2; |
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 making this just an assignment, rather than relying on the rules of ??
to probably do the thing you want.
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.
Ended up replacing this with a test in FlowTests which attempts more directly to get into a situation where CanPropagateStateWhenNotNull
could be called with a user-defined conversion with an unexpected parameter count. The method is shared between definite assignment and nullable so I feel like the level of testing introduced in the latest commit is adequate.
Done review pass (commit 4) |
Perhaps the right thing is just to remove the new project and let the test case live on in OverloadResolutionPerfTests.cs. |
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
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 (commit 7)
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 Thanks (iteration 7)
Closes #41107
Test plan: #51463
Corresponding definite assignment PR: #52425
Specification (for definite assignment, but many aspects have an analogous fit in nullable analysis): https://github.com/dotnet/csharplang/blob/main/proposals/improved-definite-assignment.md#-relational-equality-operator-expressions