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

Infer null state from is object and is { } tests #40892

Merged
merged 11 commits into from Jan 14, 2020
Merged

Infer null state from is object and is { } tests #40892

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
7883a31
Infer null state from `is object` and `is { }` tests
cston Jan 9, 2020
eb187d5
Update source using `is object`
cston Jan 10, 2020
666f3a8
Revert changes to BoundNodes.xml.Generated.vb
cston Jan 10, 2020
1d6b4e3
PR feedback
cston Jan 10, 2020
2eb10b4
Add BoundDagNonNullTest.IsExplicitTest field
cston Jan 10, 2020
85343e7
PR feedback
cston Jan 11, 2020
9a27143
Misc.
cston Jan 12, 2020
1534256
Fix build
cston Jan 12, 2020
1fd3753
Fix test
cston Jan 12, 2020
0929381
Fix formatting
cston Jan 12, 2020
04ead25
PR feedback
cston Jan 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 11 additions & 4 deletions src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs
View file
Open in desktop
Expand Up @@ -579,10 +579,16 @@ private BoundPattern BindRecursivePattern(RecursivePatternSyntax node, TypeSymbo
BindPatternDesignation(
node.Designation, declTypeWithAnnotations, inputValEscape, typeSyntax, diagnostics,
ref hasErrors, out Symbol variableSymbol, out BoundExpression variableAccess);
bool isExplicitNotNullTest =
node.Designation is null &&
boundDeclType is null &&
properties.IsDefaultOrEmpty &&
deconstructMethod is null &&
deconstructionSubpatterns.IsDefault;
return new BoundRecursivePattern(
syntax: node, declaredType: boundDeclType, inputType: inputType, deconstructMethod: deconstructMethod,
syntax: node, declaredType: boundDeclType, deconstructMethod: deconstructMethod,
deconstruction: deconstructionSubpatterns, properties: properties, variable: variableSymbol,
variableAccess: variableAccess, hasErrors: hasErrors);
variableAccess: variableAccess, isExplicitNotNullTest: isExplicitNotNullTest, inputType: inputType, hasErrors: hasErrors);
}

private MethodSymbol BindDeconstructSubpatterns(
Expand Down Expand Up @@ -933,8 +939,9 @@ private BoundPattern BindVarPattern(VarPatternSyntax node, TypeSymbol inputType,
}

return new BoundRecursivePattern(
syntax: node, declaredType: null, inputType: inputType, deconstructMethod: deconstructMethod,
deconstruction: subPatterns.ToImmutableAndFree(), properties: default, variable: null, variableAccess: null, hasErrors: hasErrors);
syntax: node, declaredType: null, deconstructMethod: deconstructMethod,
deconstruction: subPatterns.ToImmutableAndFree(), properties: default, variable: null, variableAccess: null,
isExplicitNotNullTest: false, inputType: inputType, hasErrors: hasErrors);

void addSubpatternsForTuple(ImmutableArray<TypeWithAnnotations> elementTypes)
{
Expand Down
14 changes: 8 additions & 6 deletions src/Compilers/CSharp/Portable/Binder/DecisionDagBuilder.cs
View file
Open in desktop
Expand Up @@ -5,7 +5,6 @@
using System.Collections.Immutable;
using System.Diagnostics;
using System.Linq;
using System.Text;
using Microsoft.CodeAnalysis.CSharp.Symbols;
using Microsoft.CodeAnalysis.PooledObjects;
using Roslyn.Utilities;
Expand Down Expand Up @@ -339,7 +338,7 @@ private DecisionDagBuilder(CSharpCompilation compilation, LabelSymbol defaultLab
// Add a null and type test if needed.
if (!declaration.IsVar)
{
input = MakeConvertToType(input, declaration.Syntax, type, tests);
input = MakeConvertToType(input, declaration.Syntax, type, isExplicitTest: false, tests);
}

BoundExpression variableAccess = declaration.VariableAccess;
Expand All @@ -357,12 +356,13 @@ private DecisionDagBuilder(CSharpCompilation compilation, LabelSymbol defaultLab
private static void MakeCheckNotNull(
BoundDagTemp input,
SyntaxNode syntax,
bool isExplicitTest,
ArrayBuilder<BoundDagTest> tests)
{
if (input.Type.CanContainNull())
{
// Add a null test
tests.Add(new BoundDagNonNullTest(syntax, input));
tests.Add(new BoundDagNonNullTest(syntax, isExplicitTest, input));
}
}

Expand All @@ -373,9 +373,10 @@ private DecisionDagBuilder(CSharpCompilation compilation, LabelSymbol defaultLab
BoundDagTemp input,
SyntaxNode syntax,
TypeSymbol type,
bool isExplicitTest,
ArrayBuilder<BoundDagTest> tests)
{
MakeCheckNotNull(input, syntax, tests);
MakeCheckNotNull(input, syntax, isExplicitTest, tests);
if (!input.Type.Equals(type, TypeCompareKind.AllIgnoreOptions))
{
TypeSymbol inputType = input.Type.StrippedType(); // since a null check has already been done
Expand Down Expand Up @@ -412,7 +413,7 @@ private DecisionDagBuilder(CSharpCompilation compilation, LabelSymbol defaultLab
}
else
{
var convertedInput = MakeConvertToType(input, constant.Syntax, constant.Value.Type, tests);
var convertedInput = MakeConvertToType(input, constant.Syntax, constant.Value.Type, isExplicitTest: false, tests);
tests.Add(new BoundDagValueTest(constant.Syntax, constant.ConstantValue, convertedInput));
}
}
Expand All @@ -438,8 +439,9 @@ private static void Assert(bool condition, string message = null)
ArrayBuilder<BoundPatternBinding> bindings)
{
Debug.Assert(input.Type.IsErrorType() || recursive.InputType.IsErrorType() || input.Type.Equals(recursive.InputType, TypeCompareKind.AllIgnoreOptions));

var inputType = recursive.DeclaredType?.Type ?? input.Type.StrippedType();
input = MakeConvertToType(input, recursive.Syntax, inputType, tests);
input = MakeConvertToType(input, recursive.Syntax, inputType, isExplicitTest: recursive.IsExplicitNotNullTest, tests);

if (!recursive.Deconstruction.IsDefault)
{
Expand Down
2 changes: 2 additions & 0 deletions src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml
View file
Open in desktop
Expand Up @@ -1318,6 +1318,7 @@
</Node>
<Node Name="BoundDagNonNullTest" Base="BoundDagTest">
<!--Check that the input is not null. Used as part of a type test.-->
<Field Name="IsExplicitTest" Type="bool"/>
</Node>
<Node Name="BoundDagExplicitNullTest" Base="BoundDagTest">
<!--Check that the input is null. Used when an explicit null test appears in source.-->
Expand Down Expand Up @@ -1984,6 +1985,7 @@
https://github.com/dotnet/roslyn/issues/13960 . When that is fixed this field can be
removed. -->
<Field Name="VariableAccess" Type="BoundExpression?"/>
<Field Name="IsExplicitNotNullTest" Type="bool"/>
</Node>

<Node Name="BoundITuplePattern" Base="BoundPattern">
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs
View file
Open in desktop
Expand Up @@ -3401,7 +3401,7 @@ internal bool EmitNullablePublicOnly

internal bool ShouldEmitNullableAttributes(Symbol symbol)
{
Debug.Assert(symbol is object);
RoslynDebug.Assert(symbol is object);
Debug.Assert(symbol.IsDefinition);

if (symbol.ContainingModule != SourceModule)
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Compilation/MemberSemanticModel.cs
View file
Open in desktop
Expand Up @@ -696,7 +696,7 @@ private LocalFunctionSymbol GetDeclaredLocalFunction(LocalFunctionStatementSynta

if (_lazyRemappedSymbols.TryGetValue(originalSymbol, out Symbol remappedSymbol))
{
Debug.Assert(remappedSymbol is object);
RoslynDebug.Assert(remappedSymbol is object);
return (T)remappedSymbol;
}

Expand Down
6 changes: 3 additions & 3 deletions src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.SnapshotManager.cs
View file
Open in desktop
Expand Up @@ -141,8 +141,8 @@ internal void VerifyUpdatedSymbols()
{
var debugText = expr?.Syntax.ToFullString() ?? originalSymbol.ToDisplayString();
Debug.Assert((object)originalSymbol != updatedSymbol, $"Recorded exact same symbol for {debugText}");
Debug.Assert(originalSymbol is object, $"Recorded null original symbol for {debugText}");
Debug.Assert(updatedSymbol is object, $"Recorded null updated symbol for {debugText}");
RoslynDebug.Assert(originalSymbol is object, $"Recorded null original symbol for {debugText}");
RoslynDebug.Assert(updatedSymbol is object, $"Recorded null updated symbol for {debugText}");
Debug.Assert(AreCloseEnough(originalSymbol, updatedSymbol), @$"Symbol for `{debugText}` changed:
Was {originalSymbol.ToDisplayString(SymbolDisplayFormat.FullyQualifiedFormat)}
Now {updatedSymbol.ToDisplayString(SymbolDisplayFormat.FullyQualifiedFormat)}");
Expand Down Expand Up @@ -197,7 +197,7 @@ internal SnapshotManager ToManagerAndFree()

internal int EnterNewWalker(Symbol symbol)
{
Debug.Assert(symbol is object);
RoslynDebug.Assert(symbol is object);
var previousSlot = _currentWalkerSlot;

// Because we potentially run multiple passes, we
Expand Down
19 changes: 10 additions & 9 deletions src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs
View file
Open in desktop
Expand Up @@ -7483,19 +7483,19 @@ public override BoundNode VisitIsOperator(BoundIsOperator node)
Debug.Assert(!this.IsConditionalState);

var operand = node.Operand;
var typeExpr = node.TargetType;

var result = base.VisitIsOperator(node);
Debug.Assert(node.Type.SpecialType == SpecialType.System_Boolean);

var slotBuilder = ArrayBuilder<int>.GetInstance();
GetSlotsToMarkAsNotNullable(operand, slotBuilder);
if (slotBuilder.Count > 0)
Split();
LearnFromNonNullTest(operand, ref StateWhenTrue);
if (typeExpr.Type?.SpecialType == SpecialType.System_Object)
{
Split();
MarkSlotsAsNotNull(slotBuilder, ref StateWhenTrue);
LearnFromNullTest(operand, ref StateWhenFalse);
}
slotBuilder.Free();

VisitTypeExpression(node.TargetType);
VisitTypeExpression(typeExpr);
SetNotNullResult(node);
return result;
}
Expand Down Expand Up @@ -8216,7 +8216,8 @@ private sealed class ExpressionAndSymbolEqualityComparer : IEqualityComparer<(Bo

public bool Equals((BoundNode? expr, Symbol sym) x, (BoundNode? expr, Symbol sym) y)
{
Debug.Assert(x.sym is object && y.sym is object);
RoslynDebug.Assert(x.sym is object);
RoslynDebug.Assert(y.sym is object);

// We specifically use reference equality for the symbols here because the BoundNode should be immutable.
// We should be storing and retrieving the exact same instance of the symbol, not just an "equivalent"
Expand All @@ -8226,7 +8227,7 @@ public bool Equals((BoundNode? expr, Symbol sym) x, (BoundNode? expr, Symbol sym

public int GetHashCode((BoundNode? expr, Symbol sym) obj)
{
Debug.Assert(obj.sym is object);
RoslynDebug.Assert(obj.sym is object);
return Hash.Combine(obj.expr, obj.sym.GetHashCode());
}
}
Expand Down
10 changes: 9 additions & 1 deletion src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker_Patterns.cs
View file
Open in desktop
Expand Up @@ -79,7 +79,6 @@ public override BoundNode VisitITuplePattern(BoundITuplePattern node)
/// </summary>
/// <param name="inputType">Type type of the input expression (before nullable analysis).
/// Used to determine which types can contain null.</param>
/// <returns>true if there is a top-level explicit null check</returns>
private void LearnFromAnyNullPatterns(
Copy link
Member

@gafter gafter Jan 10, 2020 •
edited by cston

Choose a reason for hiding this comment

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

LearnFromAnyNullPatterns [](start = 21, length = 24)

This will have to be modified to look for bound patterns that are explicit non-null tests. Probably adjust the name of this method as well. #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.

Added.

In reply to: 365461994 [](ancestors = 365461994)

int inputSlot,
TypeSymbol inputType,
Expand Down Expand Up @@ -109,6 +108,11 @@ public override BoundNode VisitITuplePattern(BoundITuplePattern node)
break; // nothing to learn
case BoundRecursivePattern rp:
{
if (rp.IsExplicitNotNullTest)
{
LearnFromNullTest(inputSlot, inputType, ref this.State, markDependentSlotsNotNull: false);
}

// for positional part: we only learn from tuples (not Deconstruct)
if (rp.DeconstructMethod is null && !rp.Deconstruction.IsDefault)
{
Expand Down Expand Up @@ -352,6 +356,10 @@ protected override void VisitSwitchSection(BoundSwitchSection node, bool isLastS
if (inputSlot > 0)
{
MarkDependentSlotsNotNull(inputSlot, inputType, ref this.StateWhenFalse);
if (t.IsExplicitTest)
{
LearnFromNullTest(inputSlot, inputType, ref this.StateWhenFalse, markDependentSlotsNotNull: false);
}
learnFromNonNullTest(inputSlot, ref this.StateWhenTrue);
}
gotoNode(p.WhenTrue, this.StateWhenTrue, nodeBelievedReachable);
Expand Down