Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 6 additions & 0 deletions docs/features/nullable-reference-types.md
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,12 @@ If the analysis determines that a null check always (or never) passes, a hidden
A number of null checks affect the flow state when tested for:
- comparisons to `null`: `x == null` and `x != null`
- `is` operator: `x is null`, `x is K` (where `K` is a constant), `x is string`, `x is string s`
- calls to well-known equality methods, including:
- `static bool object.Equals(object, object)`
- `static bool object.ReferenceEquals(object, object)`
- `bool object.Equals(object)` and overrides
- `bool IEquatable<T>(T)` and implementations
- `bool IEqualityComparer<T>(T, T)` and implementations

Invocation of methods annotated with the following attributes will also affect flow analysis:
- simple pre-conditions: `[AllowNull]` and `[DisallowNull]`
Expand Down
107 changes: 103 additions & 4 deletions src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2164,8 +2164,6 @@ protected override void AfterLeftChildHasBeenVisited(BoundBinaryOperator binary)

if (operandComparedToNull != null)
{
operandComparedToNull = SkipReferenceConversions(operandComparedToNull);
Copy link
Member Author

@RikkiGibson RikkiGibson Jun 26, 2019

Choose a reason for hiding this comment

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

operandComparedToNull = SkipReferenceConversions(operandComparedToNull); [](start = 20, length = 72)

FYI @jcouv I found removing this conversion-stripping step did not affect anything. #Closed


// Set all nested conditional slots. For example in a?.b?.c we'll set a, b, and c.
bool nonNullCase = op != BinaryOperatorKind.Equal; // true represents WhenTrue
splitAndLearnFromNonNullTest(operandComparedToNull, whenTrue: nonNullCase);
Expand Down Expand Up @@ -2813,8 +2811,11 @@ private void ReinferMethodAndVisitArguments(BoundCall node, TypeWithState receiv
method = (MethodSymbol)AsMemberOfType(receiverType.Type, method);
}

method = VisitArguments(node, node.Arguments, refKindsOpt, method.Parameters, node.ArgsToParamsOpt,
node.Expanded, node.InvokedAsExtensionMethod, method).method;
ImmutableArray<VisitArgumentResult> results;
(method, results) = VisitArguments(node, node.Arguments, refKindsOpt, method.Parameters, node.ArgsToParamsOpt,
node.Expanded, node.InvokedAsExtensionMethod, method);

LearnFromEqualsMethod(method, node, receiverType, results);

if (method.MethodKind == MethodKind.LocalFunction)
{
Expand All @@ -2825,6 +2826,104 @@ private void ReinferMethodAndVisitArguments(BoundCall node, TypeWithState receiv
SetResult(node, GetReturnTypeWithState(method), method.ReturnTypeWithAnnotations);
}

private void LearnFromEqualsMethod(MethodSymbol method, BoundCall node, TypeWithState receiverType, ImmutableArray<VisitArgumentResult> results)
{
// easy out
var parameterCount = method.ParameterCount;
if ((parameterCount != 1 && parameterCount != 2)
|| method.MethodKind != MethodKind.Ordinary
|| method.ReturnType.SpecialType != SpecialType.System_Boolean
|| (method.Name != SpecialMembers.GetDescriptor(SpecialMember.System_Object__Equals).Name
&& method.Name != SpecialMembers.GetDescriptor(SpecialMember.System_Object__ReferenceEquals).Name))
{
return;
}

var arguments = node.Arguments;

var isStaticEqualsMethod = method.Equals(compilation.GetSpecialTypeMember(SpecialMember.System_Object__EqualsObjectObject))
|| method.Equals(compilation.GetSpecialTypeMember(SpecialMember.System_Object__ReferenceEquals));
if (isStaticEqualsMethod ||
isWellKnownEqualityMethodOrImplementation(compilation, method, WellKnownMember.System_Collections_Generic_IEqualityComparer_T__Equals))
{
Debug.Assert(arguments.Length == 2);
learnFromEqualsMethodArguments(arguments[0], results[0].RValueType, arguments[1], results[1].RValueType);
return;
}

var isObjectEqualsMethodOrOverride = method.GetLeastOverriddenMethod(accessingTypeOpt: null)
.Equals(compilation.GetSpecialTypeMember(SpecialMember.System_Object__Equals));
if (isObjectEqualsMethodOrOverride ||
isWellKnownEqualityMethodOrImplementation(compilation, method, WellKnownMember.System_IEquatable_T__Equals))
{
Debug.Assert(arguments.Length == 1);
learnFromEqualsMethodArguments(node.ReceiverOpt, receiverType, arguments[0], results[0].RValueType);
return;
}

static bool isWellKnownEqualityMethodOrImplementation(CSharpCompilation compilation, MethodSymbol method, WellKnownMember wellKnownMember)
{
var wellKnownMethod = compilation.GetWellKnownTypeMember(wellKnownMember);
if (wellKnownMethod is null)
{
return false;
}

var wellKnownType = wellKnownMethod.ContainingType;
var parameterType = method.Parameters[0].TypeWithAnnotations;
var constructedType = wellKnownType.Construct(ImmutableArray.Create(parameterType));

Symbol constructedMethod = null;
foreach (var member in constructedType.GetMembers(WellKnownMemberNames.ObjectEquals))
{
if (member.OriginalDefinition.Equals(wellKnownMethod))
{
constructedMethod = member;
break;
}
}

Debug.Assert(constructedMethod != null, "the original definition is present but the constructed method isn't present");
Copy link
Member

@333fred 333fred Jun 28, 2019

Choose a reason for hiding this comment

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

Nit: empty line below #Resolved


// FindImplementationForInterfaceMember doesn't check if this method is itself the interface method we're looking for
if (constructedMethod.Equals(method))
{
return true;
}

var implementationMethod = method.ContainingType.FindImplementationForInterfaceMember(constructedMethod);
return method.Equals(implementationMethod);
}

void learnFromEqualsMethodArguments(BoundExpression left, TypeWithState leftType, BoundExpression right, TypeWithState rightType)
{
// comparing anything to a null literal gives maybe-null when true and not-null when false
// comparing a maybe-null to a not-null gives us not-null when true, nothing learned when false
if (left.ConstantValue?.IsNull == true)
{
Split();
LearnFromNullTest(right, ref StateWhenTrue);
LearnFromNonNullTest(right, ref StateWhenFalse);
}
else if (right.ConstantValue?.IsNull == true)
{
Split();
LearnFromNullTest(left, ref StateWhenTrue);
LearnFromNonNullTest(left, ref StateWhenFalse);
}
else if (leftType.MayBeNull && rightType.IsNotNull)
{
Split();
LearnFromNonNullTest(left, ref StateWhenTrue);
}
else if (rightType.MayBeNull && leftType.IsNotNull)
{
Split();
LearnFromNonNullTest(right, ref StateWhenTrue);
}
}
}

private TypeWithState VisitCallReceiver(BoundCall node)
{
var receiverOpt = node.ReceiverOpt;
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Symbols/TypeSymbol.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1705,7 +1705,7 @@ private static Location GetInterfaceLocation(Symbol interfaceMember, TypeSymbol
snt = implementingType as SourceMemberContainerTypeSymbol;
}

return snt?.GetImplementsLocation(@interface) ?? implementingType.Locations[0];
return snt?.GetImplementsLocation(@interface) ?? implementingType.Locations.FirstOrNone();
Copy link
Member

@333fred 333fred Jun 28, 2019

Choose a reason for hiding this comment

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

FirstOrNone [](start = 88, length = 11)

Is this Linq? #Resolved

Copy link
Member

@jcouv jcouv Jun 28, 2019

Choose a reason for hiding this comment

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

Extension:

        internal static Location FirstOrNone(this ImmutableArray<Location> items)
        {
            return items.IsEmpty ? Location.None : items[0];
        }
``` #Resolved

}

private static bool ReportAnyMismatchedConstraints(MethodSymbol interfaceMethod, TypeSymbol implementingType, MethodSymbol implicitImpl, DiagnosticBag diagnostics)
Expand Down
Loading