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

Fix nullability check in override/implementation #49723

Merged
merged 12 commits into from
Dec 5, 2020
Merged

Fix nullability check in override/implementation #49723

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
c3db802
Fix nullability check in override/implementation
jcouv Dec 1, 2020
62313f7
Fix partials scenario
jcouv Dec 2, 2020
8940d30
Use Ignored for indexed type parameters
jcouv Dec 3, 2020
d5617ae
comment
jcouv Dec 3, 2020
e71faba
assertion
jcouv Dec 3, 2020
3bd5caa
Fix CompilationContext
jcouv Dec 4, 2020
147c3e0
rename
jcouv Dec 4, 2020
322f89a
comments
jcouv Dec 4, 2020
dbfb16b
Use ConstructedFrom for diagnostics with Ignored annotations in symbols
jcouv Dec 4, 2020
df7ea75
Remove unnecessary ConstructedFrom
jcouv Dec 4, 2020
b71ca77
Check GetHashCode
jcouv Dec 4, 2020
28608b6
static
jcouv Dec 4, 2020
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
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Emitter/EditAndContinue/CSharpSymbolMatcher.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -743,7 +743,7 @@ private static MethodSymbol SubstituteTypeParameters(MethodSymbol method)
return method;
}

return method.Construct(IndexedTypeParameterSymbol.Take(n).Cast<TypeParameterSymbol, TypeSymbol>());
return method.Construct(TypeMap.TypeParametersAsTypeSymbolsWithIgnoredAnnotations(IndexedTypeParameterSymbol.Take(n)));
}

private bool AreNamedTypesEqual(NamedTypeSymbol type, NamedTypeSymbol other)
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Symbols/MemberSignatureComparer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -536,7 +536,7 @@ private static TypeMap GetTypeMap(Symbol member)
null :
new TypeMap(
typeParameters,
IndexedTypeParameterSymbol.Take(member.GetMemberArity()),
TypeMap.TypeParametersAsTypeSymbolsWithIgnoredAnnotations(IndexedTypeParameterSymbol.Take(member.GetMemberArity())),
true);
}

Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Symbols/Metadata/PE/MemberRefMetadataDecoder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,7 @@ private static bool MethodSymbolMatchesParamInfo(MethodSymbol candidateMethod, P
// so we'll cheat and use it here for comparison purposes.
TypeMap candidateMethodTypeMap = new TypeMap(
candidateMethod.TypeParameters,
IndexedTypeParameterSymbol.Take(candidateMethod.Arity), true);
TypeMap.TypeParametersAsTypeSymbolsWithIgnoredAnnotations(IndexedTypeParameterSymbol.Take(candidateMethod.Arity)), true);

if (!ReturnTypesMatch(candidateMethod, candidateMethodTypeMap, ref targetParamInfo[0]))
{
Expand Down
4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PENamedTypeSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2505,8 +2505,8 @@ private bool MatchesContainingTypeParameters()
// containing symbol for the temporary type is the namespace directly.
var nestedType = Create(this.ContainingPEModule, (PENamespaceSymbol)this.ContainingNamespace, _handle, null);
var nestedTypeParameters = nestedType.TypeParameters;
var containingTypeMap = new TypeMap(containingTypeParameters, IndexedTypeParameterSymbol.Take(n), allowAlpha: false);
var nestedTypeMap = new TypeMap(nestedTypeParameters, IndexedTypeParameterSymbol.Take(nestedTypeParameters.Length), allowAlpha: false);
var containingTypeMap = new TypeMap(containingTypeParameters, TypeMap.TypeParametersAsTypeSymbolsWithIgnoredAnnotations(IndexedTypeParameterSymbol.Take(n)), allowAlpha: false);
var nestedTypeMap = new TypeMap(nestedTypeParameters, TypeMap.TypeParametersAsTypeSymbolsWithIgnoredAnnotations(IndexedTypeParameterSymbol.Take(nestedTypeParameters.Length)), allowAlpha: false);

for (int i = 0; i < n; i++)
{
Expand Down
5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/Symbols/NullableAnnotation.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,5 +32,10 @@ internal enum NullableAnnotation : byte
/// Type is annotated with '?' - string?, T?.
/// </summary>
Annotated,

/// <summary>
/// Used for indexed type parameters and used locally in override/implementation checks.
Copy link
Contributor

@AlekseyTs AlekseyTs Dec 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Used for indexed type parameters and used locally in override/implementation checks. [](start = 12, length = 84)

Should probably provide details about how exactly it is ignored. So that one could predict the impact of usage of this annotation #Closed

/// </summary>
Ignored,
}
}
44 changes: 34 additions & 10 deletions src/Compilers/CSharp/Portable/Symbols/NullableAnnotationExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#nullable disable

using System.Collections.Immutable;
using System.Diagnostics;
using Microsoft.CodeAnalysis.CSharp.Symbols;
using Roslyn.Utilities;

Expand All @@ -25,37 +26,55 @@ internal static class NullableAnnotationExtensions
/// Join nullable annotations from the set of lower bounds for fixing a type parameter.
/// This uses the covariant merging rules. (Annotated wins over Oblivious which wins over NotAnnotated)
/// </summary>
public static NullableAnnotation Join(this NullableAnnotation a, NullableAnnotation b) => (a < b) ? b : a;
public static NullableAnnotation Join(this NullableAnnotation a, NullableAnnotation b)
{
Debug.Assert(a != NullableAnnotation.Ignored);
Debug.Assert(b != NullableAnnotation.Ignored);
return (a < b) ? b : a;
}

/// <summary>
/// Meet two nullable annotations for computing the nullable annotation of a type parameter from upper bounds.
/// This uses the contravariant merging rules. (NotAnnotated wins over Oblivious which wins over Annotated)
/// </summary>
public static NullableAnnotation Meet(this NullableAnnotation a, NullableAnnotation b) => (a < b) ? a : b;
public static NullableAnnotation Meet(this NullableAnnotation a, NullableAnnotation b)
{
Debug.Assert(a != NullableAnnotation.Ignored);
Debug.Assert(b != NullableAnnotation.Ignored);
return (a < b) ? a : b;
}

/// <summary>
/// Return the nullable annotation to use when two annotations are expected to be "compatible", which means
/// they could be the same. These are the "invariant" merging rules. (NotAnnotated wins over Annotated which wins over Oblivious)
/// </summary>
public static NullableAnnotation EnsureCompatible(this NullableAnnotation a, NullableAnnotation b) =>
(a, b) switch
public static NullableAnnotation EnsureCompatible(this NullableAnnotation a, NullableAnnotation b)
{
Debug.Assert(a != NullableAnnotation.Ignored);
Debug.Assert(b != NullableAnnotation.Ignored);
return (a, b) switch
{
(NullableAnnotation.Oblivious, _) => b,
(_, NullableAnnotation.Oblivious) => a,
_ => a < b ? a : b,
};
}

/// <summary>
/// Merges nullability.
/// </summary>
public static NullableAnnotation MergeNullableAnnotation(this NullableAnnotation a, NullableAnnotation b, VarianceKind variance) =>
variance switch
public static NullableAnnotation MergeNullableAnnotation(this NullableAnnotation a, NullableAnnotation b, VarianceKind variance)
{
Debug.Assert(a != NullableAnnotation.Ignored);
Debug.Assert(b != NullableAnnotation.Ignored);
return variance switch
{
VarianceKind.In => a.Meet(b),
VarianceKind.Out => a.Join(b),
VarianceKind.None => a.EnsureCompatible(b),
_ => throw ExceptionUtilities.UnexpectedValue(variance)
};
}

/// <summary>
/// The attribute (metadata) representation of <see cref="NullableAnnotation.NotAnnotated"/>.
Expand Down Expand Up @@ -108,13 +127,18 @@ internal static CodeAnalysis.NullableAnnotation ToPublicAnnotation(this TypeWith
internal static CodeAnalysis.NullableAnnotation ToPublicAnnotation(TypeSymbol type, NullableAnnotation annotation) =>
annotation switch
{
CSharp.NullableAnnotation.Annotated => CodeAnalysis.NullableAnnotation.Annotated,
CSharp.NullableAnnotation.NotAnnotated => CodeAnalysis.NullableAnnotation.NotAnnotated,
NullableAnnotation.Annotated => CodeAnalysis.NullableAnnotation.Annotated,
NullableAnnotation.NotAnnotated => CodeAnalysis.NullableAnnotation.NotAnnotated,

// A value type may be oblivious or not annotated depending on whether the type reference
// is from source or metadata. (Binding using the #nullable context only when setting the annotation
// to avoid checking IsValueType early.) The annotation is normalized here in the public API.
CSharp.NullableAnnotation.Oblivious when type.IsValueType => CodeAnalysis.NullableAnnotation.NotAnnotated,
CSharp.NullableAnnotation.Oblivious => CodeAnalysis.NullableAnnotation.None,
NullableAnnotation.Oblivious when type.IsValueType => CodeAnalysis.NullableAnnotation.NotAnnotated,
NullableAnnotation.Oblivious => CodeAnalysis.NullableAnnotation.None,

// May occur as type argument in symbols in diagnostic
Copy link
Contributor

@AlekseyTs AlekseyTs Dec 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

// May occur as type argument in symbols in diagnostic [](start = 16, length = 54)

I think we should be able to avoid this if we use ConstructedFrom for diagnostic instead. #Closed

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should really make an effort to not leak a symbol with an annotation like that. Otherwise, equality is likely to misbehave, etc.

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

NullableAnnotation.Ignored when type.IsTypeParameter() => CodeAnalysis.NullableAnnotation.None,

_ => throw ExceptionUtilities.UnexpectedValue(annotation)
};

Expand Down
2 changes: 1 addition & 1 deletion ...pilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol_ImplementationChecks.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -944,7 +944,7 @@ void checkSingleOverriddenMember(Symbol overridingMember, Symbol overriddenMembe

if (overridingMethod.IsGenericMethod)
{
overriddenMethod = overriddenMethod.Construct(overridingMethod.TypeArgumentsWithAnnotations);
overriddenMethod = overriddenMethod.Construct(TypeMap.TypeParametersAsTypeSymbolsWithIgnoredAnnotations(overridingMethod.TypeParameters));
}

// Check for mismatched byref returns and return type. Ignore custom modifiers, because this diagnostic is based on the C# semantics.
Expand Down
4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/Symbols/Source/SourceOrdinaryMethodSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -632,7 +632,7 @@ private static void PartialMethodChecks(SourceOrdinaryMethodSymbol definition, S
{
Debug.Assert(!ReferenceEquals(definition, implementation));

MethodSymbol constructedDefinition = definition.ConstructIfGeneric(implementation.TypeArgumentsWithAnnotations);
MethodSymbol constructedDefinition = definition.ConstructIfGeneric(TypeMap.TypeParametersAsTypeSymbolsWithIgnoredAnnotations(implementation.TypeParameters));
bool returnTypesEqual = constructedDefinition.ReturnTypeWithAnnotations.Equals(implementation.ReturnTypeWithAnnotations, TypeCompareKind.AllIgnoreOptions);
if (!returnTypesEqual
&& !SourceMemberContainerTypeSymbol.IsOrContainsErrorType(implementation.ReturnType)
Expand Down Expand Up @@ -721,7 +721,7 @@ private static void PartialMethodConstraintsChecks(SourceOrdinaryMethodSymbol de
}

var typeParameters2 = implementation.TypeParameters;
var indexedTypeParameters = IndexedTypeParameterSymbol.Take(arity);
var indexedTypeParameters = TypeMap.TypeParametersAsTypeSymbolsWithIgnoredAnnotations(IndexedTypeParameterSymbol.Take(arity));
Copy link
Contributor

@AlekseyTs AlekseyTs Dec 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

TypeParametersAsTypeSymbolsWithIgnoredAnnotations [](start = 48, length = 49)

It looks like we are missing a change in CompilationContext in EE #Closed

Copy link
Contributor

@AlekseyTs AlekseyTs Dec 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Take [](start = 125, length = 4)

Consider changing what this method returns instead. It looks like majority of call sites actually need TypeWithAnnotations. For the case of just type parameters, we can add a special helper. #Closed

Copy link
Contributor

Choose a reason for hiding this comment

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

I was intentionally suggesting to keep the same name. There are often overloads that can handle arrays of both types, however we want default case to be TypeWithAnnotations with an ignored annotation. This way, every consumer of this method, existing or added later will be automatically guided into the right direction and will not require changes. Changes will be required only for exceptions, which aren't that many.

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

var typeMap1 = new TypeMap(typeParameters1, indexedTypeParameters, allowAlpha: true);
var typeMap2 = new TypeMap(typeParameters2, indexedTypeParameters, allowAlpha: true);

Expand Down
5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/Symbols/TypeMap.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,11 @@ internal static ImmutableArray<TypeWithAnnotations> TypeParametersAsTypeSymbolsW
return typeParameters.SelectAsArray((tp) => TypeWithAnnotations.Create(tp));
}

internal static ImmutableArray<TypeWithAnnotations> TypeParametersAsTypeSymbolsWithIgnoredAnnotations(ImmutableArray<TypeParameterSymbol> typeParameters)
{
return typeParameters.SelectAsArray((tp) => TypeWithAnnotations.Create(tp, NullableAnnotation.Ignored));
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: static

}

internal static ImmutableArray<TypeSymbol> AsTypeSymbols(ImmutableArray<TypeWithAnnotations> typesOpt)
{
return typesOpt.IsDefault ? default : typesOpt.SelectAsArray(AsTypeSymbol);
Expand Down
4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/Symbols/TypeSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1770,7 +1770,7 @@ internal static void CheckNullableReferenceTypeMismatchOnImplementingMember(Type

if (implementedMethod.IsGenericMethod)
{
implementedMethod = implementedMethod.Construct(implementingMethod.TypeArgumentsWithAnnotations);
implementedMethod = implementedMethod.Construct(TypeMap.TypeParametersAsTypeSymbolsWithIgnoredAnnotations(implementingMethod.TypeParameters));
}

SourceMemberContainerTypeSymbol.CheckValidNullableMethodOverride(
Copy link
Contributor

@AlekseyTs AlekseyTs Dec 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It feels like the code in this function should also dig to constructed from as appropriate. #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

The lambdas that are passed in do use ConstrcutedFrom (actually, they don't need to dig into derived/implementating method, I was hasty and will revert that part). The rest of the method is fine (see earlier comment).

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

Expand Down Expand Up @@ -1906,7 +1906,7 @@ private static bool ReportAnyMismatchedConstraints(MethodSymbol interfaceMethod,
{
var typeParameters1 = interfaceMethod.TypeParameters;
var typeParameters2 = implicitImpl.TypeParameters;
var indexedTypeParameters = IndexedTypeParameterSymbol.Take(arity);
var indexedTypeParameters = TypeMap.TypeParametersAsTypeSymbolsWithIgnoredAnnotations(IndexedTypeParameterSymbol.Take(arity));

var typeMap1 = new TypeMap(typeParameters1, indexedTypeParameters, allowAlpha: true);
var typeMap2 = new TypeMap(typeParameters2, indexedTypeParameters, allowAlpha: true);
Expand Down
16 changes: 7 additions & 9 deletions src/Compilers/CSharp/Portable/Symbols/TypeWithAnnotations.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,11 +161,6 @@ internal bool CanBeAssignedNull
}
}

private static bool IsIndexedTypeParameter(TypeSymbol typeSymbol)
{
return typeSymbol is IndexedTypeParameterSymbol;
}

private static TypeWithAnnotations CreateNonLazyType(TypeSymbol typeSymbol, NullableAnnotation nullableAnnotation, ImmutableArray<CustomModifier> customModifiers)
{
return new TypeWithAnnotations(typeSymbol, nullableAnnotation, Extensions.Create(customModifiers));
Expand Down Expand Up @@ -429,6 +424,9 @@ public TypeWithAnnotations SubstituteType(AbstractTypeMap typeMap) =>

internal TypeWithAnnotations SubstituteTypeCore(AbstractTypeMap typeMap)
{
// Ignored may only appear on a replacement type and will not survive the substitution (ie. the original annotation wins over Ignored)
Debug.Assert(NullableAnnotation != NullableAnnotation.Ignored);

var newCustomModifiers = typeMap.SubstituteCustomModifiers(this.CustomModifiers);
TypeSymbol typeSymbol = this.Type;
var newTypeWithModifiers = typeMap.SubstituteType(typeSymbol);
Expand All @@ -437,6 +435,7 @@ internal TypeWithAnnotations SubstituteTypeCore(AbstractTypeMap typeMap)
{
Debug.Assert(newTypeWithModifiers.NullableAnnotation.IsOblivious() || (typeSymbol.IsNullableType() && newTypeWithModifiers.NullableAnnotation.IsAnnotated()));
Debug.Assert(newTypeWithModifiers.CustomModifiers.IsEmpty);
Debug.Assert(NullableAnnotation != NullableAnnotation.Ignored);

if (typeSymbol.Equals(newTypeWithModifiers.Type, TypeCompareKind.ConsiderEverything) &&
newCustomModifiers == CustomModifiers)
Expand All @@ -457,7 +456,7 @@ internal TypeWithAnnotations SubstituteTypeCore(AbstractTypeMap typeMap)
{
return this; // substitution had no effect on the type or modifiers
}
else if (Is((TypeParameterSymbol)typeSymbol))
else if (Is((TypeParameterSymbol)typeSymbol) && newTypeWithModifiers.NullableAnnotation != NullableAnnotation.Ignored)
{
return newTypeWithModifiers;
}
Expand All @@ -468,14 +467,13 @@ internal TypeWithAnnotations SubstituteTypeCore(AbstractTypeMap typeMap)
}

NullableAnnotation newAnnotation;

Debug.Assert(!IsIndexedTypeParameter(newTypeWithModifiers.Type) || newTypeWithModifiers.NullableAnnotation.IsOblivious());
Debug.Assert(newTypeWithModifiers.Type is not IndexedTypeParameterSymbol || newTypeWithModifiers.NullableAnnotation == NullableAnnotation.Ignored);

if (NullableAnnotation.IsAnnotated() || newTypeWithModifiers.NullableAnnotation.IsAnnotated())
{
newAnnotation = NullableAnnotation.Annotated;
}
else if (IsIndexedTypeParameter(newTypeWithModifiers.Type))
else if (newTypeWithModifiers.NullableAnnotation == NullableAnnotation.Ignored)
Copy link
Contributor

@AlekseyTs AlekseyTs Dec 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

newTypeWithModifiers.NullableAnnotation == NullableAnnotation.Ignore [](start = 21, length = 68)

It looks like we already have a special handling for indexed type parameters that has the same effect. If that is the case, I think we should reuse this mechanism rather than going through the trouble of dealing with the new annotation. We might need to use ConstructedFrom in diagnostics in affected components though. #Closed

Copy link
Member Author

@jcouv jcouv Dec 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

We can't use indexed type parameters because (1) they don't have names, and (2) they don't have constraints (so the logic in AreParameterAnnotationsCompatible which simulates an assignment produces wrong values/states in GetParameterState).
We could fix the first problem by introducing a new type parameter symbol. But the second problem is tougher, and I don't see a good way to solve it.
So using a new annotation seems a better fit to solve this problem. #Resolved

Copy link
Contributor

Choose a reason for hiding this comment

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

We can't use indexed type parameters because (1) they don't have names, and (2) they don't have constraints (so the logic in AreParameterAnnotationsCompatible which simulates an assignment produces wrong values/states in GetParameterState).

I would like to get a better understanding of these issues. Let's sync offline

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to remove the special handling for indexed type parameters and use the new annotation instead?

In reply to: 535509257 [](ancestors = 535509257,535501474)

Copy link
Member Author

@jcouv jcouv Dec 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Would it be possible to remove the special handling for indexed type parameters and use the new annotation instead?

Interesting idea. Thanks. Pushed a commit for that change #Resolved

{
newAnnotation = NullableAnnotation;
}
Expand Down
Loading