Address a number of variance safety issues with code in interfaces. #39839

AlekseyTs · 2019-11-15T20:46:09Z

Declaration of classes, structures and enums within variant interfaces is disallowed.
Nested classes synthesized by the compiler don't have variant emitted type parameters.
Local functions and lambdas used within a variant interface always have display class.

agocke · 2019-11-15T23:17:47Z

src/Compilers/Core/Portable/PEWriter/InheritedTypeParameter.cs

@@ -56,7 +56,7 @@ public bool MustHaveDefaultConstructor

        public TypeParameterVariance Variance
        {
-            get { return _parentParameter.Variance; }
+            get { return _inheritingType.IsInterface || _inheritingType.IsDelegate ? _parentParameter.Variance : TypeParameterVariance.NonVariant; }


Not sure I understand this change. In what situation is the inheriting type not an interface or a delegate, but the parentParameter variance is variant, and that is incorrect? #Resolved

Not sure I understand this change. In what situation is the inheriting type not an interface or a delegate, but the parentParameter variance is variant, and that is incorrect?

A class nested into an interface (for example, a display class) is inheriting type parameters of the containing interface. Those type parameters could be variant, but only interfaces and delegates are allowed to have variant type parameters.

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

Got it, thanks. #Resolved

agocke · 2019-11-15T23:19:58Z

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

+            }
+        }
+
+        internal static NamedTypeSymbol GetEnclosingVariantInterface(Symbol member)


Should this return NamedTypeSymbol? since null is an intended part of the return type? #WontFix

Should this return NamedTypeSymbol? since null is an intended part of the return type?

Nullable Reference Types feature hasn't been enabled in this file yet.

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

agocke · 2019-11-15T23:23:24Z

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

@@ -52,10 +54,70 @@ internal static void CheckInterfaceVarianceSafety(this NamedTypeSymbol interface
                    case SymbolKind.Event:
                        CheckEventVarianceSafety((EventSymbol)member, diagnostics);
                        break;
+                    case SymbolKind.NamedType:
+                        CheckNestedTypeVarianceSafety((NamedTypeSymbol)member, diagnostics);


The intent here is to make any nested type inside a variant interface an error, right? #Resolved

The intent here is to make any nested type inside a variant interface an error, right?

Not any nested type, delegates and interfaces are Ok.

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

agocke

This covers all the things I can think of:

Local functions that would be put directly in the containing class because they only capture this or can take only ref struct environments.
Attempted optimization to merge environments, thus re-parenting the local function method.
Sharing another lambda's display class, which should be fine already.

LGTM

AlekseyTs · 2019-11-18T17:47:14Z

@dotnet/roslyn-compiler Please review, need second sign-off.

AlekseyTs · 2019-11-19T00:44:58Z

@gafter, @dotnet/roslyn-compiler Please review, need second sign-off.

gafter · 2019-11-19T18:57:17Z

src/Compilers/CSharp/Portable/Errors/ErrorCode.cs

@@ -1735,6 +1735,7 @@ internal enum ErrorCode
        #endregion diagnostics introduced for C# 8.0

        ERR_InternalError = 8751,
+        ERR_VarianceInterfaceNesting = 8752,


ERR_VarianceInterfaceNesting [](start = 8, length = 28)

I think this error code should be moved into the section of "diagnostics introduced for C# 8.0" #Closed

gafter · 2019-11-19T18:58:28Z

src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/LambdaRewriter.Analysis.cs

@@ -246,7 +246,12 @@ private void InlineThisOnlyEnvironments()
                        RemoveEnv();
                    }
                }
-                else
+                // If we are in a variant interface, runtime might not consider the 
+                // method synthesized directly within the interface as viriant safe.


viriant [](start = 71, length = 7)

spelling #Closed

gafter · 2019-11-19T18:59:25Z

src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/LambdaRewriter.Analysis.cs

-                    bool isStruct = true;
+
+                    // If we are in a variant interface, runtime might not consider the 
+                    // method synthesized directly within the interface as viriant safe.


viriant [](start = 75, length = 7)

spelling #Closed

gafter · 2019-11-19T19:00:21Z

src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/LambdaRewriter.cs

+                          originalMethod.MethodKind == MethodKind.LambdaMethod &&
+                          _analysis.MethodsConvertedToDelegates.Contains(originalMethod)) ||
+                         // If we are in a variant interface, runtime might not consider the 
+                         // method synthesized directly within the interface as viriant safe.


viriant [](start = 80, length = 7)

spelling #Closed

gafter · 2019-11-19T19:04:45Z

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

+
+        internal static NamedTypeSymbol GetEnclosingVariantInterface(Symbol member)
+        {
+            var container = member.ContainingType;


var [](start = 12, length = 3)

Consider writing
for (var container = member.ContainingType; container is object; container = container.ContainingType) #Closed

gafter · 2019-11-19T19:08:49Z

src/Compilers/CSharp/Test/Symbol/Symbols/DefaultInterfaceImplementationTests.cs

+@"M1
+M2");
+            }
+        }
    }
 }


It might be helpful to add a test that demonstrates that variance safety is enforced on private methods in an interface. #Closed

gafter

Done reviewing (Iteration 2).

gafter

Looks great! (Iteration 3)

Address a number of variance safety issues with code in interfaces.

4900f8d

Fixes dotnet#39731.

AlekseyTs added Language-C# Area-Compilers Feature - Default Interface Impl Default Interface Implementation labels Nov 15, 2019

AlekseyTs requested review from agocke and a team November 15, 2019 20:46

agocke reviewed Nov 15, 2019

View reviewed changes

Properly deal with ClosureKind.ThisOnly

32ebf30

agocke approved these changes Nov 16, 2019

View reviewed changes

gafter reviewed Nov 19, 2019

View reviewed changes

PR feedback

e97e15d

gafter approved these changes Nov 19, 2019

View reviewed changes

AlekseyTs merged commit c0e1da6 into dotnet:master Nov 20, 2019

AlekseyTs mentioned this pull request Mar 16, 2020

Default interface implementations, covariance and lambdas won't play together #42469

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Address a number of variance safety issues with code in interfaces. #39839

Address a number of variance safety issues with code in interfaces. #39839

AlekseyTs commented Nov 15, 2019 •

edited

Loading

agocke Nov 15, 2019 •

edited by AlekseyTs

Loading

AlekseyTs Nov 15, 2019

agocke Nov 16, 2019 •

edited by gafter

Loading

agocke Nov 15, 2019 •

edited by AlekseyTs

Loading

AlekseyTs Nov 15, 2019

agocke Nov 15, 2019 •

edited by AlekseyTs

Loading

AlekseyTs Nov 15, 2019

agocke left a comment

AlekseyTs commented Nov 18, 2019

AlekseyTs commented Nov 19, 2019

gafter Nov 19, 2019 •

edited

Loading

gafter Nov 19, 2019 •

edited

Loading

gafter Nov 19, 2019 •

edited

Loading

gafter Nov 19, 2019 •

edited

Loading

gafter Nov 19, 2019 •

edited

Loading

gafter Nov 19, 2019 •

edited

Loading

gafter left a comment

gafter left a comment

Address a number of variance safety issues with code in interfaces. #39839

Address a number of variance safety issues with code in interfaces. #39839

Conversation

AlekseyTs commented Nov 15, 2019 • edited Loading

agocke Nov 15, 2019 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Nov 15, 2019

Choose a reason for hiding this comment

agocke Nov 16, 2019 • edited by gafter Loading

Choose a reason for hiding this comment

agocke Nov 15, 2019 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Nov 15, 2019

Choose a reason for hiding this comment

agocke Nov 15, 2019 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Nov 15, 2019

Choose a reason for hiding this comment

agocke left a comment

Choose a reason for hiding this comment

AlekseyTs commented Nov 18, 2019

AlekseyTs commented Nov 19, 2019

gafter Nov 19, 2019 • edited Loading

Choose a reason for hiding this comment

gafter Nov 19, 2019 • edited Loading

Choose a reason for hiding this comment

gafter Nov 19, 2019 • edited Loading

Choose a reason for hiding this comment

gafter Nov 19, 2019 • edited Loading

Choose a reason for hiding this comment

gafter Nov 19, 2019 • edited Loading

Choose a reason for hiding this comment

gafter Nov 19, 2019 • edited Loading

Choose a reason for hiding this comment

gafter left a comment

Choose a reason for hiding this comment

gafter left a comment

Choose a reason for hiding this comment

AlekseyTs commented Nov 15, 2019 •

edited

Loading

agocke Nov 15, 2019 •

edited by AlekseyTs

Loading

agocke Nov 16, 2019 •

edited by gafter

Loading

agocke Nov 15, 2019 •

edited by AlekseyTs

Loading

agocke Nov 15, 2019 •

edited by AlekseyTs

Loading

gafter Nov 19, 2019 •

edited

Loading

gafter Nov 19, 2019 •

edited

Loading

gafter Nov 19, 2019 •

edited

Loading

gafter Nov 19, 2019 •

edited

Loading

gafter Nov 19, 2019 •

edited

Loading

gafter Nov 19, 2019 •

edited

Loading