Skip to content

Commit

Permalink
Factor and tighten logic
Browse files Browse the repository at this point in the history
  • Loading branch information
@jcouv
jcouv committed Feb 3, 2020
1 parent bf2c8a3 commit f6c7427
Show file tree
Hide file tree
Showing 5 changed files with 194 additions and 150 deletions.
9 changes: 9 additions & 0 deletions src/Compilers/CSharp/Portable/Errors/ErrorFacts.cs
View file
Expand Up @@ -64,6 +64,15 @@ static ErrorFacts()
builder.Add(getId(ErrorCode.WRN_DuplicateInterfaceWithNullabilityMismatchInBaseList));
builder.Add(getId(ErrorCode.WRN_NullabilityMismatchInConstraintsOnPartialImplementation));
builder.Add(getId(ErrorCode.WRN_NullReferenceInitializer));
builder.Add(getId(ErrorCode.WRN_ShouldNotReturn));
builder.Add(getId(ErrorCode.WRN_DoesNotReturnMismatch));
builder.Add(getId(ErrorCode.WRN_ParameterConditionallyDisallowsNull));
builder.Add(getId(ErrorCode.WRN_NullabilityMismatchInParameterTypeOnExplicitImplementationBecauseOfAttributes));
builder.Add(getId(ErrorCode.WRN_NullabilityMismatchInParameterTypeOnImplicitImplementationBecauseOfAttributes));
builder.Add(getId(ErrorCode.WRN_NullabilityMismatchInParameterTypeOnOverrideBecauseOfAttributes));
builder.Add(getId(ErrorCode.WRN_NullabilityMismatchInReturnTypeOnExplicitImplementationBecauseOfAttributes));
builder.Add(getId(ErrorCode.WRN_NullabilityMismatchInReturnTypeOnImplicitImplementationBecauseOfAttributes));
builder.Add(getId(ErrorCode.WRN_NullabilityMismatchInReturnTypeOnOverrideBecauseOfAttributes));

NullableWarnings = builder.ToImmutable();

Expand Down
19 changes: 13 additions & 6 deletions src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs
View file
Expand Up @@ -1058,7 +1058,7 @@ private enum AssignmentKind
/// <summary>
/// Should we warn for assigning this state into this type?
/// </summary>
private static bool ShouldReportNullableAssignment(TypeWithAnnotations type, NullableFlowState state)
internal static bool ShouldReportNullableAssignment(TypeWithAnnotations type, NullableFlowState state)
{
if (!type.HasType ||
type.Type.IsValueType)
Expand Down Expand Up @@ -1451,7 +1451,7 @@ private void EnterParameter(ParameterSymbol parameter, TypeWithAnnotations param
}
}

private static TypeWithState GetParameterState(TypeWithAnnotations parameterType, FlowAnalysisAnnotations parameterAnnotations)
internal static TypeWithState GetParameterState(TypeWithAnnotations parameterType, FlowAnalysisAnnotations parameterAnnotations)
{
if ((parameterAnnotations & FlowAnalysisAnnotations.AllowNull) != 0)
{
Expand Down Expand Up @@ -3715,7 +3715,7 @@ private FlowAnalysisAnnotations GetParameterAnnotations(ParameterSymbol paramete
/// Fix a TypeWithAnnotations based on Allow/DisallowNull annotations prior to a conversion or assignment.
/// Note this does not work for nullable value types, so an additional check with <see cref="CheckDisallowedNullAssignment"/> may be required.
/// </summary>
private static TypeWithAnnotations ApplyLValueAnnotations(TypeWithAnnotations declaredType, FlowAnalysisAnnotations flowAnalysisAnnotations)
internal static TypeWithAnnotations ApplyLValueAnnotations(TypeWithAnnotations declaredType, FlowAnalysisAnnotations flowAnalysisAnnotations)
{
if ((flowAnalysisAnnotations & FlowAnalysisAnnotations.DisallowNull) == FlowAnalysisAnnotations.DisallowNull)
{
Expand All @@ -3732,7 +3732,7 @@ private static TypeWithAnnotations ApplyLValueAnnotations(TypeWithAnnotations de
/// <summary>
/// Update the null-state based on MaybeNull/NotNull
/// </summary>
private static TypeWithState ApplyUnconditionalAnnotations(TypeWithState typeWithState, FlowAnalysisAnnotations annotations)
internal static TypeWithState ApplyUnconditionalAnnotations(TypeWithState typeWithState, FlowAnalysisAnnotations annotations)
{
if ((annotations & FlowAnalysisAnnotations.MaybeNull) == FlowAnalysisAnnotations.MaybeNull)
{
Expand Down Expand Up @@ -4153,10 +4153,17 @@ private void CheckDisallowedNullAssignment(TypeWithState state, FlowAnalysisAnno
}

// We do this extra check for types whose non-nullable version cannot be represented
if (((annotations & FlowAnalysisAnnotations.DisallowNull) != 0) && hasNoNonNullableCounterpart(state.Type) && state.MayBeNull)
if (IsDisallowedNullAssignment(state, annotations))
{
ReportDiagnostic(ErrorCode.WRN_DisallowNullAttributeForbidsMaybeNullAssignment, location);
}
}

static internal bool IsDisallowedNullAssignment(TypeWithState valueState, FlowAnalysisAnnotations targetAnnotations)
{
return ((targetAnnotations & FlowAnalysisAnnotations.DisallowNull) != 0) &&
hasNoNonNullableCounterpart(valueState.Type) &&
valueState.MayBeNull;

static bool hasNoNonNullableCounterpart(TypeSymbol type)
{
Expand Down Expand Up @@ -6350,7 +6357,7 @@ static FlowAnalysisAnnotations getPropertyAnnotations(SourcePropertySymbol prope
}
}

private static FlowAnalysisAnnotations ToInwardAnnotations(FlowAnalysisAnnotations outwardAnnotations)
internal static FlowAnalysisAnnotations ToInwardAnnotations(FlowAnalysisAnnotations outwardAnnotations)
{
var annotations = FlowAnalysisAnnotations.None;
if ((outwardAnnotations & FlowAnalysisAnnotations.MaybeNull) != 0)
Expand Down
115 changes: 72 additions & 43 deletions ...pilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol_ImplementationChecks.cs
View file
Expand Up @@ -1136,8 +1136,7 @@ static void reportBadParameter(DiagnosticBag diagnostics, MethodSymbol overridde
{
reportMismatchInParameterType(diagnostics, overriddenMethod, overridingMethod, parameter, false, extraArgument);
}

if (reportMismatchInParameterType != null &&
else if (reportMismatchInParameterType != null &&
!areParameterAnnotationsCompatible(
parameter.RefKind,
overriddenParameterType,
Expand Down Expand Up @@ -1169,72 +1168,102 @@ static void reportBadParameter(DiagnosticBag diagnostics, MethodSymbol overridde

if (refKind == RefKind.None || refKind == RefKind.In)
{
bool overridingHasDisallowNull = (overridingAnnotations & FlowAnalysisAnnotations.DisallowNull) != 0;
bool overriddenHasDisallowNull = (overriddenAnnotations & FlowAnalysisAnnotations.DisallowNull) != 0;
if (overridingHasDisallowNull && !overriddenHasDisallowNull && overriddenType.GetValueNullableAnnotation() == NullableAnnotation.Annotated)
// pre-condition attributes
// Check whether we can assign a value from overridden parameter to overriding
var valueState = NullableWalker.GetParameterState(overriddenType, overriddenAnnotations);
if (isBadAssignment(valueState, overridingType, overridingAnnotations))
{
// Can't assign from overriding to overridden
return false;
}

bool overridingHasAllowNull = (overridingAnnotations & FlowAnalysisAnnotations.AllowNull) != 0;
bool overriddenHasAllowNull = (overriddenAnnotations & FlowAnalysisAnnotations.AllowNull) != 0;
if (overriddenHasAllowNull && !overridingHasAllowNull && !overridingType.CanBeAssignedNull)
// unconditional post-condition attributes
bool overridingHasNotNull = (overridingAnnotations & FlowAnalysisAnnotations.NotNull) == FlowAnalysisAnnotations.NotNull;
bool overriddenHasNotNull = (overriddenAnnotations & FlowAnalysisAnnotations.NotNull) == FlowAnalysisAnnotations.NotNull;
if (overriddenHasNotNull && !overridingHasNotNull && (refKind == RefKind.None || refKind == RefKind.In) && !forRef)
{
// Can't assign from overridden to overriding
// Overriding doesn't conform to contract of overridden (ie. promise not to return if parameter is null)
return false;
}
}

bool overridingHasNotNull = (overridingAnnotations & FlowAnalysisAnnotations.NotNull) == FlowAnalysisAnnotations.NotNull;
bool overriddenHasNotNull = (overriddenAnnotations & FlowAnalysisAnnotations.NotNull) == FlowAnalysisAnnotations.NotNull;
if (overriddenHasNotNull && !overridingHasNotNull && (refKind == RefKind.None || refKind == RefKind.In) && !forRef)
{
// Overriding doesn't conform to contract of overridden (ie. promise not to return if parameter is null)
return false;
bool overridingHasMaybeNull = (overridingAnnotations & FlowAnalysisAnnotations.MaybeNull) == FlowAnalysisAnnotations.MaybeNull;
bool overriddenHasMaybeNull = (overriddenAnnotations & FlowAnalysisAnnotations.MaybeNull) == FlowAnalysisAnnotations.MaybeNull;
if (overriddenHasMaybeNull && !overridingHasMaybeNull && (refKind == RefKind.None || refKind == RefKind.In) && !forRef)
{
// Overriding doesn't conform to contract of overridden (ie. promise to only return if parameter is null)
return false;
}
}

bool overridingHasNotNullWhenTrue = (overridingAnnotations & FlowAnalysisAnnotations.NotNullWhenTrue) != 0;
bool overriddenHasNotNullWhenTrue = (overriddenAnnotations & FlowAnalysisAnnotations.NotNullWhenTrue) != 0;
if (overriddenHasNotNullWhenTrue && !overridingHasNotNullWhenTrue && refKind == RefKind.Out && overridingType.GetValueNullableAnnotation() == NullableAnnotation.Annotated)
// conditional post-condition attributes
if (refKind == RefKind.Out)
{
// Can't assign value from overriding to overridden in true case
return false;
// when true
var valueWhenTrue = NullableWalker.ApplyUnconditionalAnnotations(
overridingType.ToTypeWithState(),
makeUnconditionalAnnotation(overridingAnnotations, sense: true));

var destAnnotationsWhenTrue = NullableWalker.ToInwardAnnotations(makeUnconditionalAnnotation(overriddenAnnotations, sense: true));
if (isBadAssignment(valueWhenTrue, overriddenType, destAnnotationsWhenTrue))
{
// Can't assign value from overriding to overridden in true case
return false;
}

// when false
var valueWhenFalse = NullableWalker.ApplyUnconditionalAnnotations(
overridingType.ToTypeWithState(),
makeUnconditionalAnnotation(overridingAnnotations, sense: false));

var destAnnotationsWhenFalse = NullableWalker.ToInwardAnnotations(makeUnconditionalAnnotation(overriddenAnnotations, sense: false));
if (isBadAssignment(valueWhenFalse, overriddenType, destAnnotationsWhenFalse))
{
// Can't assign value from overriding to overridden in false case
return false;
}
}

bool overridingHasNotNullWhenFalse = (overridingAnnotations & FlowAnalysisAnnotations.NotNullWhenFalse) != 0;
bool overriddenHasNotNullWhenFalse = (overriddenAnnotations & FlowAnalysisAnnotations.NotNullWhenFalse) != 0;
if (overriddenHasNotNullWhenFalse && !overridingHasNotNullWhenFalse && refKind == RefKind.Out && overridingType.GetValueNullableAnnotation() == NullableAnnotation.Annotated)
return true;
}

static bool isBadAssignment(TypeWithState valueState, TypeWithAnnotations destinationType, FlowAnalysisAnnotations destinationAnnotations)
{
if (NullableWalker.ShouldReportNullableAssignment(
NullableWalker.ApplyLValueAnnotations(destinationType, destinationAnnotations),
valueState.State))
{
// Can't assign value from overriding to overridden in false case
return false;
return true;
}

bool overridingHasMaybeNull = (overridingAnnotations & FlowAnalysisAnnotations.MaybeNull) == FlowAnalysisAnnotations.MaybeNull;
bool overriddenHasMaybeNull = (overriddenAnnotations & FlowAnalysisAnnotations.MaybeNull) == FlowAnalysisAnnotations.MaybeNull;
if (overriddenHasMaybeNull && !overridingHasMaybeNull && (refKind == RefKind.None || refKind == RefKind.In) && !forRef)
if (NullableWalker.IsDisallowedNullAssignment(valueState, destinationAnnotations))
{
// Overriding doesn't conform to contract of overridden (ie. promise to only return if parameter is null)
return false;
return true;
}

bool overridingHasMaybeNullWhenTrue = (overridingAnnotations & FlowAnalysisAnnotations.MaybeNullWhenTrue) != 0;
bool overriddenHasMaybeNullWhenTrue = (overriddenAnnotations & FlowAnalysisAnnotations.MaybeNullWhenTrue) != 0;
if (overridingHasMaybeNullWhenTrue && !overriddenHasMaybeNullWhenTrue && refKind == RefKind.Out && !overriddenType.CanBeAssignedNull)
return false;
}

// Convert both conditional annotations to unconditional ones or nothing
static FlowAnalysisAnnotations makeUnconditionalAnnotation(FlowAnalysisAnnotations annotations, bool sense)
{
if (sense)
{
// Can't assign value from overriding to overridden in true case
return false;
var unconditionalAnnotationWhenTrue = makeUnconditionalAnnotationCore(annotations, FlowAnalysisAnnotations.NotNullWhenTrue, FlowAnalysisAnnotations.NotNull);
return makeUnconditionalAnnotationCore(unconditionalAnnotationWhenTrue, FlowAnalysisAnnotations.MaybeNullWhenTrue, FlowAnalysisAnnotations.MaybeNull);
}

bool overridingHasMaybeNullWhenFalse = (overridingAnnotations & FlowAnalysisAnnotations.MaybeNullWhenFalse) != 0;
bool overriddenHasMaybeNullWhenFalse = (overriddenAnnotations & FlowAnalysisAnnotations.MaybeNullWhenFalse) != 0;
if (overridingHasMaybeNullWhenFalse && !overriddenHasMaybeNullWhenFalse && refKind == RefKind.Out && !overriddenType.CanBeAssignedNull)
var unconditionalAnnotationWhenFalse = makeUnconditionalAnnotationCore(annotations, FlowAnalysisAnnotations.NotNullWhenFalse, FlowAnalysisAnnotations.NotNull);
return makeUnconditionalAnnotationCore(unconditionalAnnotationWhenFalse, FlowAnalysisAnnotations.MaybeNullWhenFalse, FlowAnalysisAnnotations.MaybeNull);
}

// Convert Maybe/NotNullWhen into Maybe/NotNull or nothing
static FlowAnalysisAnnotations makeUnconditionalAnnotationCore(FlowAnalysisAnnotations annotations, FlowAnalysisAnnotations conditionalAnnotation, FlowAnalysisAnnotations replacementAnnotation)
{
if ((annotations & conditionalAnnotation) != 0)
{
// Can't assign value from overriding to overridden in false case
return false;
return annotations | replacementAnnotation;
}

return true;
return annotations & ~replacementAnnotation;
}

static bool isValidNullableConversion(
Expand Down

0 comments on commit f6c7427

Please sign in to comment.