Support declaration and consumption of user-defined Unsigned Right Shift operator. by AlekseyTs · Pull Request #60616 · dotnet/roslyn

AlekseyTs · 2022-04-07T01:10:14Z

https://github.com/dotnet/csharplang/blob/main/proposals/unsigned-right-shift-operator.md

Also added language version checks.

Test plan #60433

https://github.com/dotnet/csharplang/blob/main/proposals/unsigned-right-shift-operator.md

AlekseyTs · 2022-04-07T16:50:59Z

@jcouv, @333fred, @dotnet/roslyn-compiler Please review.

AlekseyTs · 2022-04-07T22:43:20Z

@jcouv, @333fred, @dotnet/roslyn-compiler Please review.

AlekseyTs · 2022-04-08T14:43:49Z

@jcouv, @333fred, @dotnet/roslyn-compiler Please review.

jcouv · 2022-04-08T16:43:51Z

Looking

jcouv · 2022-04-08T17:07:24Z

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

+        throw null;
+    }
+
+    public static int operator >>(C4 x, int y)


Consider adding a positive case for parameters being nullable: public static int operator >>>(S1? x, int? y) #Closed

jcouv · 2022-04-08T17:13:05Z

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

-                result &= CheckFeatureAvailability(node, MessageID.IDS_FeatureCheckedUserDefinedOperators, diagnostics);
+                Debug.Assert((methodOpt.Name == WellKnownMemberNames.UnsignedRightShiftOperatorName) == isUnsignedRightShift);
+
+                if (Compilation.SourceModule != methodOpt.ContainingModule)


I didn't understand why the unsigned right shift operator LangVer check is conditioned on resolved method being in source. I would have expected an unconditional LangVer diagnostic for >>> where ever the best signature is found. #Closed

I didn't understand why the unsigned right shift operator LangVer check is conditioned on resolved method being in source. I would have expected an unconditional LangVer diagnostic for >>> where ever the best signature is found.

This logic is not new

If the operator is declared in the same module (note, not just in source), we perform the check on the declaration and report the error, if needed. Reporting the same error at every consumption site would only add more noise.

jcouv · 2022-04-08T17:15:51Z

src/Compilers/CSharp/Portable/Parser/DocumentationCommentParser.cs

                                operatorToken.ValueText + operatorToken2.ValueText + operatorToken3.ValueText,
                                operatorToken3.GetTrailingTrivia());
+
+                            operatorToken = CheckFeatureAvailability(operatorToken, MessageID.IDS_FeatureUnsignedRightShift, forceWarning: true);


nit: Why do a parser LangVer check as opposed to add check in Binder_Cref? #Closed

Why do a parser LangVer check as opposed to add check in Binder_Cref?

Just following the established pattern.

Note that these are warnings, not errors

jcouv · 2022-04-08T17:16:45Z

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


                case WellKnownMemberNames.LeftShiftOperatorName:
                case WellKnownMemberNames.RightShiftOperatorName:
+                case WellKnownMemberNames.UnsignedRightShiftOperatorName:


nit: consider updating comment in CheckShiftSignature (add >>>) #Closed

jcouv

Done with review pass (iteration 2)

jcouv

LGTM Thanks (iteration 2)

AlekseyTs · 2022-04-11T15:36:20Z

@333fred, @dotnet/roslyn-compiler For the second review.

jcouv

LGTM Thanks (iteration 3)

333fred

Done review pass (commit 3)

333fred · 2022-04-11T20:32:26Z

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

        /// Returns false if reported an error, true otherwise.
        /// </summary>
-        private bool CheckConstraintLanguageVersionAndRuntimeSupportForOperator(SyntaxNode node, MethodSymbol? methodOpt, TypeSymbol? constrainedToTypeOpt, BindingDiagnosticBag diagnostics)
+        private bool CheckConstraintLanguageVersionAndRuntimeSupportForOperator(SyntaxNode node, MethodSymbol? methodOpt, bool isUnsignedRightShift, TypeSymbol? constrainedToTypeOpt, BindingDiagnosticBag diagnostics)


Consider just passing the operator kind, and letting the "is unsigned right shift" logic be contained within this method, rather than needing to be specified by a caller. #WontFix

333fred · 2022-04-11T20:45:33Z

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

+            // PROTOTYPE(UnsignedRightShift): This code was previously allowed. Confirm that we are Ok with this 
+            //                                breaking change and document it.  
+            compilation1.VerifyDiagnostics(
+                // (9,40): error CS0571: 'C1.operator >>>(C1, int)': cannot explicitly call operator or accessor


Would it be worth adding a version of this test without the specialname flag in the IL? #Pending

Would it be worth adding a version of this test without the specialname flag in the IL?

A method like that can be defined in plain C#.

333fred · 2022-04-11T21:00:10Z

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

+    }
+
+    static C1? Test1(C1? x, int? y) => x >>> y; 
+    static C1? Test2(C1? x, int? y) => x >> y; 


Unused? #Resolved

Unused?

I think it is used for IL comparison

333fred · 2022-04-11T21:00:55Z

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

+        return x;
+    }
+
+    static C1? Test2(C1? x, int? y)


Unused? #Resolved

Unused?

Used for IL comparison

AlekseyTs added 2 commits April 6, 2022 18:06

Support declaration and consumption of Unsigned Right Shift operator.

e804a5e

https://github.com/dotnet/csharplang/blob/main/proposals/unsigned-right-shift-operator.md

Add language version checks

b227fec

AlekseyTs added Area-Compilers Feature - UnsignedRightShift labels Apr 7, 2022

AlekseyTs requested review from 333fred and jcouv April 7, 2022 01:10

AlekseyTs requested a review from a team as a code owner April 7, 2022 01:10

jcouv self-assigned this Apr 8, 2022

jcouv reviewed Apr 8, 2022

View reviewed changes

jcouv approved these changes Apr 8, 2022

View reviewed changes

PR feedback

a419924

jcouv approved these changes Apr 11, 2022

View reviewed changes

333fred reviewed Apr 11, 2022

View reviewed changes

333fred approved these changes Apr 11, 2022

View reviewed changes

AlekseyTs merged commit a71a5cf into dotnet:features/UnsignedRightShift Apr 11, 2022

Conversation

AlekseyTs commented Apr 7, 2022 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs commented Apr 7, 2022

Uh oh!

AlekseyTs commented Apr 7, 2022

Uh oh!

AlekseyTs commented Apr 8, 2022

Uh oh!

jcouv commented Apr 8, 2022

Uh oh!

jcouv Apr 8, 2022 • 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!

jcouv Apr 8, 2022 • 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 8, 2022

Choose a reason for hiding this comment

Uh oh!

jcouv Apr 8, 2022 • 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 8, 2022

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Apr 8, 2022

Choose a reason for hiding this comment

Uh oh!

jcouv Apr 8, 2022 • 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!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Apr 11, 2022

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

333fred left a comment

Choose a reason for hiding this comment

Uh oh!

333fred Apr 11, 2022 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

333fred Apr 11, 2022 • edited by AlekseyTs 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 11, 2022

Choose a reason for hiding this comment

Uh oh!

333fred Apr 11, 2022 • edited by AlekseyTs 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 11, 2022

Choose a reason for hiding this comment

Uh oh!

333fred Apr 11, 2022 • edited by AlekseyTs 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 11, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

AlekseyTs commented Apr 7, 2022 •

edited by jcouv

Loading

jcouv Apr 8, 2022 •

edited

Loading

jcouv Apr 8, 2022 •

edited

Loading

jcouv Apr 8, 2022 •

edited

Loading

jcouv Apr 8, 2022 •

edited

Loading

333fred Apr 11, 2022 •

edited by AlekseyTs

Loading

333fred Apr 11, 2022 •

edited by AlekseyTs

Loading

333fred Apr 11, 2022 •

edited by AlekseyTs

Loading

333fred Apr 11, 2022 •

edited by AlekseyTs

Loading