Properly treat ambiguous explicit interface implementations by YairHalberstadt · Pull Request #34584 · dotnet/roslyn

YairHalberstadt · 2019-03-29T08:33:47Z

involving nullable reference types, including maintaining backwards compatability with pre -NRT code.

pre C# 8.0, void I.Foo<T>(T? value) { } was a valid implementation of void Foo<T>(T? value) where T : struct;.

In C# 8 it's a valid implementation of:

 void Foo<T>(T? value) where T : struct;
 void Foo<T>(T? value) where T : class;
 void Foo<T>(T value) where T : class;

This leads to the following 2 issues.

1.

See #34508

The following is a valid, unambiguous program in C# 7.0, but in C# 8.0 leads to an ambiguity which prevents it from compiling:

interface I
{
    void Foo<T>(T value);
    void Foo<T>(T? value) where T : struct;
}

//works fine
class C1 : I
{
    public void Foo<T>(T value) { }
    public void Foo<T>(T? value) where T : struct { }
}

//several build errors
class C2 : I
{
    void I.Foo<T>(T value) { }

    //error CS0111: Type 'C2' already defines a member called 'I.Foo' with the same parameter types
    void I.Foo<T>(T? value) { }
}

Given that void I.Foo(T? value) { } is a valid implementation of both void Foo<T>(T value); and void Foo<T>(T? value) where T : struct; the compiler is allowed to decide which it should implement. It does so arbitrarily (by choosing the member declared first by the interface), causing a compile error.

To fix this regression I have taken the following approach.

When looking for the implemented member of an explicit interface implementation, The compiler will first look for members of the interface which match the implementing member including nullability annotations. Only if that fails will it look for members whose nullability annotations do not match.

Since no pre-C# 8 code has nullability annotations, this can be guaranteed not to break backwards compatibility.

In my opinion this is an intuitive, simple, and understandable solution, which doesn't require special casing all sorts of potentially backwards compatibility breaking cases.

2.

See dotnet/csharplang#2370

The compiler is required to issue a warning whenever an explicit interface implementation can match two members, or is ambiguous to the runtime. See https://docs.microsoft.com/en-us/dotnet/csharp/misc/cs0473 and https://blogs.msdn.microsoft.com/ericlippert/2006/04/06/odious-ambiguous-overloads-part-two/.

Explicit interface implementation 'method name' matches more than one interface member. Which interface member is actually chosen is implementation-dependent. Consider using a non-explicit implementation instead.

However it was not issuing such an error in this case:

#nullable enable
interface I
{
    void Foo<T>(T? value) where T : class;
    void Foo<T>(T? value) where T : struct;
}

class C2 : I
{
    void I.Foo<T>(T? value) { }
    public void Foo<T>(T? value) where T : struct {}
}

Despite the fact that the explicit interface implementation is ambiguous to the compiler.

This was because the logic for doing the check to see if an explicit interface implementation matches two interface members is done at a different point to the location where the lookup is done. As a result the logic became out of sync. This pull request moves the logic to the same location, so that the warning is guaranteed to be issued accurately.

Note that the lookup to see if a method is ambiguous to the runtime remains in its old location, as it requires completely different logic.

… nullable reference types, including maintaining backwards compatability with pre -NRT code.

AlekseyTs · 2019-03-29T17:27:18Z

@YairHalberstadt
It sounds like the issue addressed in this PR is very similar to an issue we have with overriding (for example #29846). I think we would want to use the same approach for dealing with both scenarios. Would you like to address overriding scenarios as well? #Closed

YairHalberstadt · 2019-03-29T17:32:05Z

@AlekseyTs
Sure.
The only changes in this PR are to ExplicitInterfaceHelper so I imagine that even if they are conceptually similar, they will have a different code fix.

…lision diagnostics.

…parer

src/Compilers/CSharp/Portable/Symbols/MemberSignatureComparer.cs

gafter · 2019-04-03T20:08:36Z

@YairHalberstadt Please see dotnet/csharplang#2378 (comment) . I know you're aware of that because you responded there, but I wanted to close the loop here. Could you please modify this PR to do "step 1" to fix the compatibility issue? #Resolved

YairHalberstadt · 2019-04-03T20:26:36Z

Sure. I think it's worth keeping the change where we check for compile time ambiguities during the lookup, rather than afterwards, since this is more correct, and would have helped diagnose this breaking change earlier. #Resolved

Only make one pass in explicitImplementationHelpers

…ble<T>

YairHalberstadt · 2019-04-04T08:18:42Z

Obviously this fix has broken a large number of unit tests. Depending on the intent of the test I've tried to do one of these things:

add a class constraint to the method, and skip the test till the next stage is implemented.
Change the expected result of the test.
Copy the test, and do both of the above options.

However it has been tricky to know exactly which one is best, and it's likely a few tests will need changing.

I'm now pushing my changes to here, to see which other tests fail. I'm sure there will be a few. #Resolved

…without class constraint.

YairHalberstadt · 2019-04-04T13:26:09Z

It looks like all the tests are now passing.
@gafter I think this is ready for a review for the initial part of the fix. #Resolved

gafter

LGTM. @AlekseyTs Can you please have a look?

AlekseyTs · 2019-04-08T23:11:12Z

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

        }

-        [Fact]
+        [Fact(Skip = "https://github.com/dotnet/csharplang/issues/2378#issuecomment-479634969")]


[Fact(Skip = "https://github.com/dotnet/csharplang/issues/2378#issuecomment-479634969")] [](start = 8, length = 88)

I think skipping tests doesn't help us to understand what would be the behavior after this PR. I would suggest to unskip the tests that existed prior to this PR and undo any modifications to the scenarios (like addition of constraints etc.). It is OK to add new skipped tests, but they should be linked to an active compiler issue (not a csharplang issue). #Closed

AlekseyTs · 2019-04-09T18:00:42Z

src/Compilers/CSharp/Portable/Symbols/MemberSignatureComparer.cs

-
-                for (int i = arity - 1; i >= 0; i--)
-                {
-                    builder.Add(IndexedTypeParameterSymbolForOverriding.GetTypeParameter(i, typeParameters2[i].IsValueType));


IndexedTypeParameterSymbolForOverriding.GetTypeParameter [](start = 32, length = 56)

It looks like we can remove the IndexedTypeParameterSymbolForOverriding type. #Closed

AlekseyTs · 2019-04-09T18:15:16Z

src/Compilers/CSharp/Portable/Symbols/TypeWithAnnotations.cs

                return makeNullableT();
            }

+            if (typeParameterSymbol.DeclaringMethod is {} declaringMethod && (declaringMethod.IsOverride || declaringMethod.IsExplicitInterfaceImplementation))


if (typeParameterSymbol.DeclaringMethod is {} declaringMethod && (declaringMethod.IsOverride || declaringMethod.IsExplicitInterfaceImplementation)) [](start = 12, length = 147)

This condition should be applicable only to type references in a signature of a method declaration, but it looks like this isn't the case. #Closed

For example, what would be the type of the local in this scenario (please add a test)?

interface I5 { void M5<T>(T value) where T : class; } class C51 : I5 { void I5.M5<T>(T value) { T? x = value; } }

I think the more robust approach would be to force the type from the Binder, if the Binder knows that it binds types from the signature. Or, even better, to force the type in method symbol implementation. That would allow us to take a look at the presence of the constraints once we start working on step #2.

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

The binder doesn't know it binds types from the signature.

As I see it we have three options:

When creating the TypeParameterSymbols in SourceOrdianaryMethodSymbol, recursively check the ParameterSyntaxes to see if any TypeParameters are used as NullableTypes.

When binding a Type, pass in enough information for the binder to know it is binding types from the signature.

When binding a parameter, deep copy the type of the parameter, replacing all lazy nullable types with Nullable Types.

I think 3. is the worst option, as it is both most expensive, and most error prone. I am not sure about 1 or 2, but will go with 1 for now unless you tell me otherwise.

I would try to do this inside method symbol. Once the return types and parameter types are bound, we can visit them and force lazy nullable types created over method type parameters to resolve towards Nullable<T>. We do not need to replace the TypeWithAnnotations instances. Of course, that would require adding some new APIs to TypeWithAnnotations. We would need to be able to get to the underlying type (__defaultType?) without triggering resolution. And we would need an API that would allow us to resolve the type towards Nullable<T>.

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

Given a case like this:

interface I { void M<T>(T? value) where T : class; } class C : I { void I.M<T>(T? value) { T? x = value; } }

If we implement your suggestion, we will warn on T? x = value; that T is an unconstrained type.
Is this a problem?

If we implement your suggestion, we will warn on T? x = value; that T is an unconstrained type.
Is this a problem?

I don't think this is a problem, assuming that we are going to get another error saying the implemented method is not found. I think this is the current behavior for the scenario when I.M is not present (i.e. the interface is empty).

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

YairHalberstadt · 2019-04-11T17:38:18Z

@AlekseyTs
I have made all the changes you've suggested, and am just finishing up the unit tests.
Hopefully by Monday at the latest I should be ready.
Of course that's assuming that further stages of peer review aren't necessary.

AlekseyTs · 2019-04-11T17:40:26Z

Hopefully by Monday at the latest I should be ready.

Sounds good. #Closed

Only force type parameters in method signature as nullable structs Revert changes to explicit inteface helper. Changes to unit tests

YairHalberstadt · 2019-04-12T12:03:49Z

@AlekseyTs
I think this ready for review.

One note:

We would need to be able to get to the underlying type (__defaultType?) without triggering resolution.

I assumed you used a double underscore there on purpose, so renamed _defaultType to __defaultType.

Since it is an implementation detail we are exposing, and is also readonly, I didn't hide it behind a property.

AlekseyTs · 2019-04-12T16:53:33Z

I assumed you used a double underscore there on purpose, so renamed _defaultType to __defaultType.

That actually was a typo. Sorry about that. Since that field is no longer private, it should be renamed to DefaultType to follow our naming conventions.

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

AlekseyTs · 2019-04-12T16:57:25Z

src/Compilers/CSharp/Portable/Symbols/TypeWithAnnotations.cs

        /// The underlying type, unless overridden by _extensions.
        /// </summary>
-        private readonly TypeSymbol _defaultType;
+        internal readonly TypeSymbol __defaultType;


__defaultType [](start = 37, length = 13)

Names of internal fields should start with a capital letter - DefaultType #Closed

AlekseyTs · 2019-04-12T17:52:24Z

            typeOpt = canDigThroughNullable ? next.NullableUnderlyingTypeOrSelf : null;

It looks like this is going to do resolution, but it is possible that canDigThroughNullable == true is never combined with useDefaultType == true. There are other similar places in this function. It would be good to add an assert on entry into this function. #Closed

Refers to: src/Compilers/CSharp/Portable/Symbols/TypeSymbolExtensions.cs:710 in e260f51. [](commit_id = e260f51, deletion_comment = False)

AlekseyTs · 2019-04-12T17:55:49Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceOrdinaryMethodSymbol.cs

            }

+            // Any nullable typeParameter declared by the method in the signature of an override or explicit interface implementation is considered a Nullable<T>
+            if (IsExplicitInterfaceImplementation || IsOverride)


IsExplicitInterfaceImplementation [](start = 16, length = 33)

syntax.ExplicitInterfaceSpecifier != null? #Closed

I wasn't sure which to use. What is the difference?

I wasn't sure which to use. What is the difference?

It is good to be consistent in a way this functions checks for certain conditions. For example, below it is using syntax to check if this is an explicit interface implementation. This check will also be faster, eventually the same syntax condition will be checked, I think, but it will jump through more hoops. Etc.

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

AlekseyTs · 2019-04-12T18:00:23Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceOrdinaryMethodSymbol.cs

+                        type.TryForceResolveAsNullableValueType();
+                    }
+                    return false;
+                }, null, null, false, true);


, null, null, false, true [](start = 17, length = 25)

Please use named arguments. #Closed

AlekseyTs · 2019-04-12T18:03:45Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceOrdinaryMethodSymbol.cs

+            {
+                type.VisitType<object>(null, (type, unused1, unused2) =>
+                {
+                    if (type.DefaultType.IsTypeParameter() && ((TypeParameterSymbol)type.DefaultType).DeclaringMethod != null)


type.DefaultType.IsTypeParameter() && ((TypeParameterSymbol)type.DefaultType).DeclaringMethod != null [](start = 24, length = 101)

(type.DefaultType as TypeParameterSymbol)?.DeclaringMethod == (object)this? #Closed

AlekseyTs

LGTM (iteration 19)

AlekseyTs · 2019-04-12T18:52:16Z

@gafter Could you please review the updated PR?

AlekseyTs · 2019-04-12T21:52:21Z

src/Compilers/CSharp/Portable/Symbols/TypeSymbolExtensions.cs

+            bool useDefaultType = false)
        {
            Debug.Assert(typeWithAnnotationsOpt.HasType == (typeOpt is null));
+            Debug.Assert(canDigThroughNullable = false || useDefaultType == false, "digging through nullable will cause early resolution of nullable types");


canDigThroughNullable = false [](start = 25, length = 29)

I think this assignment is not intentional and is the source of the test failures. #Closed

YairHalberstadt · 2019-04-15T07:26:02Z

I remember jcouv telling me that Foo was banned in the codebase because of spellcheckers, so I've replaced it with Goo in the test files.

AlekseyTs · 2019-04-15T20:34:29Z

@YairHalberstadt Thanks for the contribution!

* dotnet/master: (495 commits) Roslyn Installer: Stop processes that block VSIX installation. (dotnet#34886) Remove unused helper BeginInvokeOnUIThread Apply a hang mitigating timeout to InvokeOnUIThread Apply a hang mitigating timeout in RestoreNuGetPackages Apply a hang mitigating timeout to WaitForApplicationIdle Fix Value/Ref checks for pattern Index/Range (dotnet#35004) Fix assert in remove unused member analyzer Treat unconstrained type parameters declared in `#nullable disable` context as having an oblivious nullability in case they are substituted with a reference type. (dotnet#34797) Add symbol name to tests. Fix to be the correct name emitted PR feedback Improve IDE0052 diagnostic message for properties with used setter but unused getter Use original definition symbol for fetching control flow graph of generic local functions. Properly treat ambiguous explicit interface implementations (dotnet#34584) Remove the dependence between the order in NullableAnnotation and metadata attribute values (dotnet#34909) Fix warning level test. Fix bootstrap on Linux/Mac (dotnet#34978) disable completion for immediate window commands (dotnet#32631) Updated PreviewWarning color Implement design changes for "pattern" Index/Range indexers (dotnet#34897) Add IVTs to 16.1 version of RemoteLS ...

YairHalberstadt added 2 commits March 28, 2019 17:39

Properly treat ambiguous explicit interface implementations involving…

684fbd0

… nullable reference types, including maintaining backwards compatability with pre -NRT code.

Add extra tests

d846296

YairHalberstadt requested a review from a team as a code owner March 29, 2019 08:33

YairHalberstadt mentioned this pull request Mar 29, 2019

Explicit implementation of generic interface method with 'T?' parameter does not build #34508

Closed

YairHalberstadt added 2 commits March 29, 2019 08:45

Merge with master

6f73a80

Remove false comments about Runtime Checker being innaccurate

83bd505

jcouv added the Area-Compilers label Mar 29, 2019

gafter added Community The pull request was submitted by a contributor who is not a Microsoft employee. Feature - Nullable Reference Types Nullable Reference Types labels Mar 29, 2019

Simplify code for checking for duplicate runtime and compile time col…

84d86d3

…lision diagnostics.

YairHalberstadt mentioned this pull request Mar 31, 2019

Regression: Explicit Implementation fails to compile #34583

Closed

YairHalberstadt added 2 commits March 31, 2019 11:24

Add test for 34583

71d1a7e

Create Indexed Type Parameters in correct order in MemberSignatureCom…

bb669b3

…parer

YairHalberstadt commented Mar 31, 2019

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/MemberSignatureComparer.cs Outdated Show resolved Hide resolved

gafter self-assigned this Apr 3, 2019

YairHalberstadt added 3 commits April 4, 2019 07:37

Don't skip NullableReferenceTypesTests.Overriding_04

4c63489

Only make one pass in explicitImplementationHelpers

Treat T? in an override or explicit interface implementation as Nulla…

3609e87

…ble<T>

Fix unit test

4c662de

YairHalberstadt added 2 commits April 4, 2019 13:02

Skip one failing test, and fix another

94805e8

Skip Overriding_03 and add class constraint, and add equavalent test …

5f68cce

…without class constraint.

gafter approved these changes Apr 5, 2019

View reviewed changes

AlekseyTs reviewed Apr 8, 2019

View reviewed changes

AlekseyTs reviewed Apr 9, 2019

View reviewed changes

YairHalberstadt added 5 commits April 12, 2019 08:34

Changes based on review:

11216ff

Only force type parameters in method signature as nullable structs Revert changes to explicit inteface helper. Changes to unit tests

merged from master

36bd2b6

Fix some unit tests, and add some more.

26e57ae

rename tests

35b8be4

Added 1 more test to NullableReferenceTypeTests

b2c2aaf

AlekseyTs reviewed Apr 12, 2019

View reviewed changes

rename __defaultType to DefaultType

e260f51

AlekseyTs reviewed Apr 12, 2019

View reviewed changes

changes based on review

e8c1da3

AlekseyTs approved these changes Apr 12, 2019

View reviewed changes

AlekseyTs requested a review from gafter April 12, 2019 18:51

AlekseyTs reviewed Apr 12, 2019

View reviewed changes

YairHalberstadt and others added 3 commits April 13, 2019 21:35

fix assignment instead of equals

d0a3682

Remove accidental changes to NullableReferenceTypeTests

dcd838c

replace Foo with Goo

56a6aeb

gafter approved these changes Apr 15, 2019

View reviewed changes

AlekseyTs merged commit 8a6b522 into dotnet:master Apr 15, 2019

YairHalberstadt mentioned this pull request Apr 15, 2019

Implement where T: class and where T: struct for overrides and implementations #34798

Closed

jcouv added this to the 16.1.P3 milestone Apr 18, 2019

Conversation

YairHalberstadt commented Mar 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1.

2.

Uh oh!

AlekseyTs commented Mar 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

YairHalberstadt commented Mar 29, 2019

Uh oh!

Uh oh!

gafter commented Apr 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

YairHalberstadt commented Apr 3, 2019 • edited by gafter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

YairHalberstadt commented Apr 4, 2019 • edited by gafter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

YairHalberstadt commented Apr 4, 2019 • edited by gafter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gafter left a comment

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Apr 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Apr 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Apr 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YairHalberstadt commented Apr 11, 2019

Uh oh!

AlekseyTs commented Apr 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

YairHalberstadt commented Apr 12, 2019

Uh oh!

AlekseyTs commented Apr 12, 2019

Uh oh!

AlekseyTs Apr 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Apr 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs Apr 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Apr 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Apr 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

YairHalberstadt commented Mar 29, 2019 •

edited

Loading

AlekseyTs commented Mar 29, 2019 •

edited

Loading

gafter commented Apr 3, 2019 •

edited

Loading

YairHalberstadt commented Apr 3, 2019 •

edited by gafter

Loading

YairHalberstadt commented Apr 4, 2019 •

edited by gafter

Loading

YairHalberstadt commented Apr 4, 2019 •

edited by gafter

Loading

AlekseyTs Apr 8, 2019 •

edited

Loading

AlekseyTs Apr 9, 2019 •

edited

Loading

AlekseyTs Apr 9, 2019 •

edited

Loading

AlekseyTs commented Apr 11, 2019 •

edited

Loading

AlekseyTs Apr 12, 2019 •

edited

Loading

AlekseyTs commented Apr 12, 2019 •

edited

Loading

AlekseyTs Apr 12, 2019 •

edited

Loading

AlekseyTs Apr 12, 2019 •

edited

Loading

AlekseyTs Apr 12, 2019 •

edited

Loading

AlekseyTs Apr 12, 2019 •

edited

Loading