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 2 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
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
6 changes: 5 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 @@ -352,6 +351,11 @@ protected override void VisitSwitchSection(BoundSwitchSection node, bool isLastS
if (inputSlot > 0)
{
MarkDependentSlotsNotNull(inputSlot, inputType, ref this.StateWhenFalse);
if (t.Syntax is RecursivePatternSyntax { Type: null, Designation: null, PositionalPatternClause: null } syntax &&
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.

RecursivePatternSyntax [](start = 56, length = 22)

It is not correct to check the syntax. See https://github.com/dotnet/roslyn/blob/master/docs/compilers/Design/Bound%20Node%20Design.md#bound-nodes-should-capture-all-semantic-information-embedded-in-the-syntax

Instead this should be based on the node. It may be necessary to introduce a "BoundDagExplicitNonNullTest" just like we have an explicit and non-explicit null test. Alternatively, add a bool IsExplicit property to BoundDagNonNullTest. #Resolved

syntax.PropertyPatternClause?.Subpatterns.Count == 0)
{
LearnFromNullTest(inputSlot, inputType, ref this.StateWhenFalse, markDependentSlotsNotNull: false);
}
learnFromNonNullTest(inputSlot, ref this.StateWhenTrue);
}
gotoNode(p.WhenTrue, this.StateWhenTrue, nodeBelievedReachable);
Expand Down