Relax ordering constraints for parameter modifiers #23643

alrz · 2017-12-07T19:56:34Z

Customer scenario

We allow ref this in ref extension methods but not the other way around. That's wrong! We can't disallow ref this, but we should allow and prefer this ref.

Note: this is a bug fix change to C# 7.2 that would ship in 15.6, per discussion with LDM.

Bugs this fixes

Fixes dotnet/csharplang#1022 (per LDM 2017-12-04)

Workarounds, if any

Type ref this instead of this ref.

Risk

Performance impact

TBD

Is this a regression from a previous update?

No. This restriction was introduced when the ref extension method was implemented in C# 7.2 (15.5).

Root cause analysis

How was the bug found?

Reported by customer.

jcouv

LGTM Thanks

jcouv · 2017-12-09T20:38:52Z

Tagging @VSadov as FYI. Although we hadn't discussed in (only ref), the same relaxation on ordering makes sense for in too (as this PR implements).

AlekseyTs · 2017-12-11T18:26:02Z

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

@@ -1517,7 +1517,7 @@ internal enum ErrorCode
        ERR_TypeReserved = 8336,
        ERR_RefExtensionMustBeValueTypeOrConstrainedToOne = 8337,
        ERR_InExtensionMustBeValueType = 8338,
-        ERR_BadParameterModifiersOrder = 8339,
+        // ERR_BadParameterModifiersOrder = 8339,


// ERR_BadParameterModifiersOrder = 8339, [](start = 8, length = 41)

I would expect the error message to be removed from the .resx file

I would also add a small comment to explain why it was removed.

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

AlekseyTs · 2017-12-11T18:36:09Z

I would expect to see some tests that verify that the different order of modifiers is actually properly treated by the compiler, i.e. the parameter is actually treated as ref/in by the compiler in addition to not reporting the error.

AlekseyTs · 2017-12-11T18:36:46Z

Done with review pass (iteration 1).

VSadov · 2017-12-11T18:53:07Z

Yes, it applies equally to ‘ref’ and ‘in’ extensions.

VSadov · 2017-12-11T18:56:19Z

Also need some tests for scenarios with possibly new behavior like:
“ref this ref”,
“ref this in”

Still an error, but for a different reason.

OmarTawfik · 2017-12-18T20:35:02Z

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

-                // (10,36): error CS8339: The parameter modifier 'in' cannot be used after the modifier 'this'
-                // public static void Method4<T>(this in int i) { }
-                Diagnostic(ErrorCode.ERR_BadParameterModifiersOrder, "in").WithArguments("in", "this").WithLocation(10, 36));
+            CreateCompilationWithMscorlibAndSystemCore(test).GetDeclarationDiagnostics().Verify();
        }


I'd use compilation.VerifyDiagnostics() for completeness. #Closed

I'd also change the name of the test.

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

OmarTawfik · 2017-12-18T20:38:03Z

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

                Diagnostic(ErrorCode.ERR_BadExtensionAgg, "GenExtensions").WithLocation(12, 21));
        }

+        [Fact]
+        public void BadParameterModifiers()


BadParameterModifiers [](start = 20, length = 21)

BadParameterModifiers [](start = 20, length = 21)

Can we change the test name to describe which modifiers are bad? #Closed

I suggest adding the opposite test scenarios as well:

Testing we produce an error on in this ref int i

Testing we produce an error on in this in int i

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

OmarTawfik · 2017-12-18T20:40:39Z

    public void BadParameterModifiers_ThisWithRef()

Can we rename that test to be more descriptive? this is now testing producing errors on the containing type. #Closed

Refers to: src/Compilers/CSharp/Test/Syntax/Parsing/ParserErrorMessageTests.cs:3087 in e107ed1. [](commit_id = e107ed1, deletion_comment = False)

OmarTawfik · 2017-12-18T20:43:00Z

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

@@ -1965,6 +1965,26 @@ public static class Ext
            Assert.Equal(RefKind.In, symbol.RefKind);
        }

+        [Fact]
+        public void ParameterSymbolsRetrievedThroughSemanticModel_ModifierOrdering()


ParameterSymbolsRetrievedThroughSemanticModel_ModifierOrdering [](start = 20, length = 62)

ParameterSymbolsRetrievedThroughSemanticModel_ModifierOrdering [](start = 20, length = 62)

This title is misleading. The symbols are not retrieved through the semantic model. I suggest following what ParameterSymbolsRetrievedThroughSemanticModel_InExtensionMethods do. I also suggest separating this in and this ref. #Closed

That test is still useful, but I suggest moving it to the Symbols test project, and use CompileAndVerify() helper, making use of the symbolValidator and sourceSymbolValidator parameters to check for the symbols.

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

OmarTawfik

Thanks @alrz for taking care of this!
Left a few comments.

alrz · 2017-12-19T12:33:44Z

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

@@ -3505,7 +3458,7 @@ public class TestType

        [Fact]
        [CompilerTrait(CompilerFeature.ReadOnlyReferences)]
-        public void BadInWithThisParameterModifiers()
+        public void InWithThis_ParameterModifiers()


We don't need this test now. VerifyDiagnostics is called on the same combination of modifiers elsewhere.
But if we do want to keep this, it should be moved to another file.

OmarTawfik

OmarTawfik · 2017-12-21T21:33:52Z

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

+            }
+
+            CompileAndVerify(source, validator: Validator, options: TestOptions.ReleaseDll).VerifyDiagnostics();
+        }


nit: for these two methods, CompileAndVerify will throw if there are any errors, so the trailing VerifyDiagnostics() is not needed.
However, you might want to make these tests actually print something through the extension methods, and use expectedOutput: "XX" parameter to verify the result.

jcouv · 2018-01-02T17:21:22Z

@MeiChin-Tsai for ask-mode approval for 15.6. Thanks

MeiChin-Tsai · 2018-01-04T21:50:21Z

approved.

alrz mentioned this pull request Dec 7, 2017

Relax ordering constraints around 'ref' and 'partial' modifiers #23533

Closed

jcouv added this to the 15.6 milestone Dec 8, 2017

jcouv added the Area-Compilers label Dec 8, 2017

jcouv requested a review from OmarTawfik December 8, 2017 23:55

jcouv approved these changes Dec 9, 2017

View reviewed changes

AlekseyTs reviewed Dec 11, 2017

View reviewed changes

alrz force-pushed the mod-ordering-extensions branch from 640fadb to f4a4534 Compare December 13, 2017 22:05

OmarTawfik reviewed Dec 18, 2017

View reviewed changes

OmarTawfik self-assigned this Dec 18, 2017

OmarTawfik reviewed Dec 18, 2017

View reviewed changes

alrz added 5 commits December 19, 2017 15:33

Relax ordering constraints for parameter modifiers

5df471f

Remove the error message from .resx file

b0139f3

Add tests

8bcfe6b

Add comment for removed error

10e7f4b

Add symbol tests

aaff6e9

alrz force-pushed the mod-ordering-extensions branch from f4a4534 to 57cc5bc Compare December 19, 2017 12:18

alrz added 3 commits December 19, 2017 15:51

Add semantic model tests

48c0cf1

Fix up parsing tests

30c3800

Update resources

547f4fe

alrz force-pushed the mod-ordering-extensions branch from 57cc5bc to 547f4fe Compare December 19, 2017 12:22

Add missing Fact attribute

e107ed1

alrz commented Dec 19, 2017

View reviewed changes

OmarTawfik approved these changes Dec 21, 2017

View reviewed changes

OmarTawfik reviewed Dec 21, 2017

View reviewed changes

jcouv mentioned this pull request Dec 22, 2017

Wrong order of 'in' and 'this' in extension method tooltip #23895

Closed

PR feedback

dafa9be

jcouv added the Pending director approval label Jan 2, 2018

jcouv merged commit adbce76 into dotnet:master Jan 4, 2018

alrz deleted the mod-ordering-extensions branch January 4, 2018 23:10

jcouv added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Jan 9, 2018

drewnoakes mentioned this pull request Mar 30, 2018

Introduce ArgumentBuilder gitextensions/gitextensions#4662

Merged

wtgodbe mentioned this pull request May 23, 2018

Remove msbuild dependency from buildtools dotnet/buildtools#2048

Merged

KalleOlaviNiemitalo mentioned this pull request Nov 22, 2024

Grammar rule *parameter_modifier* disallows valid V7 constructs dotnet/csharpstandard#1209

Open

Relax ordering constraints for parameter modifiers #23643

Relax ordering constraints for parameter modifiers #23643

Uh oh!

Conversation

alrz commented Dec 7, 2017 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Customer scenario

Bugs this fixes

Workarounds, if any

Risk

Performance impact

Is this a regression from a previous update?

Root cause analysis

How was the bug found?

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv commented Dec 9, 2017

Uh oh!

AlekseyTs Dec 11, 2017

Choose a reason for hiding this comment

Uh oh!

OmarTawfik Dec 18, 2017

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Dec 11, 2017

Uh oh!

AlekseyTs commented Dec 11, 2017

Uh oh!

VSadov commented Dec 11, 2017

Uh oh!

VSadov commented Dec 11, 2017

Uh oh!

OmarTawfik Dec 18, 2017 • 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!

OmarTawfik Dec 18, 2017

Choose a reason for hiding this comment

Uh oh!

OmarTawfik Dec 18, 2017 • 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!

OmarTawfik Dec 18, 2017

Choose a reason for hiding this comment

Uh oh!

OmarTawfik commented Dec 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

OmarTawfik Dec 18, 2017 • 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!

OmarTawfik Dec 18, 2017 • 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!

OmarTawfik left a comment

Choose a reason for hiding this comment

Uh oh!

alrz Dec 19, 2017

Choose a reason for hiding this comment

Uh oh!

OmarTawfik left a comment

Choose a reason for hiding this comment

Uh oh!

OmarTawfik Dec 21, 2017

Choose a reason for hiding this comment

Uh oh!

jcouv commented Jan 2, 2018

Uh oh!

MeiChin-Tsai commented Jan 4, 2018

Uh oh!

Uh oh!

alrz commented Dec 7, 2017 •

edited by jcouv

Loading

OmarTawfik Dec 18, 2017 •

edited

Loading

OmarTawfik Dec 18, 2017 •

edited

Loading

OmarTawfik commented Dec 18, 2017 •

edited

Loading

OmarTawfik Dec 18, 2017 •

edited

Loading

OmarTawfik Dec 18, 2017 •

edited

Loading