Remove support for 'ref scoped' by jcouv · Pull Request #62788 · dotnet/roslyn

jcouv · 2022-07-20T15:46:48Z

Relates to test plan #59194

jcouv · 2022-07-22T15:39:42Z

@dotnet/roslyn-compiler for review please.

cston · 2022-07-22T16:23:53Z

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

@@ -541,14 +540,28 @@ internal static void CheckParameterModifiers(BaseParameterSyntax parameter, Bind
                    case SyntaxKind.ScopedKeyword when !parsingFunctionPointerParams:
                        ModifierUtils.CheckScopedModifierAvailability(parameter, modifier, diagnostics);
                        {


{

Minor: This enclosing block is no longer necessary. #Closed

cston · 2022-07-22T16:26:53Z

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

    static R F1(ref R r) { Console.WriteLine(r._i); return new R(); }
    static R F2(scoped ref R r) { Console.WriteLine(r._i); return new R(); }
-    static R F3(ref scoped R r) { Console.WriteLine(r._i); return new R(); }
-    static R F4(scoped ref R r) { Console.WriteLine(r._i); return new R(); }


static R F4(scoped ref R r) { Console.WriteLine(r._i); return new R(); }

Consider keeping this case and d4 below, to verify the synthesized delegate is shared between d2 and d4. #Closed

cston · 2022-07-22T16:31:19Z

public static implicit operator B<T>(in scoped A<T> a) => default;

Consider keeping this and changing the parameter to scoped in A<T> a, to ensure we're testing operators.

In reply to: 1192746687

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/RefFieldTests.cs:6692 in 95c778a. [](commit_id = 95c778a, deletion_comment = True)

cston · 2022-07-22T16:40:00Z

public void M4(scoped ref R<T> r) { }

Let's test ref R<T> r and scoped ref R<T> r pairs instead. In short, keep the M3() methods but change ref scoped to simply ref.

In reply to: 1192753712

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/RefFieldTests.cs:8741 in 95c778a. [](commit_id = 95c778a, deletion_comment = True)

cston · 2022-07-22T16:40:33Z

public abstract R<T> F3(ref scoped R<T> r);

Let's test ref rather than ref scoped.

In reply to: 1192754176

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/RefFieldTests.cs:8800 in 95c778a. [](commit_id = 95c778a, deletion_comment = True)

cston · 2022-07-22T16:40:57Z

R<T> F3(ref scoped R<T> r);

Let's test ref rather than ref scoped.

In reply to: 1192754503

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/RefFieldTests.cs:8860 in 95c778a. [](commit_id = 95c778a, deletion_comment = True)

cston · 2022-07-22T16:45:19Z

    var f = (ref scoped Unknown u) => { };

Change to scoped ref?

In reply to: 1192758031

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/RefFieldTests.cs:8968 in 95c778a. [](commit_id = 95c778a, deletion_comment = False)

cston · 2022-07-22T16:47:10Z

Extra blank line.

In reply to: 1192759346

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/RefFieldTests.cs:12301 in 95c778a. [](commit_id = 95c778a, deletion_comment = False)

cston · 2022-07-22T16:48:21Z

        var comp = CreateCompilationWithSpanAndMemoryExtensions(source);

CreateCompilation here and for other calls in this test.

In reply to: 1192760273

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/RefFieldTests.cs:12310 in 95c778a. [](commit_id = 95c778a, deletion_comment = False)

cston · 2022-07-22T16:50:52Z

src/Compilers/CSharp/Test/Symbol/SymbolDisplay/SymbolDisplayTests.cs

    static void M(R r0)
    {
-        scoped R r1;
-        ref readonly scoped R r2 = ref r1;


ref readonly scoped

We should include a ref readonly example. Perhaps change r3 to scoped ref readonly R r3; #Pending

cston · 2022-07-22T16:53:26Z

src/Compilers/VisualBasic/Test/Symbol/SymbolDisplay/SymbolDisplayTests.vb

-        scoped R r1;
-        ref readonly scoped R r2 = ref r1;
+        scoped R r1 = r0;
        scoped ref R r3 = ref r0;


scoped ref R r3

scoped ref readonly R r3 #Closed

jcouv · 2022-07-25T17:49:35Z

@dotnet/roslyn-compiler for second review. Thanks

cston · 2022-07-25T19:19:06Z

public void M3(ref scoped R<string> r) { } // 3

scoped ref?

In reply to: 1194511793

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/RefFieldTests.cs:8292 in 2d8194d. [](commit_id = 2d8194d, deletion_comment = False)

cston · 2022-07-25T19:20:01Z

public override object this[scoped R<string> r] { get { return null; } set { } } // 5

F3(scoped ref R<string> r) and F4(scoped ref R<string> r) instead?

In reply to: 1194512755

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/RefFieldTests.cs:8353 in 2d8194d. [](commit_id = 2d8194d, deletion_comment = False)

333fred · 2022-07-26T00:31:46Z

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

@@ -7343,9 +7076,33 @@ public void DelegateReturnTypeScope(LanguageVersion langVersion)
 ";
            var comp = CreateCompilation(source, parseOptions: TestOptions.Regular.WithLanguageVersion(langVersion));
            comp.VerifyEmitDiagnostics(
-                // (2,14): error CS0106: The modifier 'scoped' is not valid for this item
+                // (2,14): error CS0246: The type or namespace name 'scoped' could not be found (are you missing a using directive or an assembly reference?)


This is an extremely bad parse. I think we should be able to do better. #Resolved

333fred · 2022-07-26T00:33:51Z

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

        scoped {refModifier} scoped S s3 = ref s;
    }}
 }}";
            var comp = CreateCompilation(source);
            comp.VerifyDiagnostics(
-                // (6,29): error CS9048: The 'scoped' modifier can be used for refs and ref struct values only.
+                // (6,22): error CS0246: The type or namespace name 'scoped' could not be found (are you missing a using directive or an assembly reference?)


Same here. We forbid types named scoped in C# 11, right? This should be unambiguously a bad modifier, not a type name. #Resolved

333fred

Done review pass. I went through some of the tests, and I don't think the approach we're taking towards misplaced scoped is good. We should be able to recognize that it's a misplaced modifier and give a proper error, rather than dumping the user into a bad state. In particular, I expect people accidentally typing ref scoped instead of scoped ref to a very common failure (I still don't remember off the top of my head if it's ref readonly or readonly ref), so we need to do better for the error recovery here.

jcouv · 2022-07-26T17:53:02Z

docs/compilers/CSharp/Compiler Breaking Changes - DotNet 7.md


+## `scoped` treated as a contextual keyword
+
+***Introduced in Visual Studio 2022 version 17.4.*** Starting in C# 11, types cannot be named `scoped`. The compiler will report an error on all such type names. To work around this, the type name and all usages must be escaped with an `@`:


📝 the full implementation of this break will come shortly

cston · 2022-07-26T18:03:06Z

docs/compilers/CSharp/Compiler Breaking Changes - DotNet 7.md

@@ -1,5 +1,22 @@
 # This document lists known breaking changes in Roslyn after .NET 6 all the way to .NET 7.

+## `scoped` treated as a contextual keyword


Should this be "Types cannot be named scoped" instead, assuming that captures the break, for consistency with the changes for file and required? #Resolved

333fred · 2022-07-26T18:13:38Z

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

-                // ref struct scoped { }
-                Diagnostic(ErrorCode.WRN_LowerCaseTypeName, "scoped").WithArguments("scoped").WithLocation(5, 12));
+                // (4,15): warning CS0219: The variable 's4' is assigned but its value is never used
+                // scoped scoped s4 = default; // 2


Are we expecting an error on types named scoped in a future PR? Presumably what's happening is that the parser isn't treating the second scoped as a contextual keyword here? #Resolved

I don't expect a blanket error on referencing types names scoped. I only expect that anytime we're looking for scoped, we'll prefer the keyword interpretation to the identifier interpretation.
The behavior for scoped scoped s4 is an existing issue. Filed tracking issue: #62950

cston · 2022-07-26T18:19:25Z

src/Compilers/CSharp/Test/Syntax/Parsing/DeclarationScopeParsingTests.cs

+        public void Local_06(LanguageVersion langVersion)
+        {
+            string source =
+@"scoped.nested a;


scoped.nested

It seems odd that scoped.nested is treated as a type name in scoped.nested a; but not in ref scoped.nested b;. #Resolved

Filed follow-up issue: #62950

cston · 2022-07-26T18:23:16Z

src/Compilers/CSharp/Test/Syntax/Parsing/DeclarationScopeParsingTests.cs

            string source =
 @"using scoped s;
 using ref scoped r;
+using ref scoped r r2;


using ref scoped r r2

Covered in Using_02_RefScoped? #Resolved

cston · 2022-07-26T18:23:59Z

src/Compilers/CSharp/Test/Syntax/Parsing/DeclarationScopeParsingTests.cs

 @"await using scoped s;
+await using ref scoped;
 await using ref scoped r;
+await using ref scoped r r2;


Covered in Using_04_RefScoped? #Resolved

jcouv self-assigned this Jul 20, 2022

ghost added the Area-Compilers label Jul 20, 2022

jcouv force-pushed the ref-scoped branch 2 times, most recently from ffaac73 to a0c1933 Compare July 20, 2022 22:15

jcouv marked this pull request as ready for review July 21, 2022 04:52

jcouv requested review from a team as code owners July 21, 2022 04:52

jcouv requested a review from cston July 21, 2022 15:42

Remove support for 'ref scoped'

59b8e38

jcouv force-pushed the ref-scoped branch from a173e44 to 59b8e38 Compare July 21, 2022 15:46

Julien Couvreur added 2 commits July 21, 2022 08:49

comments

74280d4

Merge remote-tracking branch 'dotnet/main' into ref-scoped

95c778a

cston reviewed Jul 22, 2022

View reviewed changes

Address feedback from Chuck

2d8194d

jcouv requested a review from cston July 25, 2022 17:49

cston approved these changes Jul 25, 2022

View reviewed changes

tweak a couple tests

3233f64

333fred reviewed Jul 26, 2022

View reviewed changes

Address feedback from Fred

2a5a0ad

jcouv commented Jul 26, 2022

View reviewed changes

Merge remote-tracking branch 'dotnet/main' into ref-scoped

b111d9d

cston reviewed Jul 26, 2022

View reviewed changes

333fred reviewed Jul 26, 2022

View reviewed changes

cston reviewed Jul 26, 2022

View reviewed changes

Address feedback

4ba377d

jcouv requested a review from 333fred July 26, 2022 19:45

cston approved these changes Jul 26, 2022

View reviewed changes

jcouv added this to the 17.4 milestone Jul 26, 2022

333fred approved these changes Jul 26, 2022

View reviewed changes

333fred merged commit d81ce62 into dotnet:main Jul 26, 2022

jcouv deleted the ref-scoped branch July 26, 2022 22:32

ghost modified the milestones: 17.4, Next Jul 26, 2022

allisonchou modified the milestones: Next, 17.4 P1 Jul 26, 2022

ryzngard mentioned this pull request Jul 27, 2022

Enable new inline rename by default #62992

Merged

This was referenced Jul 28, 2022

[Automated] PRs inserted in VS build $((Release.Artifacts._DD-CB-TestSignVS.BuildNumber) #63032

Closed

[Automated] PRs inserted in VS build main-32727.216 #63034

Closed


		## `scoped` treated as a contextual keyword

		*Introduced in Visual Studio 2022 version 17.4.* Starting in C# 11, types cannot be named `scoped`. The compiler will report an error on all such type names. To work around this, the type name and all usages must be escaped with an `@`:

		@@ -1,5 +1,22 @@
		# This document lists known breaking changes in Roslyn after .NET 6 all the way to .NET 7.

		## `scoped` treated as a contextual keyword

Conversation

jcouv commented Jul 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv commented Jul 22, 2022

Uh oh!

cston Jul 22, 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!

cston Jul 22, 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!

cston commented Jul 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cston commented Jul 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cston commented Jul 22, 2022 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cston commented Jul 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cston commented Jul 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cston commented Jul 22, 2022 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cston commented Jul 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cston Jul 22, 2022 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Jul 22, 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 commented Jul 25, 2022

Uh oh!

cston commented Jul 25, 2022 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cston commented Jul 25, 2022 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

333fred Jul 26, 2022 • edited by jcouv 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 Jul 26, 2022 • edited by jcouv 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 left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv Jul 26, 2022

Choose a reason for hiding this comment

Uh oh!

cston Jul 26, 2022 • edited by jcouv 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 Jul 26, 2022 • edited by jcouv 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 Jul 26, 2022

Choose a reason for hiding this comment

Uh oh!

cston Jul 26, 2022 • edited by jcouv 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 commented Jul 20, 2022 •

edited

Loading

cston Jul 22, 2022 •

edited

Loading

cston Jul 22, 2022 •

edited

Loading

cston commented Jul 22, 2022 •

edited

Loading

cston commented Jul 22, 2022 •

edited

Loading

cston commented Jul 22, 2022 •

edited by jcouv

Loading

cston commented Jul 22, 2022 •

edited

Loading

cston commented Jul 22, 2022 •

edited

Loading

cston commented Jul 22, 2022 •

edited by jcouv

Loading

cston commented Jul 22, 2022 •

edited

Loading

cston Jul 22, 2022 •

edited by jcouv

Loading

cston Jul 22, 2022 •

edited

Loading

cston commented Jul 25, 2022 •

edited by jcouv

Loading

cston commented Jul 25, 2022 •

edited by jcouv

Loading

333fred Jul 26, 2022 •

edited by jcouv

Loading

333fred Jul 26, 2022 •

edited by jcouv

Loading

cston Jul 26, 2022 •

edited by jcouv

Loading

333fred Jul 26, 2022 •

edited by jcouv

Loading

cston Jul 26, 2022 •

edited by jcouv

Loading

cston Jul 26, 2022 •

edited by jcouv

Loading

cston Jul 26, 2022 •

edited by jcouv

Loading