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

jcouv · 2020-11-12T22:17:42Z

Scenarios where an interface is implemented twice with nullability differences result in lookup errors.
See example below. My PR on type substitution (#48694) would make this more common (wouldn't require the class constraint) and ran into this error during bootstrap build.

The general desire for the nullability feature is that it should only produce warnings.

Options:

merge types and their nullabilities (see TypeWithAnnotations.MergeEquivalentTypes), so that AllInterfaces wouldn't contain such duplicates
merges types and their nullabilities for purpose of lookup (see GetBaseInterfaces in Binder_Lookup.cs)
pick a winner whenever such conflicts are detected during lookup

From discussion with Aleksey, option 3 seems the safest, as it does not create types that didn't exist. It's possible that parts of the code base assume that .Interfaces is a subset of .AllInterfaces, for instance, or that members found during lookup on an interface belong to that interface or its bases.

Open question: should we gate this fix behing LangVer in some way?

#nullable disable
interface I<T> where T : class { T Item { get; } }

#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; // error CS0229: Ambiguity between 'I<object>.Item' and 'I<object>.Item'
    }
}

RikkiGibson · 2020-11-13T23:45:38Z

Open question: should we gate this fix behing LangVer in some way?

At a glance it feels like no to me. This is a bug-fix level change, especially since in old compilers use of the class constraint will also be needed to get a unification failure.
#Closed

jaredpar · 2020-11-16T18:47:27Z

Agree. Don't gate on langver #Closed

jaredpar · 2020-11-16T18:48:35Z

src/Compilers/CSharp/Portable/Binder/Binder_Lookup.cs

@@ -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))


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

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

AlekseyTs · 2020-11-16T22:37:15Z

src/Compilers/CSharp/Portable/Binder/Binder_Lookup.cs

@@ -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


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

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

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

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)

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)

AlekseyTs · 2020-11-16T22:45:59Z

src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs

+}
+";
+            var comp = CreateCompilation(src);
+            comp.VerifyDiagnostics();


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

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)

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)

AlekseyTs · 2020-11-16T22:47:36Z

Done with review pass (iteration 1) #Closed

AlekseyTs · 2020-11-16T22:49:41Z

src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs

+    void M(I3 i)
+    {
+        _ = i.Item;
+        i.Item = null;


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

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

AlekseyTs · 2020-12-16T14:24:38Z

src/Compilers/CSharp/Portable/Binder/Binder_Lookup.cs

@@ -1058,11 +1058,21 @@ static void addAllInterfaces(NamedTypeSymbol @interface, HashSet<NamedTypeSymbol
            if (interfaces.Length > 0)
            {
                var tmp = LookupResult.GetInstance();
-                foreach (TypeSymbol baseInterface in interfaces)
+                HashSet<NamedTypeSymbol> seenInterfaces = null;


HashSet [](start = 16, length = 7)

Would it make sense to use pooled hash set? #Closed

The reason I didn't use the pool is that we'd need a new one for this specific comparer. Not sure that's warranted

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

AlekseyTs · 2020-12-16T14:29:39Z

src/Compilers/CSharp/Portable/Binder/Binder_Lookup.cs

+
+                foreach (NamedTypeSymbol baseInterface in interfaces)
+                {
+                    if (seenInterfaces is null || !seenInterfaces.Contains(baseInterface))


seenInterfaces is null || !seenInterfaces.Contains(baseInterface) [](start = 24, length = 65)

seenInterfaces?.Add(baseInterface) != false and get rid of the Add inside the if? #Closed

AlekseyTs · 2020-12-16T14:37:13Z

src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs

+            var tree = comp.SyntaxTrees.Single();
+            var model = comp.GetSemanticModel(tree, ignoreAccessibility: false);
+            var item = tree.GetRoot().DescendantNodes().OfType<MemberAccessExpressionSyntax>().First();
+            Assert.True(model.LookupNames(item.SpanStart, i3.GetPublicSymbol()).Contains("Item"));


.Contains("Item") [](start = 79, length = 17)

I think we actually want to assert in a way that would allow us to verify from which flavor of the interface this symbol actually came from. We want to see is there an ambiguity here and how it is resolved. #Closed

AlekseyTs · 2020-12-16T14:41:22Z

src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs

+            var model = comp.GetSemanticModel(tree, ignoreAccessibility: false);
+            var item = tree.GetRoot().DescendantNodes().OfType<ElementAccessExpressionSyntax>().First();
+            Assert.Equal("i[0]", item.ToString());
+            Assert.Equal("System.Object I<System.Object>.this[System.Int32 i] { get; set; }", model.GetSymbolInfo(item).Symbol.ToTestDisplayString());


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

Should we use TypeWithAnnotations.TestDisplayFormat here? #Closed

AlekseyTs · 2020-12-16T14:44:49Z

src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs

+            var model = comp.GetSemanticModel(tree, ignoreAccessibility: false);
+            var item = tree.GetRoot().DescendantNodes().OfType<InvocationExpressionSyntax>().First();
+            Assert.Equal("i.Get()", item.ToString());
+            Assert.Equal("ref System.Object I<System.Object>.Get()", model.GetSymbolInfo(item).Symbol.ToTestDisplayString());


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

Should we use TypeWithAnnotations.TestDisplayFormat here? #Closed

AlekseyTs · 2020-12-16T14:46:03Z

src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs

+            Assert.Equal("ref System.Object I<System.Object>.Get()", model.GetSymbolInfo(item).Symbol.ToTestDisplayString());
+
+            var i3 = comp.GetTypeByMetadataName("I3");
+            Assert.True(model.LookupNames(item.SpanStart, i3.GetPublicSymbol()).Contains("Get"));


Contains("Get") [](start = 80, length = 15)

Similar comment as for the other test above. #Closed

AlekseyTs · 2020-12-16T14:50:51Z

src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs

+
+public class C
+{
+    void M(I3 i)


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

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)

AlekseyTs · 2020-12-16T14:54:43Z

src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs

+            var model = comp.GetSemanticModel(tree, ignoreAccessibility: false);
+            var item = tree.GetRoot().DescendantNodes().OfType<MemberAccessExpressionSyntax>().First();
+            Assert.Equal("i.Item", item.ToString());
+            Assert.Equal("System.Object I<System.Object>.Item { get; set; }", model.GetSymbolInfo(item).Symbol.ToTestDisplayString());


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

TypeWithAnnotations.TestDisplayFormat? #Closed

AlekseyTs · 2020-12-16T15:08:37Z

src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs

+interface I2<T> : I<T> where T : class { }
+
+#nullable disable
+interface I3 : I<object>, I2<object> { }


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

Does flipping the order here change what symbols we get? #Closed

It does. See AmbigMember_DifferenceBetweenNonnullableAndOblivious_WithConstraint_ReverseOrder

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

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)

AlekseyTs · 2020-12-16T15:12:23Z

Done with review pass (commit 4) #Closed

AlekseyTs

LGTM (commit 5), assuming CI is passing.

jcouv · 2020-12-18T15:38:33Z

@dotnet/roslyn-compiler for second review. Thanks

RikkiGibson · 2020-12-18T22:46:52Z

src/Compilers/Test/Utilities/CSharp/SymbolUtilities.cs

        {
-            return symbols.Select(s => s.ToTestDisplayString()).ToArray();
+            format ??= SymbolDisplayFormat.TestFormat;


nit: consider doing something like the following here:

return symbols.SelectAsArray(static (s, format) => s.ToDisplayString(format), format);

I'll leave as-is to get this into preview3. Thanks

jcouv added Area-Compilers Feature - Nullable Reference Types Nullable Reference Types labels Nov 12, 2020

jcouv self-assigned this Nov 12, 2020

jcouv force-pushed the merge-types branch 2 times, most recently from 9714d4d to 2eb4242 Compare November 13, 2020 20:18

Avoid AmbigMember in lookup of interfaces with nullability differences

7ad43bd

jcouv force-pushed the merge-types branch from 2eb4242 to 7ad43bd Compare November 13, 2020 21:35

jcouv changed the title ~~Avoid AmbigMember in lookup of interfaces with nullability differences~~ Avoid AmbigMember in lookup of interfaces with nullability differences only Nov 13, 2020

jcouv marked this pull request as ready for review November 13, 2020 22:46

jcouv requested a review from a team as a code owner November 13, 2020 22:46

jcouv requested a review from AlekseyTs November 13, 2020 22:46

jaredpar reviewed Nov 16, 2020

View reviewed changes

AlekseyTs reviewed Nov 16, 2020

View reviewed changes

Test with different kinds of members

3d0f5f4

jcouv mentioned this pull request Dec 15, 2020

Disallow record with ambiguous constructors #49975

Merged

jcouv added 2 commits December 15, 2020 12:43

Move check to LookupMembersInInterfacesWithoutInheritance

0f513dc

Check LookupNames

d34117e

AlekseyTs reviewed Dec 16, 2020

View reviewed changes

Address feedback

a26167d

AlekseyTs approved these changes Dec 17, 2020

View reviewed changes

jcouv added 2 commits December 17, 2020 12:01

Merge remote-tracking branch 'dotnet/master' into merge-types

2d926ad

remove newlines

d789eca

RikkiGibson self-assigned this Dec 18, 2020

RikkiGibson reviewed Dec 18, 2020

View reviewed changes

RikkiGibson approved these changes Dec 18, 2020

View reviewed changes

jcouv merged commit 932e0f1 into dotnet:master Dec 18, 2020

ghost added this to the Next milestone Dec 18, 2020

jcouv deleted the merge-types branch December 18, 2020 23:08

dibarbet modified the milestones: Next, 16.9.P3 Dec 19, 2020

jcouv mentioned this pull request Feb 9, 2021

Apparent compiler issue with differing NRT settings across project boundaries #41179

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jcouv commented Nov 12, 2020 •

edited

Loading

RikkiGibson commented Nov 13, 2020 •

edited by jcouv

Loading

jaredpar commented Nov 16, 2020 •

edited by jcouv

Loading

jaredpar Nov 16, 2020 •

edited by jcouv

Loading

jcouv Nov 16, 2020 •

edited

Loading

AlekseyTs Nov 16, 2020 •

edited

Loading

AlekseyTs Nov 16, 2020

jcouv Dec 15, 2020

AlekseyTs Dec 15, 2020

AlekseyTs Nov 16, 2020 •

edited

Loading

jcouv Dec 14, 2020

AlekseyTs Dec 15, 2020

AlekseyTs commented Nov 16, 2020 •

edited by jcouv

Loading

AlekseyTs Nov 16, 2020 •

edited

Loading

AlekseyTs Dec 16, 2020 •

edited

Loading

jcouv Dec 16, 2020

AlekseyTs Dec 16, 2020 •

edited

Loading

AlekseyTs Dec 16, 2020 •

edited

Loading

AlekseyTs Dec 16, 2020 •

edited

Loading

AlekseyTs Dec 16, 2020 •

edited

Loading

AlekseyTs Dec 16, 2020 •

edited

Loading

AlekseyTs Dec 16, 2020 •

edited

Loading

AlekseyTs Dec 16, 2020

AlekseyTs Dec 16, 2020 •

edited

Loading

AlekseyTs Dec 16, 2020 •

edited

Loading

jcouv Dec 16, 2020

AlekseyTs Dec 16, 2020

AlekseyTs commented Dec 16, 2020 •

edited

Loading

AlekseyTs left a comment

jcouv commented Dec 18, 2020

RikkiGibson Dec 18, 2020

jcouv Dec 18, 2020

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

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

Conversation

jcouv commented Nov 12, 2020 • edited Loading

RikkiGibson commented Nov 13, 2020 • edited by jcouv Loading

jaredpar commented Nov 16, 2020 • edited by jcouv Loading

jaredpar Nov 16, 2020 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Nov 16, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Nov 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Nov 16, 2020 • edited by jcouv Loading

AlekseyTs Nov 16, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Dec 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Dec 16, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Dec 16, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Dec 16, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Dec 16, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Dec 16, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Dec 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Dec 16, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Dec 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Dec 16, 2020 • edited Loading

AlekseyTs left a comment

Choose a reason for hiding this comment

jcouv commented Dec 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv commented Nov 12, 2020 •

edited

Loading

RikkiGibson commented Nov 13, 2020 •

edited by jcouv

Loading

jaredpar commented Nov 16, 2020 •

edited by jcouv

Loading

jaredpar Nov 16, 2020 •

edited by jcouv

Loading

jcouv Nov 16, 2020 •

edited

Loading

AlekseyTs Nov 16, 2020 •

edited

Loading

AlekseyTs Nov 16, 2020 •

edited

Loading

AlekseyTs commented Nov 16, 2020 •

edited by jcouv

Loading

AlekseyTs Nov 16, 2020 •

edited

Loading

AlekseyTs Dec 16, 2020 •

edited

Loading

AlekseyTs Dec 16, 2020 •

edited

Loading

AlekseyTs Dec 16, 2020 •

edited

Loading

AlekseyTs Dec 16, 2020 •

edited

Loading

AlekseyTs Dec 16, 2020 •

edited

Loading

AlekseyTs Dec 16, 2020 •

edited

Loading

AlekseyTs Dec 16, 2020 •

edited

Loading

AlekseyTs Dec 16, 2020 •

edited

Loading

AlekseyTs Dec 16, 2020 •

edited

Loading

AlekseyTs commented Dec 16, 2020 •

edited

Loading