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

Adjust type substitution for oblivious type argument in not-annotated type parameter #48694

Closed
wants to merge 7 commits into from
Closed

Adjust type substitution for oblivious type argument in not-annotated type parameter #48694

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5292354
Capture baseline for type substitution
jcouv Oct 14, 2020
b559d07
Change type substitution accounting for nullability
jcouv Oct 16, 2020
ab78bba
Fix binary operator scenario
jcouv Nov 10, 2020
58988a0
Avoid AmbigMember in lookup of interfaces with nullability differences
jcouv Nov 11, 2020
98e34b3
Bootstrapping issues
jcouv Nov 15, 2020
b1b73ee
fix bad merge
jcouv Nov 16, 2020
ec66680
Update tests
jcouv Nov 19, 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
4 changes: 4 additions & 0 deletions src/Compilers/CSharp/Portable/Binder/Binder_Lookup.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1148,6 +1148,10 @@ private static void MergeHidingLookupResults(LookupResult resultHiding, LookupRe
// SPEC: interface type, the base types of T are the base interfaces
// SPEC: of T and the class type object.

if (hiddenContainer.Equals(hidingSym.ContainingType, TypeCompareKind.IgnoreNullableModifiersForReferenceTypes))
{
goto symIsHidden; // discard the new candidate, as it is not really different
}
if (!IsDerivedType(baseType: hiddenContainer, derivedType: hidingSym.ContainingType, basesBeingResolved, useSiteDiagnostics: ref useSiteDiagnostics) &&
hiddenContainer.SpecialType != SpecialType.System_Object)
{
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3828,7 +3828,7 @@ void splitAndLearnFromNonNullTest(BoundExpression operandComparedToNonNull, bool
}
derivedType = derivedType.StrippedType();
HashSet<DiagnosticInfo>? useSiteDiagnostics = null;
var conversion = _conversions.ClassifyBuiltInConversion(derivedType, baseType, ref useSiteDiagnostics);
var conversion = _conversions.WithNullability(false).ClassifyBuiltInConversion(derivedType, baseType, ref useSiteDiagnostics);
if (conversion.Exists && !conversion.IsExplicit)
{
return derivedType;
Expand Down
3 changes: 2 additions & 1 deletion src/Compilers/CSharp/Portable/Symbols/Tuples/TupleTypeSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -877,7 +877,8 @@ private TypeSymbol MergeTupleNames(NamedTypeSymbol other, NamedTypeSymbol merged
}
}

bool namesUnchanged = mergedNames.IsDefault ? TupleElementNames.IsDefault : mergedNames.SequenceEqual(TupleElementNames);
// TODO2 bug number
bool namesUnchanged = mergedNames.IsDefault ? TupleElementNames.IsDefault : mergedNames.SequenceEqual(TupleElementNames, comparer: null);
return (namesUnchanged && this.Equals(mergedType, TypeCompareKind.ConsiderEverything))
? this
: CreateTuple(mergedType, mergedNames, this.TupleErrorPositions, this.TupleElementLocations, this.Locations);
Expand Down
21 changes: 5 additions & 16 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 @@ -475,30 +475,19 @@ internal TypeWithAnnotations SubstituteTypeCore(AbstractTypeMap typeMap)
{
newAnnotation = NullableAnnotation.Annotated;
}
else if (IsIndexedTypeParameter(newTypeWithModifiers.Type))
else if (NullableAnnotation.IsNotAnnotated())
{
newAnnotation = NullableAnnotation;
newAnnotation = NullableAnnotation.NotAnnotated;
}
else if (NullableAnnotation != NullableAnnotation.Oblivious)
else if (newTypeWithModifiers.NullableAnnotation.IsNotAnnotated())
{
if (!typeSymbol.IsTypeParameterDisallowingAnnotationInCSharp8())
Copy link
Contributor

@AlekseyTs AlekseyTs Oct 17, 2020 •
edited by jcouv
Loading

Choose a reason for hiding this comment

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

IsTypeParameterDisallowingAnnotationInCSharp8 [](start = 32, length = 45)

Where else do we use this helper and whether we still need to use it, given that we now allow annotations on type parameters like that? #Resolved

Copy link
Member Author

@jcouv jcouv Oct 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Unless it's crucial for this discussion or PR, I'd rather not branch out to that question. There are many other places that use this method at the moment.

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

{
newAnnotation = NullableAnnotation;
}
else
{
newAnnotation = newTypeWithModifiers.NullableAnnotation;
}
}
else if (newTypeWithModifiers.NullableAnnotation != NullableAnnotation.Oblivious)
{
newAnnotation = newTypeWithModifiers.NullableAnnotation;
newAnnotation = NullableAnnotation.NotAnnotated;
}
else
{
Debug.Assert(NullableAnnotation.IsOblivious());
Debug.Assert(newTypeWithModifiers.NullableAnnotation.IsOblivious());
newAnnotation = NullableAnnotation;
newAnnotation = NullableAnnotation.Oblivious;
}

return CreateNonLazyType(
Expand Down
10 changes: 5 additions & 5 deletions src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_Nullable.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ public interface I0 : I1<string>
imc1.InterfacesNoUseSiteDiagnostics().Select(i => i.ToTestDisplayString(includeNonNullable: true)));

AssertEx.SetEqual(
new[] { "I1<System.String>", "I2<System.String, System.Object!>" },
new[] { "I1<System.String>", "I2<System.String!, System.Object!>" },
imc1.AllInterfacesNoUseSiteDiagnostics.Select(i => i.ToTestDisplayString(includeNonNullable: true)));

var client_source = @"
Expand All @@ -90,12 +90,12 @@ public void M(I0 imc)
// Note: it is expected that the symbol shows different Interfaces in PE vs. compilation reference
AssertEx.SetEqual(
useImageReferences
? new[] { "I1<System.String>", "I2<System.String, System.Object!>" }
? new[] { "I1<System.String>", "I2<System.String!, System.Object!>" }
: new[] { "I1<System.String>" },
imc2.InterfacesNoUseSiteDiagnostics().Select(i => i.ToTestDisplayString(includeNonNullable: true)));

AssertEx.SetEqual(
new[] { "I1<System.String>", "I2<System.String, System.Object!>" },
new[] { "I1<System.String>", "I2<System.String!, System.Object!>" },
imc2.AllInterfacesNoUseSiteDiagnostics.Select(i => i.ToTestDisplayString(includeNonNullable: true)));
}

Expand Down Expand Up @@ -136,7 +136,7 @@ public class C0 : I1<string>
lib2_c0.InterfacesNoUseSiteDiagnostics().Select(i => i.ToTestDisplayString(includeNonNullable: true)));

AssertEx.SetEqual(
new[] { "I1<System.String>", "I2<System.String, System.Object!>" },
new[] { "I1<System.String>", "I2<System.String!, System.Object!>" },
lib2_c0.AllInterfacesNoUseSiteDiagnostics.Select(i => i.ToTestDisplayString(includeNonNullable: true)));

CompileAndVerify(lib2_comp, validator: assembly =>
Expand Down Expand Up @@ -171,7 +171,7 @@ public class C1 : C0
lib3_c0.InterfacesNoUseSiteDiagnostics().Select(i => i.ToTestDisplayString(includeNonNullable: true)));

AssertEx.SetEqual(
new[] { "I1<System.String>", "I2<System.String, System.Object!>" },
new[] { "I1<System.String>", "I2<System.String!, System.Object!>" },
lib3_c0.AllInterfacesNoUseSiteDiagnostics.Select(i => i.ToTestDisplayString(includeNonNullable: true)));

CompileAndVerify(lib3_comp, validator: assembly =>
Expand Down
Loading