Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Nullable analysis of conditional operator should use best common type unless target typed #73047

Merged
merged 6 commits into from
Apr 18, 2024
Merged

Nullable analysis of conditional operator should use best common type unless target typed #73047

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2984634
Nullable analysis of conditional operator should use best common type…
cston Apr 16, 2024
d7482f3
Use best common type
cston Apr 16, 2024
cf778b5
Update test
cston Apr 16, 2024
c2594be
GetTypeInfo()
cston Apr 16, 2024
5502ded
Add assert
cston Apr 16, 2024
45b901a
Check GetTypeInfo() for conditional
cston Apr 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 1 addition & 6 deletions src/Compilers/CSharp/Portable/Binder/Binder_Operators.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4339,23 +4339,18 @@ private BoundExpression BindValueConditionalOperator(ConditionalExpressionSyntax
return new BoundUnconvertedConditionalOperator(node, condition, trueExpr, falseExpr, constantValue, noCommonTypeError, hasErrors: constantValue?.IsBad == true);
}

TypeSymbol type;
bool hasErrors;
if (bestType.IsErrorType())
{
trueExpr = BindToNaturalType(trueExpr, diagnostics, reportNoTargetType: false);
falseExpr = BindToNaturalType(falseExpr, diagnostics, reportNoTargetType: false);
type = bestType;
hasErrors = true;
}
else
{
trueExpr = GenerateConversionForAssignment(bestType, trueExpr, diagnostics);
falseExpr = GenerateConversionForAssignment(bestType, falseExpr, diagnostics);
hasErrors = trueExpr.HasAnyErrors || falseExpr.HasAnyErrors;
// If one of the conversions went wrong (e.g. return type of method group being converted
// didn't match), then we don't want to use bestType because it's not accurate.
type = hasErrors ? CreateErrorType() : bestType;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To fix the bug, it seems sufficient to either take this change (to use the best common type in binding always), or the change in NullableWalker (to use best common in nullable analysis always). But both changes seemed appropriate to me.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me check my understanding.

  1. In this path, bestType is not null and not ErrorType. If one of the conditional expr operands had an ErrorType, we would have inferred ErrorType as the bestType.
  2. The removed lines here are checking if there is some error within the operands, which didn't cause them to have an ErrorType, per (1). We previously thought in this case that we need to change the conditional-expr type to ErrorType in this case. Now we are simply keeping the bestType which we already determined in the face of such errors.

Basically I'm not seeing what purpose the original check was serving, it feels like this change is just propagating along reasonable type information, which allows our assumptions about the bound tree shape to be met when we reach nullable analysis.

Copy link
Contributor

@RikkiGibson RikkiGibson Apr 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intent, from the comment, seems to be that we are doing a conversion check from the operands to the best type, which can fail, and I also find that a bit confusing. One issue here is that we are not checking specifically if the conversion had errors, we are checking for any kind of errors within the whole operand.

Essentially if one of the operands doesn't convert to the "best type", why was that chosen as the "best type"? Are there some invariants in best type analysis we can rely on in this regard?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BestTypeInferrer.InferBestTypeForConditionalOperator() and GenerateConversionForAssignment() are both using ClassifyImplicitConversionFromExpression(), so it's not clear when the GenerateConversionForAssignment() calls above would fail if the best common type is not an error type.

}

if (!hasErrors)
Expand All @@ -4364,7 +4359,7 @@ private BoundExpression BindValueConditionalOperator(ConditionalExpressionSyntax
hasErrors = constantValue != null && constantValue.IsBad;
}

return new BoundConditionalOperator(node, isRef: false, condition, trueExpr, falseExpr, constantValue, naturalTypeOpt: type, wasTargetTyped: false, type, hasErrors);
return new BoundConditionalOperator(node, isRef: false, condition, trueExpr, falseExpr, constantValue, naturalTypeOpt: bestType, wasTargetTyped: false, bestType, hasErrors);
}
#nullable disable

Expand Down
3 changes: 2 additions & 1 deletion src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5867,7 +5867,7 @@ void makeAndAdjustReceiverSlot(BoundExpression receiver)

TypeSymbol? resultType;
bool wasTargetTyped = node is BoundConditionalOperator { WasTargetTyped: true };
if (node.HasErrors || wasTargetTyped)
if (wasTargetTyped)
Copy link
Contributor

@RikkiGibson RikkiGibson Apr 16, 2024 •
edited by cston
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to change the condition on 5898 if (!wasTargetTyped) to Debug.Assert(!wasTargetTyped)? #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added an assert rather than replacing the if.

{
resultType = null;
}
Expand Down Expand Up @@ -5895,6 +5895,7 @@ void makeAndAdjustReceiverSlot(BoundExpression receiver)

if (resultType is null)
{
Debug.Assert(!wasTargetTyped);
if (!wasTargetTyped)
{
// This can happen when we're inferring the return type of a lambda or visiting a node without diagnostics like
Expand Down
203 changes: 203 additions & 0 deletions src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37997,6 +37997,209 @@ static void Main()
Diagnostic(ErrorCode.ERR_NoTypeDef, "[x, ..y]").WithArguments("MyCollectionA<>", $"{assemblyA}, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null").WithLocation(7, 35));
}

[WorkItem("https://github.com/dotnet/roslyn/issues/72898")]
[Fact]
public void Nullable_ConditionalOperator_Error()
{
string sourceA = """
using System;
using System.Collections;
using System.Collections.Generic;
using System.Runtime.CompilerServices;
[CollectionBuilder(typeof(MyCollectionBuilder), "Create")]
struct MyCollection<T> : IEnumerable<T>
{
IEnumerator<T> IEnumerable<T>.GetEnumerator() => null;
IEnumerator IEnumerable.GetEnumerator() => null;
}
class MyCollectionBuilder
{
public static MyCollection<T> Create<T>(ReadOnlySpan<T> items) => default;
}
""";
string sourceB = """
#nullable enable
using System.Collections.Generic;
class Program
{
static IEnumerable<object> F(bool b, MyCollection<object> x, object y)
{
return b ? x : [y);
}
}
""";

var comp = CreateCompilation([sourceB, sourceA], targetFramework: TargetFramework.Net80);
comp.VerifyEmitDiagnostics(
// (7,26): error CS1003: Syntax error, ',' expected
// return b ? x : [y);
Diagnostic(ErrorCode.ERR_SyntaxError, ")").WithArguments(",").WithLocation(7, 26),
// (7,27): error CS1003: Syntax error, ']' expected
// return b ? x : [y);
Diagnostic(ErrorCode.ERR_SyntaxError, ";").WithArguments("]").WithLocation(7, 27));

var tree = comp.SyntaxTrees[0];
var model = comp.GetSemanticModel(tree);
var nodes = tree.GetRoot().DescendantNodes();
var expr = nodes.OfType<IdentifierNameSyntax>().Last();
Assert.Equal("y", expr.ToString());
Copy link
Member

@jcouv jcouv Apr 18, 2024 •
edited by cston
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider verifying the result of GetSymbolInfo on the conditional expression since the change to BoundConditionalOperator.NaturalTypeOpt is surfaced through .Type from this API #Resolved

_ = model.GetSymbolInfo(expr);

var conditional = nodes.OfType<ConditionalExpressionSyntax>().Single();
var info = model.GetTypeInfo(conditional);
Assert.Equal("MyCollection<System.Object>", info.Type.ToTestDisplayString());
}

[Fact]
public void Nullable_SwitchExpression_Error()
{
string sourceA = """
using System;
using System.Collections;
using System.Collections.Generic;
using System.Runtime.CompilerServices;
[CollectionBuilder(typeof(MyCollectionBuilder), "Create")]
struct MyCollection<T> : IEnumerable<T>
{
IEnumerator<T> IEnumerable<T>.GetEnumerator() => null;
IEnumerator IEnumerable.GetEnumerator() => null;
}
class MyCollectionBuilder
{
public static MyCollection<T> Create<T>(ReadOnlySpan<T> items) => default;
}
""";
string sourceB = """
#nullable enable
using System.Collections.Generic;
class Program
{
static void F(bool b, MyCollection<object> x, object y)
{
_ = b switch
{
true => x,
false => [y),
};
}
}
""";

var comp = CreateCompilation([sourceB, sourceA], targetFramework: TargetFramework.Net80);
comp.VerifyEmitDiagnostics(
// (10,28): error CS1003: Syntax error, ',' expected
// false => [y),
Diagnostic(ErrorCode.ERR_SyntaxError, ")").WithArguments(",").WithLocation(10, 28),
// (10,30): error CS1003: Syntax error, ']' expected
// false => [y),
Diagnostic(ErrorCode.ERR_SyntaxError, "").WithArguments("]").WithLocation(10, 30));

var tree = comp.SyntaxTrees[0];
var model = comp.GetSemanticModel(tree);
var expr = tree.GetRoot().DescendantNodes().OfType<IdentifierNameSyntax>().Last();
Assert.Equal("y", expr.ToString());
_ = model.GetSymbolInfo(expr);
}

[Fact]
public void Nullable_ArrayInitializer_Error()
{
string sourceA = """
using System;
using System.Collections;
using System.Collections.Generic;
using System.Runtime.CompilerServices;
[CollectionBuilder(typeof(MyCollectionBuilder), "Create")]
struct MyCollection<T> : IEnumerable<T>
{
IEnumerator<T> IEnumerable<T>.GetEnumerator() => null;
IEnumerator IEnumerable.GetEnumerator() => null;
}
class MyCollectionBuilder
{
public static MyCollection<T> Create<T>(ReadOnlySpan<T> items) => default;
}
""";
string sourceB = """
#nullable enable
using System.Collections.Generic;
class Program
{
static void F(MyCollection<object> x, object y)
{
_ = new[] { x, [y) };
}
}
""";

var comp = CreateCompilation([sourceB, sourceA], targetFramework: TargetFramework.Net80);
comp.VerifyEmitDiagnostics(
// (7,26): error CS1003: Syntax error, ',' expected
// _ = new[] { x, [y) };
Diagnostic(ErrorCode.ERR_SyntaxError, ")").WithArguments(",").WithLocation(7, 26),
// (7,28): error CS1003: Syntax error, ']' expected
// _ = new[] { x, [y) };
Diagnostic(ErrorCode.ERR_SyntaxError, "}").WithArguments("]").WithLocation(7, 28));

var tree = comp.SyntaxTrees[0];
var model = comp.GetSemanticModel(tree);
var expr = tree.GetRoot().DescendantNodes().OfType<IdentifierNameSyntax>().Last();
Assert.Equal("y", expr.ToString());
_ = model.GetSymbolInfo(expr);
}

[Fact]
public void Nullable_LambdaExpression_Error()
{
string sourceA = """
using System;
using System.Collections;
using System.Collections.Generic;
using System.Runtime.CompilerServices;
[CollectionBuilder(typeof(MyCollectionBuilder), "Create")]
struct MyCollection<T> : IEnumerable<T>
{
IEnumerator<T> IEnumerable<T>.GetEnumerator() => null;
IEnumerator IEnumerable.GetEnumerator() => null;
}
class MyCollectionBuilder
{
public static MyCollection<T> Create<T>(ReadOnlySpan<T> items) => default;
}
""";
string sourceB = """
#nullable enable
using System;
using System.Collections.Generic;
class Program
{
static void F(MyCollection<object> x, object y)
{
var f = (bool b) =>
{
if (b) return x;
return [y);
};
}
}
""";

var comp = CreateCompilation([sourceB, sourceA], targetFramework: TargetFramework.Net80);
comp.VerifyEmitDiagnostics(
// (11,26): error CS1003: Syntax error, ',' expected
// return [y);
Diagnostic(ErrorCode.ERR_SyntaxError, ")").WithArguments(",").WithLocation(11, 26),
// (11,27): error CS1003: Syntax error, ']' expected
// return [y);
Diagnostic(ErrorCode.ERR_SyntaxError, ";").WithArguments("]").WithLocation(11, 27));

var tree = comp.SyntaxTrees[0];
var model = comp.GetSemanticModel(tree);
var expr = tree.GetRoot().DescendantNodes().OfType<IdentifierNameSyntax>().Last();
Assert.Equal("y", expr.ToString());
_ = model.GetSymbolInfo(expr);
}

[Fact]
public void SynthesizedReadOnlyList_SingleElement()
{
Expand Down
49 changes: 24 additions & 25 deletions src/Compilers/CSharp/Test/IOperation/IOperation/IOperationTests_IFixedStatement.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.CSharp.Test.Utilities;
using Microsoft.CodeAnalysis.Test.Utilities;
using System.Linq;
using Xunit;

namespace Microsoft.CodeAnalysis.CSharp.UnitTests
Expand Down Expand Up @@ -599,7 +600,6 @@ unsafe void M(bool b)
Statements (0)
Next (Regular) Block[B1]
Entering: {R1} {R2}

.locals {R1}
{
Locals: [System.Int32* p]
Expand All @@ -611,89 +611,88 @@ unsafe void M(bool b)
Statements (0)
Jump if False (Regular) to Block[B3]
IParameterReferenceOperation: b (OperationKind.ParameterReference, Type: System.Boolean) (Syntax: 'b')

Next (Regular) Block[B2]
Block[B2] - Block
Predecessors: [B1]
Statements (1)
IFlowCaptureOperation: 0 (OperationKind.FlowCapture, Type: null, IsInvalid, IsImplicit) (Syntax: '&i1')
Value:
Value:
IAddressOfOperation (OperationKind.AddressOf, Type: System.Int32*, IsInvalid) (Syntax: '&i1')
Reference:
Reference:
IFieldReferenceOperation: System.Int32 MyClass.i1 (OperationKind.FieldReference, Type: System.Int32, IsInvalid) (Syntax: 'i1')
Instance Receiver:
Instance Receiver:
IInstanceReferenceOperation (ReferenceKind: ContainingTypeInstance) (OperationKind.InstanceReference, Type: MyClass, IsInvalid, IsImplicit) (Syntax: 'i1')

Next (Regular) Block[B4]
Block[B3] - Block
Predecessors: [B1]
Statements (1)
IFlowCaptureOperation: 0 (OperationKind.FlowCapture, Type: null, IsInvalid, IsImplicit) (Syntax: '&i2')
Value:
Value:
IAddressOfOperation (OperationKind.AddressOf, Type: System.Int32*, IsInvalid) (Syntax: '&i2')
Reference:
Reference:
IFieldReferenceOperation: System.Int32 MyClass.i2 (OperationKind.FieldReference, Type: System.Int32, IsInvalid) (Syntax: 'i2')
Instance Receiver:
Instance Receiver:
IInstanceReferenceOperation (ReferenceKind: ContainingTypeInstance) (OperationKind.InstanceReference, Type: MyClass, IsInvalid, IsImplicit) (Syntax: 'i2')

Next (Regular) Block[B4]
Block[B4] - Block
Predecessors: [B2] [B3]
Statements (1)
ISimpleAssignmentOperation (OperationKind.SimpleAssignment, Type: System.Int32*, IsInvalid, IsImplicit) (Syntax: 'p = b ? &i1 : &i2')
Left:
Left:
ILocalReferenceOperation: p (IsDeclaration: True) (OperationKind.LocalReference, Type: System.Int32*, IsInvalid, IsImplicit) (Syntax: 'p = b ? &i1 : &i2')
Right:
IFlowCaptureReferenceOperation: 0 (OperationKind.FlowCaptureReference, Type: ?, IsInvalid, IsImplicit) (Syntax: 'b ? &i1 : &i2')

Right:
IFlowCaptureReferenceOperation: 0 (OperationKind.FlowCaptureReference, Type: System.Int32*, IsInvalid, IsImplicit) (Syntax: 'b ? &i1 : &i2')
Copy link
Contributor

@RikkiGibson RikkiGibson Apr 16, 2024 •
edited by cston
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this is the meaningful change in here. I didn't notice it until after looking at it for a bit.

Perhaps we should also exercise the difference in semanticModel.GetTypeInfo(conditionalExprSyntax) as a result of this change. #Resolved

Next (Regular) Block[B5]
Leaving: {R2}
}

Block[B5] - Block
Predecessors: [B4]
Statements (0)
Jump if False (Regular) to Block[B7]
IParameterReferenceOperation: b (OperationKind.ParameterReference, Type: System.Boolean) (Syntax: 'b')
Leaving: {R1}

Next (Regular) Block[B6]
Block[B6] - Block
Predecessors: [B5]
Statements (1)
IExpressionStatementOperation (OperationKind.ExpressionStatement, Type: null) (Syntax: 'System.Cons ... is {*p}"");')
Expression:
Expression:
IInvocationOperation (void System.Console.WriteLine(System.String value)) (OperationKind.Invocation, Type: System.Void) (Syntax: 'System.Cons ... P is {*p}"")')
Instance Receiver:
Instance Receiver:
null
Arguments(1):
IArgumentOperation (ArgumentKind.Explicit, Matching Parameter: value) (OperationKind.Argument, Type: null) (Syntax: '$""P is {*p}""')
IInterpolatedStringOperation (OperationKind.InterpolatedString, Type: System.String) (Syntax: '$""P is {*p}""')
Parts(2):
IInterpolatedStringTextOperation (OperationKind.InterpolatedStringText, Type: null) (Syntax: 'P is ')
Text:
Text:
ILiteralOperation (OperationKind.Literal, Type: System.String, Constant: ""P is "", IsImplicit) (Syntax: 'P is ')
IInterpolationOperation (OperationKind.Interpolation, Type: null) (Syntax: '{*p}')
Expression:
Expression:
IOperation: (OperationKind.None, Type: System.Int32) (Syntax: '*p')
Children(1):
ILocalReferenceOperation: p (OperationKind.LocalReference, Type: System.Int32*) (Syntax: 'p')
Alignment:
Alignment:
null
FormatString:
FormatString:
null
InConversion: CommonConversion (Exists: True, IsIdentity: True, IsNumeric: False, IsReference: False, IsUserDefined: False) (MethodSymbol: null)
OutConversion: CommonConversion (Exists: True, IsIdentity: True, IsNumeric: False, IsReference: False, IsUserDefined: False) (MethodSymbol: null)

Next (Regular) Block[B7]
Leaving: {R1}
}

Block[B7] - Exit
Predecessors: [B5] [B6]
Statements (0)
";
VerifyFlowGraphAndDiagnosticsForTest<BlockSyntax>(source, expectedFlowGraph, expectedDiagnostics, compilationOptions: TestOptions.UnsafeDebugDll);
var comp = CreateCompilation(source, options: TestOptions.UnsafeDebugDll);
VerifyFlowGraphAndDiagnosticsForTest<BlockSyntax>(comp, expectedFlowGraph, expectedDiagnostics);

var tree = comp.SyntaxTrees[0];
var model = comp.GetSemanticModel(tree);
var expr = tree.GetRoot().DescendantNodes().OfType<ConditionalExpressionSyntax>().Single();
var info = model.GetTypeInfo(expr);
Assert.Equal("System.Int32*", info.Type.ToTestDisplayString());
}

[CompilerTrait(CompilerFeature.IOperation, CompilerFeature.Dataflow)]
Expand Down