Support declaring operators `+ - ! ~ ++ -- true false * / % & | ^ << >> > < >= <=` in interfaces. #20401

AlekseyTs · 2017-06-22T19:10:20Z

@dotnet/roslyn-compiler Please review.

…< >> > < >= <=``` in interfaces.

agocke · 2017-06-22T21:13:27Z

src/Compilers/CSharp/Portable/Binder/Semantics/Operators/UnaryOperatorOverloadResolution.cs

@@ -319,6 +320,36 @@ private bool GetUserDefinedOperators(UnaryOperatorKind kind, BoundExpression ope
                }
            }

+            // Look in base interfaces, or effective interfaces for type parameters  


A note above for the spec quote could be useful to indicate that it's no longer valid. #Resolved

agocke · 2017-06-22T21:16:26Z

src/Compilers/CSharp/Portable/Binder/Semantics/Operators/UnaryOperatorOverloadResolution.cs

+
+                    foreach (NamedTypeSymbol @interface in interfaces)
+                    {
+                        GetUserDefinedUnaryOperatorsFromType(@interface, kind, name, operators);


Why do we only look at the user defined operators for one base type at at time, where for interfaces we gather all user defined operators together? #Resolved

For simplicity, the overload resolution is going to do the right thing any way and each interface can define at most one unary operator of each kind. Tracking inheritance relationship in interfaces is not that trivial due to multiple inheritance. Effectively, the end result is going to be the same. #Resolved

agocke · 2017-06-22T21:20:44Z

src/Compilers/Core/Portable/InternalUtilities/ISetExtensions.cs

+            var result = false;
+            foreach (var v in values)
+            {
+                result |= set.Add(v);


If a set returns true only if the element was successfully added, wouldn't you expect AddAll to only return true if all elements were successfully added?

I am not trying to redefine the result, just adding a more efficient overload for an ImmutableArray. Either behavior would be fine, depending on consumer's needs. For the purposes of this PR, I do not care what is the result.

Even if you don't use the result, I think the proper result behavior is to return true if and only if all items were added successfully and false otherwise. That way others can use this helper and rely upon the result.

I see the overload now, nevermind :) 👍

agocke · 2017-06-22T21:21:09Z

src/Compilers/Core/Portable/InternalUtilities/ISetExtensions.cs

+            var result = false;
+            foreach (var v in values)
+            {
+                result |= set.Remove(v);


Same question about result behavior, as above.

Same answer.

AlekseyTs · 2017-06-22T23:25:56Z

https://ci.dot.net/job/dotnet_roslyn/job/features_DefaultInterfaceImplementation/job/windows_debug_vs-integration_prtest/42/
https://ci.dot.net/job/dotnet_roslyn/job/features_DefaultInterfaceImplementation/job/windows_release_vs-integration_prtest/47/

jcouv · 2017-06-23T04:59:44Z

src/Compilers/CSharp/Portable/CSharpResources.resx

-  <data name="ERR_InterfacesCantContainOperators" xml:space="preserve">
-    <value>Interfaces cannot contain operators</value>
+  <data name="ERR_InterfacesCantContainConversionOrEqualityOperators" xml:space="preserve">
+    <value>Interfaces cannot contain conversion, equality, or inequality operators</value>


Looking at test Operators_01, it seems that inequality operators are supported. Maybe the error message is out-of-date? #Closed

Looking at test Operators_01, it seems that inequality operators are supported. Maybe the error message is out-of-date?

This test doesn't test unsupported operators. Neither conversion, nor equality/inequality. #Closed

I got confused thinking that inequality was <= and related...

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

jcouv · 2017-06-23T05:07:48Z

docs/features/DefaultInterfaceImplementation.md

@@ -51,6 +51,8 @@ class Test1 : I1

 - Declaring static fields, auto-properties and field-like events (**protected** modifier is not allowed).

+- Declaring operators ```+ - ! ~ ++ -- true false * / % & | ^ << >> > < >= <=``` in interfaces.


Is the intention to support equality later on? If not, maybe add a note as to why it is not allowed.

Is the intention to support equality later on? If not, maybe add a note as to why it is not allowed.

This is a topic for LDM discussion. The list is meant to simply enumerate what is supported by the prototype without any reasoning, and it doesn't have to match the proposed spec.

jcouv · 2017-06-23T05:11:50Z

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

+                Diagnostic(ErrorCode.ERR_ConcreteMissingBody, "+").WithArguments("IA.operator +(int, int)").WithLocation(4, 17),
+                // (4,17): error CS0563: One of the parameters of a binary operator must be the containing type
+                //    int operator +(int aa, int bb);   // CS0567
+                Diagnostic(ErrorCode.ERR_BadBinaryOperatorSignature, "+").WithLocation(4, 17)


The diagnostics offer solutions that will fail (because LangVer=7). Seems confusing.
Maybe some of the messages could include a required version ("Please use version X or greater"), conditional on LangVer?
Could be follow-up issue.

The diagnostics offer solutions that will fail (because LangVer=7). Seems confusing.
Maybe some of the messages could include a required version ("Please use version X or greater"), conditional on LangVer?

No these are two unrelated errors and each has distinct fix. I do not intend to bundle them together now, or in the future. #Resolved

To clarify, we don't complain about language version because we complain about missing body. This approach is consistent with other kinds of non-virtual members in interfaces. Basically, if whatever you have is invalid regardless of the version, we might not complain about version mismatch as a measure of suppressing the noise. #Resolved

cston · 2017-06-23T15:53:29Z

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

+                    {
+                        Binder.CheckFeatureAvailability(node, MessageID.IDS_DefaultInterfaceImplementation, diagnostics);
+                    }
+                }


Consider extracting a helper method for use here and in BinaryOperatorOverloadResolution. #Resolved

Consider extracting a helper method for use here and in BinaryOperatorOverloadResolution.

Done. #Resolved

jcouv

LGTM

cston · 2017-06-23T20:50:11Z

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

                        case MethodKind.UserDefinedOperator:
-                            diagnostics.Add(ErrorCode.ERR_InterfacesCantContainOperators, member.Locations[0]);
+                            if (meth.Name == WellKnownMemberNames.EqualityOperatorName || meth.Name == WellKnownMemberNames.InequalityOperatorName)


Consider extracting name checks to helper method since the same checks occur in multiple places.

cston · 2017-06-23T21:20:13Z

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

+            CompilationReference compilationReference = compilation1.ToMetadataReference();
+            MetadataReference metadataReference = compilation1.EmitToImageReference();
+
+


Are we testing shadowing does not depend on the order of base interfaces in the derived interface declaration?

interface IA { public static IA operator+(IA x, IB y) => null; } interface IB : IA { public static IA operator+(IB x, IA y) => null; } interface IC1 : IA, IB { } interface IC2 : IB, IA { }

I'll check and add an explicit test for a scenario like that in the next PR.

Support declaring operators ```+ - ! ~ ++ -- true false * / % & | ^ <…

c6727ea

…< >> > < >= <=``` in interfaces.

AlekseyTs added Area-Compilers Language-C# Feature - Default Interface Impl Default Interface Implementation labels Jun 22, 2017

dnfclas added the cla-already-signed label Jun 22, 2017

Add better ImmutableArray overloads to ISetExtensions.

ad459e6

agocke reviewed Jun 22, 2017

View reviewed changes

agocke approved these changes Jun 23, 2017

View reviewed changes

CR feedback

602d508

jcouv reviewed Jun 23, 2017

View reviewed changes

cston reviewed Jun 23, 2017

View reviewed changes

Extract common helper

7a54cfa

jcouv approved these changes Jun 23, 2017

View reviewed changes

cston reviewed Jun 23, 2017

View reviewed changes

AlekseyTs merged commit 12c5fec into dotnet:features/DefaultInterfaceImplementation Jun 23, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support declaring operators `+ - ! ~ ++ -- true false * / % & | ^ << >> > < >= <=` in interfaces. #20401

Support declaring operators `+ - ! ~ ++ -- true false * / % & | ^ << >> > < >= <=` in interfaces. #20401

AlekseyTs commented Jun 22, 2017

agocke Jun 22, 2017 •

edited by jcouv

Loading

agocke Jun 22, 2017 •

edited by jcouv

Loading

AlekseyTs Jun 22, 2017 •

edited by jcouv

Loading

agocke Jun 22, 2017

AlekseyTs Jun 22, 2017

agocke Jun 23, 2017

agocke Jun 23, 2017

agocke Jun 22, 2017

AlekseyTs Jun 22, 2017

AlekseyTs commented Jun 22, 2017

jcouv Jun 23, 2017 •

edited

Loading

AlekseyTs Jun 23, 2017 •

edited by jcouv

Loading

jcouv Jun 23, 2017

jcouv Jun 23, 2017

AlekseyTs Jun 23, 2017

jcouv Jun 23, 2017

AlekseyTs Jun 23, 2017 •

edited by jcouv

Loading

AlekseyTs Jun 23, 2017 •

edited by jcouv

Loading

cston Jun 23, 2017 •

edited by jcouv

Loading

AlekseyTs Jun 23, 2017 •

edited by jcouv

Loading

jcouv left a comment

cston Jun 23, 2017

cston Jun 23, 2017

AlekseyTs Jun 23, 2017

		@@ -51,6 +51,8 @@ class Test1 : I1

		- Declaring static fields, auto-properties and field-like events (protected modifier is not allowed).

		- Declaring operators ```+ - ! ~ ++ -- true false * / % & \| ^ << >> > < >= <=``` in interfaces.

		CompilationReference compilationReference = compilation1.ToMetadataReference();
		MetadataReference metadataReference = compilation1.EmitToImageReference();

Support declaring operators + - ! ~ ++ -- true false * / % & | ^ << >> > < >= <= in interfaces. #20401

Support declaring operators + - ! ~ ++ -- true false * / % & | ^ << >> > < >= <= in interfaces. #20401

Conversation

AlekseyTs commented Jun 22, 2017

agocke Jun 22, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

agocke Jun 22, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

AlekseyTs Jun 22, 2017 • edited by jcouv 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Jun 22, 2017

jcouv Jun 23, 2017 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jun 23, 2017 • edited by jcouv 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

Choose a reason for hiding this comment

AlekseyTs Jun 23, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

AlekseyTs Jun 23, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

cston Jun 23, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

AlekseyTs Jun 23, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv left a comment

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

Support declaring operators `+ - ! ~ ++ -- true false * / % & | ^ << >> > < >= <=` in interfaces. #20401

Support declaring operators `+ - ! ~ ++ -- true false * / % & | ^ << >> > < >= <=` in interfaces. #20401

agocke Jun 22, 2017 •

edited by jcouv

Loading

agocke Jun 22, 2017 •

edited by jcouv

Loading

AlekseyTs Jun 22, 2017 •

edited by jcouv

Loading

jcouv Jun 23, 2017 •

edited

Loading

AlekseyTs Jun 23, 2017 •

edited by jcouv

Loading

AlekseyTs Jun 23, 2017 •

edited by jcouv

Loading

AlekseyTs Jun 23, 2017 •

edited by jcouv

Loading

cston Jun 23, 2017 •

edited by jcouv

Loading

AlekseyTs Jun 23, 2017 •

edited by jcouv

Loading