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

Avoid AmbigMember in lookup of interfaces with nullability differences only #49338

Merged
merged 7 commits into from
Dec 18, 2020
+855 −43
Merged

Avoid AmbigMember in lookup of interfaces with nullability differences only #49338

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7ad43bd
Avoid AmbigMember in lookup of interfaces with nullability differences
jcouv Nov 11, 2020
3d0f5f4
Test with different kinds of members
jcouv Dec 14, 2020
0f513dc
Move check to LookupMembersInInterfacesWithoutInheritance
jcouv Dec 15, 2020
d34117e
Check LookupNames
jcouv Dec 16, 2020
a26167d
Address feedback
jcouv Dec 16, 2020
2d926ad
Merge remote-tracking branch 'dotnet/master' into merge-types
jcouv Dec 17, 2020
d789eca
remove newlines
jcouv Dec 17, 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))
Copy link
Member

@jaredpar jaredpar Nov 16, 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.

For my eductation: can we get into a similar problem when interfaces differ by object and dynamic type parameters? #Closed

Copy link
Member Author

@jcouv jcouv Nov 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

yes, but dynamic is not allowed as a type argument. See AmbigMember_DynamicDifferences below. #Closed

{
goto symIsHidden; // discard the new candidate, as it is not really different
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

goto symIsHidden [](start = 32, length = 16)

How do we know it is the "same" member? We are not looking at signature, etc. For example, in the if below that is also deals with "hiding" we don't make this assumption. I am assuming that we are "resolving" that "hiding" elsewhere. It feels like the new hiding should be resolved there as well. Meaning that we simply add negated if condition to the if below and add additional code elsewhere to handle methods and indexers. #Closed

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like alternatively we could filter out "duplicate" interfaces inside ```GetBaseInterfaces```` helper.

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we can filter in GetBaseInterfaces because that would affect IsDerivedType which also uses that helper, but we could do the filtering at the call-site of GetBaseInterfaces in LookupMembersInInterfaceOnly.
I had done that in drafting this PR, but changed to this approach to reduce allocations and iterations.
Do you have a preference?

I didn't fully understand your question above. We can discuss tomorrow.

In reply to: 524709873 [](ancestors = 524709873,524690053)

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we can filter in GetBaseInterfaces because that would affect IsDerivedType which also uses that helper

That is a fair point. Then it looks like we could filter interfaces in LookupMembersInInterfacesWithoutInheritance. Why bother even performing a lookup if we already looked in a "similar" interface and planning to discard everything we find?

I didn't fully understand your question above.

C# hides by signature. Signature is part of detecting if the candidate is different (as the comment says). Where do we check the signature if we are saying "as it is not really different"? If we don't check the signature, we shouldn't claim that "it is not really different".

In reply to: 542952838 [](ancestors = 542952838,524709873,524690053)

}
if (!IsDerivedType(baseType: hiddenContainer, derivedType: hidingSym.ContainingType, basesBeingResolved, useSiteDiagnostics: ref useSiteDiagnostics) &&
hiddenContainer.SpecialType != SpecialType.System_Object)
{
Expand Down
371 changes: 371 additions & 0 deletions src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141927,5 +141927,376 @@ static T GetValue2<T>(Collection c, object key)
// return s2; // 4
Diagnostic(ErrorCode.WRN_NullReferenceReturn, "s2").WithLocation(47, 20));
}

[Fact]
public void AmbigMember_DynamicDifferences()
{
var src = @"
interface I<T> { T Item { get; } }

interface I2<T> : I<T> { }

interface I3 : I<dynamic>, I2<object> { }

public class C
{
void M(I3 i)
Copy link
Contributor

@AlekseyTs AlekseyTs Dec 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

void M(I3 i) [](start = 4, length = 12)

I think we should also test lookup in a type parameter. I.e. when i is of type T, which has I3 as a constraint. #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 also test lookup in a type parameter. I.e. when i is of type T, which has I3 as a constraint.

This comment is for scenarios involving the nullability mismatch. Also, I think we should test scenarios when constraints are just I<object>, I2<object> and I2<object>, I<object>.

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

{
_ = i.Item;
}
}
";
var comp = CreateCompilation(src);
comp.VerifyDiagnostics(
// (6,11): error CS8779: 'I<object>' is already listed in the interface list on type 'I3' as 'I<dynamic>'.
// interface I3 : I<dynamic>, I2<object> { }
Diagnostic(ErrorCode.ERR_DuplicateInterfaceWithDifferencesInBaseList, "I3").WithArguments("I<object>", "I<dynamic>", "I3").WithLocation(6, 11),
// (6,16): error CS1966: 'I3': cannot implement a dynamic interface 'I<dynamic>'
// interface I3 : I<dynamic>, I2<object> { }
Diagnostic(ErrorCode.ERR_DeriveFromConstructedDynamic, "I<dynamic>").WithArguments("I3", "I<dynamic>").WithLocation(6, 16),
// (12,15): error CS0229: Ambiguity between 'I<dynamic>.Item' and 'I<object>.Item'
// _ = i.Item;
Diagnostic(ErrorCode.ERR_AmbigMember, "Item").WithArguments("I<dynamic>.Item", "I<object>.Item").WithLocation(12, 15)
);
var i3 = comp.GetTypeByMetadataName("I3");
AssertEx.Equal(new string[] { "I<dynamic>", "I2<object>" },
i3.Interfaces().ToTestDisplayStrings(TypeWithAnnotations.TestDisplayFormat));
AssertEx.Equal(new string[] { "I<dynamic>", "I2<object>", "I<object>" },
i3.AllInterfaces().ToTestDisplayStrings(TypeWithAnnotations.TestDisplayFormat));
}

[Fact]
public void AmbigMember_TupleDifferences()
{
var src = @"
interface I<T> { T Item { get; } }

interface I2<T> : I<T> { }

interface I3 : I<(int a, int b)>, I2<(int notA, int notB)> { }

public class C
{
void M(I3 i)
{
_ = i.Item;
}
}
";
var comp = CreateCompilation(src);
comp.VerifyDiagnostics(
// (6,11): error CS8140: 'I<(int notA, int notB)>' is already listed in the interface list on type 'I3' with different tuple element names, as 'I<(int a, int b)>'.
// interface I3 : I<(int a, int b)>, I2<(int notA, int notB)> { }
Diagnostic(ErrorCode.ERR_DuplicateInterfaceWithTupleNamesInBaseList, "I3").WithArguments("I<(int notA, int notB)>", "I<(int a, int b)>", "I3").WithLocation(6, 11),
// (12,15): error CS0229: Ambiguity between 'I<(int a, int b)>.Item' and 'I<(int notA, int notB)>.Item'
// _ = i.Item;
Diagnostic(ErrorCode.ERR_AmbigMember, "Item").WithArguments("I<(int a, int b)>.Item", "I<(int notA, int notB)>.Item").WithLocation(12, 15)
);
var i3 = comp.GetTypeByMetadataName("I3");
AssertEx.Equal(new string[] { "I<(int a, int b)>", "I2<(int notA, int notB)>" },
i3.Interfaces().ToTestDisplayStrings(TypeWithAnnotations.TestDisplayFormat));
AssertEx.Equal(new string[] { "I<(int a, int b)>", "I2<(int notA, int notB)>", "I<(int notA, int notB)>" },
i3.AllInterfaces().ToTestDisplayStrings(TypeWithAnnotations.TestDisplayFormat));
}

[Fact]
public void AmbigMember_TupleAndNullabilityDifferences()
{
var src = @"
#nullable disable
interface I<T> { T Item { get; } }

#nullable enable
interface I2<T> : I<T> { }

#nullable disable
interface I3 : I<(object a, object b)>, I2<(object notA, object notB)> { }

#nullable enable
public class C
{
void M(I3 i)
{
_ = i.Item;
}
}
";
var comp = CreateCompilation(src);
comp.VerifyDiagnostics(
// (9,11): error CS8140: 'I<(object notA, object notB)>' is already listed in the interface list on type 'I3' with different tuple element names, as 'I<(object a, object b)>'.
// interface I3 : I<(object a, object b)>, I2<(object notA, object notB)> { }
Diagnostic(ErrorCode.ERR_DuplicateInterfaceWithTupleNamesInBaseList, "I3").WithArguments("I<(object notA, object notB)>", "I<(object a, object b)>", "I3").WithLocation(9, 11),
// (16,15): error CS0229: Ambiguity between 'I<(object a, object b)>.Item' and 'I<(object notA, object notB)>.Item'
// _ = i.Item;
Diagnostic(ErrorCode.ERR_AmbigMember, "Item").WithArguments("I<(object a, object b)>.Item", "I<(object notA, object notB)>.Item").WithLocation(16, 15)
);
var i3 = comp.GetTypeByMetadataName("I3");
AssertEx.Equal(new string[] { "I<(object a, object b)>", "I2<(object notA, object notB)>" },
i3.Interfaces().ToTestDisplayStrings(TypeWithAnnotations.TestDisplayFormat));
AssertEx.Equal(new string[] { "I<(object a, object b)>", "I2<(object notA, object notB)>", "I<(object notA, object notB)>" },
i3.AllInterfaces().ToTestDisplayStrings(TypeWithAnnotations.TestDisplayFormat));
}

[Fact]
public void AmbigMember_NoDifference()
{
var src = @"
interface I<T> { T Item { get; } }

interface I2<T> : I<T> { }

interface I3 : I<object>, I2<object> { }

public class C
{
void M(I3 i)
{
_ = i.Item;
}
}
";
var comp = CreateCompilation(src);
comp.VerifyDiagnostics(
);
var i3 = comp.GetTypeByMetadataName("I3");
AssertEx.Equal(new string[] { "I<object>", "I2<object>" },
i3.Interfaces().ToTestDisplayStrings(TypeWithAnnotations.TestDisplayFormat));
AssertEx.Equal(new string[] { "I2<object>", "I<object>" },
i3.AllInterfaces().ToTestDisplayStrings(TypeWithAnnotations.TestDisplayFormat));
}

[Fact]
public void AmbigMember_DifferenceBetweenNonnullableAndOblivious_WithoutConstraint()
{
var src = @"
#nullable disable
interface I<T> { T Item { get; set; } }

#nullable enable
interface I2<T> : I<T> { }

#nullable disable
interface I3 : I<object>, I2<object> { }

#nullable enable
public class C
{
void M(I3 i)
{
_ = i.Item;
i.Item = null;
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Item [](start = 10, length = 4)

Consider expanding testing to other kinds of members: fields, indexers, events, nested types, methods. #Closed

}
}
";
var comp = CreateCompilation(src);
comp.VerifyDiagnostics();
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

comp.VerifyDiagnostics(); [](start = 12, length = 25)

In this and other tests that are supposed to verify that an ambiguity is successfully resolved, I think we should verify how exactly it is resolved to ensure that the resolution is deterministic, etc. #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.

We're already verifying this in two ways: the behavior of .Item during nullability analysis, and by checking how the ambiguity is resolved in AllInterfaces.
I could also check GetSymbolInfo on i.Item, but that doesn't seem to add much. Is that what you had in mind?

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

Copy link
Contributor

Choose a reason for hiding this comment

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

and by checking how the ambiguity is resolved in AllInterfaces

As far as I can tell, there is no change to the behavior of this API. What could we be checking?

I could also check GetSymbolInfo on i.Item, but that doesn't seem to add much. Is that what you had in mind?

Yes. something like that, I think we need to assert specific symbol. We probably should also test lookup APIs that SemanticModel offers.

In reply to: 542852918 [](ancestors = 542852918,524699086)

var i3 = comp.GetTypeByMetadataName("I3");
AssertEx.Equal(new string[] { "I<object>", "I2<object>" },
i3.Interfaces().ToTestDisplayStrings(TypeWithAnnotations.TestDisplayFormat));
AssertEx.Equal(new string[] { "I2<object>", "I<object>" },
i3.AllInterfaces().ToTestDisplayStrings(TypeWithAnnotations.TestDisplayFormat));
}

[Fact]
public void AmbigMember_DifferenceBetweenNonnullableAndOblivious_WithoutConstraint_ReverseOrder()
{
var src = @"
Copy link
Contributor

@AlekseyTs AlekseyTs Dec 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

ToTestDisplayString [](start = 111, length = 19)

TypeWithAnnotations.TestDisplayFormat? #Closed

#nullable disable
interface I<T> { T Item { get; set; } }

#nullable enable
interface I2<T> : I<T> { }

#nullable disable
interface I3 : I2<object>, I<object> { }

#nullable enable
public class C
{
void M(I3 i)
{
_ = i.Item;
i.Item = null;
}
}
";
var comp = CreateCompilation(src);
comp.VerifyDiagnostics();
var i3 = comp.GetTypeByMetadataName("I3");
AssertEx.Equal(new string[] { "I2<object>", "I<object>" },
i3.Interfaces().ToTestDisplayStrings(TypeWithAnnotations.TestDisplayFormat));
AssertEx.Equal(new string[] { "I2<object>", "I<object>" },
i3.AllInterfaces().ToTestDisplayStrings(TypeWithAnnotations.TestDisplayFormat));
}

[Fact]
public void AmbigMember_DifferenceBetweenNonnullableAndOblivious_WithConstraint()
{
var src = @"
#nullable disable
interface I<T> where T : class { T Item { get; set; } }

#nullable enable
interface I2<T> : I<T> where T : class { }

#nullable disable
interface I3 : I<object>, I2<object> { }

#nullable enable
public class C
{
void M(I3 i)
{
_ = i.Item;
i.Item = null;
}
}
";
var comp = CreateCompilation(src);
comp.VerifyDiagnostics();
var i3 = comp.GetTypeByMetadataName("I3");
AssertEx.Equal(new string[] { "I<object>", "I2<object>" },
i3.Interfaces().ToTestDisplayStrings(TypeWithAnnotations.TestDisplayFormat));
AssertEx.Equal(new string[] { "I<object>", "I2<object>", "I<object!>" },
i3.AllInterfaces().ToTestDisplayStrings(TypeWithAnnotations.TestDisplayFormat));
}

[Fact]
public void AmbigMember_DifferenceBetweenNonnullableAndOblivious_WithConstraint_ReverseOrder()
{
var src = @"
#nullable disable
interface I<T> where T : class { T Item { get; set; } }

#nullable enable
interface I2<T> : I<T> where T : class { }

#nullable disable
interface I3 : I2<object>, I<object> { }

#nullable enable
public class C
{
void M(I3 i)
{
_ = i.Item;
i.Item = null;
}
}
";
var comp = CreateCompilation(src);
comp.VerifyDiagnostics(
// (17,18): warning CS8625: Cannot convert null literal to non-nullable reference type.
// i.Item = null;
Diagnostic(ErrorCode.WRN_NullAsNonNullable, "null").WithLocation(17, 18)
);
var i3 = comp.GetTypeByMetadataName("I3");
AssertEx.Equal(new string[] { "I2<object>", "I<object>" },
i3.Interfaces().ToTestDisplayStrings(TypeWithAnnotations.TestDisplayFormat));
AssertEx.Equal(new string[] { "I2<object>", "I<object!>", "I<object>" },
i3.AllInterfaces().ToTestDisplayStrings(TypeWithAnnotations.TestDisplayFormat));
}

[Fact]
public void AmbigMember_DifferenceBetweenNullableAndOblivious_WithConstraint()
{
var src = @"
#nullable disable
interface I<T> where T : class { T Item { get; } }

#nullable enable
interface I2<T> : I<T> where T : class { }

interface I3 : I<object?>,
#nullable disable
I2<object> { }

#nullable enable
public class C
{
void M(I3 i)
{
_ = i.Item;
}
}
";
var comp = CreateCompilation(src);
comp.VerifyDiagnostics(
// (8,11): warning CS8645: 'I<object>' is already listed in the interface list on type 'I3' with different nullability of reference types.
// interface I3 : I<object?>,
Diagnostic(ErrorCode.WRN_DuplicateInterfaceWithNullabilityMismatchInBaseList, "I3").WithArguments("I<object>", "I3").WithLocation(8, 11)
);
var i3 = comp.GetTypeByMetadataName("I3");
AssertEx.Equal(new string[] { "I<object?>", "I2<object>" },
i3.Interfaces().ToTestDisplayStrings(TypeWithAnnotations.TestDisplayFormat));
AssertEx.Equal(new string[] { "I<object?>", "I2<object>", "I<object!>" },
i3.AllInterfaces().ToTestDisplayStrings(TypeWithAnnotations.TestDisplayFormat));
}

[Fact]
public void AmbigMember_DifferenceBetweenNullableAndOblivious_WithoutConstraint()
{
var src = @"
#nullable disable
interface I<T> { T Item { get; } }

#nullable enable
interface I2<T> : I<T> { }

interface I3 : I<object?>,
#nullable disable
I2<object> { }

#nullable enable
public class C
{
void M(I3 i)
{
_ = i.Item;
}
}
";
var comp = CreateCompilation(src);
comp.VerifyDiagnostics();
var i3 = comp.GetTypeByMetadataName("I3");
AssertEx.Equal(new string[] { "I<object?>", "I2<object>" },
i3.Interfaces().ToTestDisplayStrings(TypeWithAnnotations.TestDisplayFormat));
AssertEx.Equal(new string[] { "I<object?>", "I2<object>", "I<object>" },
i3.AllInterfaces().ToTestDisplayStrings(TypeWithAnnotations.TestDisplayFormat));
}

[Fact]
public void AmbigMember_DifferenceBetweenNullableAndNonnullable()
{
var src = @"
#nullable disable
interface I<T> where T : class { T Item { get; } }

#nullable enable
interface I2<T> : I<T> where T : class { }

interface I3 : I<object?>, I2<object> { }

#nullable enable
public class C
{
void M(I3 i)
Copy link
Contributor

@AlekseyTs AlekseyTs Dec 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I, I2 [](start = 15, length = 21)

Does flipping the order here change what symbols we get? #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.

It does. See AmbigMember_DifferenceBetweenNonnullableAndOblivious_WithConstraint_ReverseOrder

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

Copy link
Contributor

Choose a reason for hiding this comment

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

It does. See AmbigMember_DifferenceBetweenNonnullableAndOblivious_WithConstraint_ReverseOrder

It doesn't look like we assert symbol in that test.

In reply to: 544603988 [](ancestors = 544603988,544374723)

{
_ = i.Item;
}
}
";
var comp = CreateCompilation(src);
comp.VerifyDiagnostics(
// (8,11): warning CS8645: 'I<object>' is already listed in the interface list on type 'I3' with different nullability of reference types.
// interface I3 : I<object?>, I2<object> { }
Diagnostic(ErrorCode.WRN_DuplicateInterfaceWithNullabilityMismatchInBaseList, "I3").WithArguments("I<object>", "I3").WithLocation(8, 11)
);
var i3 = comp.GetTypeByMetadataName("I3");
AssertEx.Equal(new string[] { "I<object?>", "I2<object!>" },
i3.Interfaces().ToTestDisplayStrings(TypeWithAnnotations.TestDisplayFormat));
AssertEx.Equal(new string[] { "I<object?>", "I2<object!>", "I<object!>" },
i3.AllInterfaces().ToTestDisplayStrings(TypeWithAnnotations.TestDisplayFormat));
}
}
}
Loading