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.

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

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

Merged
merged 11 commits into from Jan 14, 2020
Merged

Conversation

@cston
Copy link
Member

cston commented Jan 10, 2020

No description provided.

@cston cston requested a review from dotnet/roslyn-compiler as a code owner Jan 10, 2020
@cston cston added this to the 16.5.P3 milestone Jan 10, 2020
@@ -61355,6 +61355,197 @@ void F(C x)
);
}

[Fact]
public void OtherComparisonsAsPureNullTests_01()

This comment has been minimized.

Copy link
@jcouv

jcouv Jan 10, 2020

Member

Could you also test the object pattern in nested positions? { Property: object } for example #Resolved

This comment has been minimized.

Copy link
@gafter

gafter Jan 10, 2020

Member

object is not a pattern in C# 8.0. It is a pattern in C# 9.0. The closest thing in C# 8.0 is object _.


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

@@ -61355,6 +61355,197 @@ void F(C x)
);
}

[Fact]

This comment has been minimized.

Copy link
@jcouv

jcouv Jan 10, 2020

Member

nit Consider issue link or reference to LDM notes? #Resolved

@jcouv
jcouv approved these changes Jan 10, 2020
Copy link
Member

jcouv left a comment

LGTM Thanks (iteration 1) modulo bootstrap issue ;-)

@cston cston requested a review from dotnet/roslyn-ide as a code owner Jan 10, 2020
@jcouv
jcouv approved these changes Jan 10, 2020
Copy link
Member

jcouv left a comment

Still LGTM (iteration 2)

cston added 2 commits Jan 10, 2020
@@ -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 &&

This comment has been minimized.

Copy link
@gafter

gafter Jan 10, 2020

Member

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

@@ -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(

This comment has been minimized.

Copy link
@gafter

gafter Jan 10, 2020

Member

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

This comment has been minimized.

Copy link
@cston

cston Jan 12, 2020

Author Member

Added.


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

@gafter

This comment has been minimized.

Copy link
Member

gafter commented Jan 10, 2020

Finished reviewing iteration 4.

cston added 6 commits Jan 10, 2020
boundDeclType is null &&
properties.IsDefaultOrEmpty &&
deconstructMethod is null &&
deconstructionSubpatterns.IsDefaultOrEmpty;

This comment has been minimized.

Copy link
@gafter

gafter Jan 13, 2020

Member

deconstructionSubpatterns.IsDefaultOrEmpty [](start = 16, length = 42)

This appears to treat () and () {} as explicit null tests. They aren't. The deconstruction subpatterns need to be null and the properties need to be non-null and empty. Please test these cases to demonstrate whether or not they are considered pure tests. #Resolved

@@ -261,7 +261,8 @@ private static void ReportUseSiteAndObsoleteDiagnostics(CSharpSyntaxNode? syntax
/// </summary>
internal static void VerifyTupleTypePresent(int cardinality, CSharpSyntaxNode? syntax, CSharpCompilation compilation, DiagnosticBag diagnostics)
{
Debug.Assert(diagnostics is object && syntax is object);
RoslynDebug.Assert(diagnostics is object);

This comment has been minimized.

Copy link
@gafter

gafter Jan 13, 2020

Member

RoslynDebug [](start = 12, length = 11)

Did the assertions not work in the && form? If so, that is worrying. #Resolved

This comment has been minimized.

Copy link
@cston

cston Jan 13, 2020

Author Member

The assertions work with && as well but it seemed more useful to test the parts separately to narrow down any failure.


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

}
static void F2(E e)
{
if (e is Object) return;

This comment has been minimized.

Copy link
@gafter

gafter Jan 13, 2020

Member

Object [](start = 17, length = 6)

Please add a test for e is SomeName where SomeName is a using alias for System.Object.
Please add a test for e is dynamic. Because it is "equivalent" to object, I believe that should work as a pure test. #Resolved

This comment has been minimized.

Copy link
@cston

cston Jan 13, 2020

Author Member

Added.

e is dynamic is not treated as a pure null test.


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

@gafter

This comment has been minimized.

Copy link
Member

gafter commented Jan 13, 2020

Finished reviewing iteration 10.

@gafter
gafter approved these changes Jan 13, 2020
Copy link
Member

gafter left a comment

:shipit:

@cston cston merged commit 7102821 into dotnet:master Jan 14, 2020
18 checks passed
18 checks passed
WIP Ready for review
Details
license/cla All CLA requirements met.
Details
roslyn-CI Build #20200113.31 succeeded
Details
roslyn-CI (Linux_Test coreclr) Linux_Test coreclr succeeded
Details
roslyn-CI (SourceBuild_Test) SourceBuild_Test succeeded
Details
roslyn-CI (Windows_CoreClr_Unit_Tests debug) Windows_CoreClr_Unit_Tests debug succeeded
Details
roslyn-CI (Windows_CoreClr_Unit_Tests release) Windows_CoreClr_Unit_Tests release succeeded
Details
roslyn-CI (Windows_Correctness_Test) Windows_Correctness_Test succeeded
Details
roslyn-CI (Windows_Desktop_Spanish_Unit_Tests) Windows_Desktop_Spanish_Unit_Tests succeeded
Details
roslyn-CI (Windows_Desktop_Unit_Tests debug_32) Windows_Desktop_Unit_Tests debug_32 succeeded
Details
roslyn-CI (Windows_Desktop_Unit_Tests debug_64) Windows_Desktop_Unit_Tests debug_64 succeeded
Details
roslyn-CI (Windows_Desktop_Unit_Tests release_32) Windows_Desktop_Unit_Tests release_32 succeeded
Details
roslyn-CI (Windows_Desktop_Unit_Tests release_64) Windows_Desktop_Unit_Tests release_64 succeeded
Details
roslyn-CI (Windows_Determinism_Test) Windows_Determinism_Test succeeded
Details
roslyn-CI (macOS_Test) macOS_Test succeeded
Details
roslyn-integration-CI Build #20200113.31 succeeded
Details
roslyn-integration-CI (VS_Integration debug_async) VS_Integration debug_async succeeded
Details
roslyn-integration-CI (VS_Integration release_async) VS_Integration release_async succeeded
Details
@cston cston deleted the cston:is-object branch Jan 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.