Use 'checked' names for synthesized vb intrinsic operators by CyrusNajmabadi · Pull Request #63604 · dotnet/roslyn

CyrusNajmabadi · 2022-08-25T22:53:26Z

This brings VB in line with C#, which is nice for consistency, and helps the effort to add an API to generate intrinsic operators from a compilation (#63186)

Draft to see what breaks.

src/Compilers/Test/Core/Compilation/TestOperationVisitor.cs

…slyn into vbCheckedNames

src/Compilers/VisualBasic/Test/Semantic/Semantics/UnaryOperators.vb

…rs.vb

src/Compilers/CSharp/Portable/Compilation/CSharpSemanticModel.cs

AlekseyTs · 2022-08-25T23:32:30Z

src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedIntrinsicOperatorSymbol.cs


        public SynthesizedIntrinsicOperatorSymbol(TypeSymbol leftType, string name, TypeSymbol rightType, TypeSymbol returnType, bool isCheckedBuiltin)
        {
+            Debug.Assert(SyntaxFacts.IsCheckedOperator(name) == isCheckedBuiltin);


SyntaxFacts.IsCheckedOperator(name) == isCheckedBuiltin

This assumption might not hold true for operators that do not have checked form. For cases like this, we should simply adjust the boolean to reflect the fact. Doing that here should be fine. #Closed

hrmm... that surprises me. The way we get the name at the callsite is to feed this same boolean into the helper that produces the name. So i would expect these to match.

basically, teh caller does this:

symbols = ImmutableArray.Create<Symbol>(new SynthesizedIntrinsicOperatorSymbol(objectType, OperatorFacts.BinaryOperatorNameFromOperatorKind(op, isChecked: binaryOperator.OperatorKind.IsChecked()), objectType, binaryOperator.Type, binaryOperator.OperatorKind.IsChecked()));

(or the unary equivalent). So the same binaryOperator.OperatorKind.IsChecked() feeds into determining the name and passing the isCheckedBuiltIn flag through.

We could even consider not passing the boolean in and just determining it from the name if that works for you.

We could even consider not passing the boolean in and just determining it from the name if that works for you.

Yes, that would work

Done.

Note: tehre was an interesting change here (but as this only affects tehse 'ide' symbols i think it's fine). Specifically, it used to be possible to get 'IsCheckedBuiltin=true' methods even for a non-checked method name. This would happen with virtually all dynamic operators, even for operators that have no checked version.

Now, by using hte name, the checked-ness or not is based only on the name alone. This led to some changes in test behavior, but i think it's acceptable/appropriate.

…slyn into vbCheckedNames

CyrusNajmabadi · 2022-08-26T02:46:01Z

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

-
-                default:
-                    expectChecked = type.IsDynamic();
-                    break;


now, no other op-types ever expect to be checked.

CyrusNajmabadi · 2022-08-26T02:46:06Z

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

-
-                default:
-                    isChecked = isDynamic;
-                    break;


now, no other op-types ever expect to be checked.

CyrusNajmabadi · 2022-08-26T02:46:27Z

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

+            var symbols2 = (from node2 in nodes select (IMethodSymbol)semanticModel.GetSymbolInfo(node2).Symbol).ToArray();
+            foreach (var symbol2 in symbols2)
+            {
+                Assert.False(symbol2.IsCheckedBuiltin);


these equality operator methods are not checked anymore.

CyrusNajmabadi · 2022-08-26T02:46:40Z

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

+
+            for (int i = 0; i < symbols1.Length; i++)
+            {
+                Assert.Equal(symbols1[i], symbols2[i]);


because they're not checked, they are equal to the non-checked versions always.

CyrusNajmabadi · 2022-08-26T02:46:57Z

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

+    void Test(dynamic x)
+    {
+        var z1 = x + 0;
+        var z2 = 0 + x;


i added this new test with dynamic and operators to show where 'checked' might actually appear.

src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedIntrinsicOperatorSymbol.cs

AlekseyTs · 2022-08-26T14:24:06Z

            Return _isCheckedBuiltin

Let's make it a calculated property, name is available.

In reply to: 1228555745

Refers to: src/Compilers/VisualBasic/Portable/Symbols/SynthesizedSymbols/SynthesizedIntrinsicOperatorSymbol.vb:168 in 7823890. [](commit_id = 7823890, deletion_comment = False)

AlekseyTs · 2022-08-26T14:25:10Z

        If _isCheckedBuiltin = other._isCheckedBuiltin AndAlso

It looks like this condition can be removed now.

In reply to: 1228557421

Refers to: src/Compilers/VisualBasic/Portable/Symbols/SynthesizedSymbols/SynthesizedIntrinsicOperatorSymbol.vb:78 in 7823890. [](commit_id = 7823890, deletion_comment = False)

src/Compilers/Test/Core/Compilation/TestOperationVisitor.cs

AlekseyTs · 2022-08-26T14:35:30Z

Done with review pass (commit 17)

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

333fred

LGTM (commit 19)

CyrusNajmabadi · 2022-08-27T01:36:46Z

@AlekseyTs ptal when you have time :)

AlekseyTs

LGTM (commit 19).Please squash commits in the process of merging.

CyrusNajmabadi · 2022-08-29T16:40:34Z

Please squash commits in the process of merging.

Will do.

…perators for the target language. Also ensure SymbolDisplay for VB can handle symbols for VB checked built-in operators after the recent change in dotnet#63604. This fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1615080.

…perators for the target language. (#64109) Also ensure SymbolDisplay for VB can handle symbols for VB checked built-in operators after the recent change in #63604. This fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1615080.

CyrusNajmabadi added 2 commits August 25, 2022 15:46

Use 'checked' names for synthesized vb operators

4f0d321

Exercise test paths

0549a16

ghost added the Area-Compilers label Aug 25, 2022

CyrusNajmabadi added 5 commits August 25, 2022 16:00

Fix

8a7b3c9

Fix

b918f3b

Is fixed

06d3616

Fix

c8c9ce6

formatting

89a596d

CyrusNajmabadi marked this pull request as ready for review August 25, 2022 23:18

CyrusNajmabadi requested a review from a team as a code owner August 25, 2022 23:18

CyrusNajmabadi commented Aug 25, 2022

View reviewed changes

src/Compilers/Test/Core/Compilation/TestOperationVisitor.cs Outdated Show resolved Hide resolved

Update src/Compilers/Test/Core/Compilation/TestOperationVisitor.cs

0e2f25a

CyrusNajmabadi commented Aug 25, 2022

View reviewed changes

src/Compilers/Test/Core/Compilation/TestOperationVisitor.cs Outdated Show resolved Hide resolved

CyrusNajmabadi and others added 3 commits August 25, 2022 16:20

Update src/Compilers/Test/Core/Compilation/TestOperationVisitor.cs

1b6fff3

Formatting

07f412b

Merge branch 'vbCheckedNames' of https://github.com/CyrusNajmabadi/ro…

219696f

…slyn into vbCheckedNames

CyrusNajmabadi commented Aug 25, 2022

View reviewed changes

src/Compilers/VisualBasic/Test/Semantic/Semantics/UnaryOperators.vb Outdated Show resolved Hide resolved

Update src/Compilers/VisualBasic/Test/Semantic/Semantics/UnaryOperato…

d355c30

…rs.vb

AlekseyTs reviewed Aug 25, 2022

View reviewed changes

src/Compilers/CSharp/Portable/Compilation/CSharpSemanticModel.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Aug 25, 2022

View reviewed changes

CyrusNajmabadi and others added 5 commits August 25, 2022 16:41

REvert c# file

fc0e85f

Merge branch 'vbCheckedNames' of https://github.com/CyrusNajmabadi/ro…

db1a8c4

…slyn into vbCheckedNames

Determine the checked state from the name, not an additional bool

a596329

Fix test

3266423

Update tests

7823890

CyrusNajmabadi commented Aug 26, 2022

View reviewed changes

CyrusNajmabadi requested review from 333fred and AlekseyTs August 26, 2022 02:47

AlekseyTs reviewed Aug 26, 2022

View reviewed changes

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

AlekseyTs reviewed Aug 26, 2022

View reviewed changes

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

AlekseyTs reviewed Aug 26, 2022

View reviewed changes

src/Compilers/Test/Core/Compilation/TestOperationVisitor.cs Outdated Show resolved Hide resolved

CyrusNajmabadi added 2 commits August 26, 2022 09:16

Move to a computed app

18d577a

Revert

648f802

CyrusNajmabadi requested a review from AlekseyTs August 26, 2022 16:17

333fred reviewed Aug 26, 2022

View reviewed changes

src/Compilers/CSharp/Test/Semantic/Semantics/OperatorTests.cs Show resolved Hide resolved

CyrusNajmabadi requested a review from 333fred August 26, 2022 18:34

333fred approved these changes Aug 26, 2022

View reviewed changes

AlekseyTs approved these changes Aug 29, 2022

View reviewed changes

CyrusNajmabadi merged commit f22d054 into dotnet:main Aug 29, 2022

CyrusNajmabadi deleted the vbCheckedNames branch August 29, 2022 16:40

ghost added this to the Next milestone Aug 29, 2022

allisonchou mentioned this pull request Aug 30, 2022

[Automated] PRs inserted in VS build main-32830.180 #63678

Closed

dibarbet modified the milestones: Next, 17.4 P2 Sep 1, 2022

AlekseyTs mentioned this pull request Sep 17, 2022

Improve SymbolDisplay to handle operator symbols that are not valid operators for the target language. #64109

Merged

Conversation

CyrusNajmabadi commented Aug 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AlekseyTs Aug 25, 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!

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!

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!

Uh oh!

Uh oh!

AlekseyTs commented Aug 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs commented Aug 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

AlekseyTs commented Aug 26, 2022

Uh oh!

Uh oh!

333fred left a comment

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Aug 27, 2022

Uh oh!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Aug 29, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

CyrusNajmabadi commented Aug 25, 2022 •

edited

Loading

AlekseyTs Aug 25, 2022 •

edited

Loading

AlekseyTs commented Aug 26, 2022 •

edited

Loading

AlekseyTs commented Aug 26, 2022 •

edited

Loading