OHI tests for ref readonly #17547

OmarTawfik · 2017-03-04T00:22:08Z

Related to #17287

OmarTawfik · 2017-03-06T02:15:16Z

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

-                Diagnostic(ErrorCode.ERR_ParamUnassigned, "M").WithArguments("t"));
-
-            var ns = comp.SourceModule.GlobalNamespace.GetMembers("NS").Single() as NamespaceSymbol;
-            // TODO...


Is this still needed? #Closed

OmarTawfik · 2017-03-06T02:15:35Z

@VSadov @dotnet/roslyn-compiler #Resolved

cston · 2017-03-06T15:58:35Z

src/Compilers/CSharp/Portable/CSharpResources.resx

@@ -5005,4 +5002,7 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
  <data name="ERR_AttributesInLocalFuncDecl" xml:space="preserve">
    <value>Attributes are not allowed on local function parameters or type parameters</value>
  </data>
+  <data name="IDS_SK_CONSTRUCTOR" xml:space="preserve">
+    <value>constructor</value>
+  </data>


Perhaps move beside other IDS_SK_ items, line 280 or so. #Resolved

cston · 2017-03-06T16:00:56Z

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

@@ -130,6 +130,7 @@ internal enum MessageID

        IDS_FeatureReadonlyReferences = MessageBase + 12718,

+        IDS_SK_CONSTRUCTOR = MessageBase + 12719,


Group with other IDS_SK_ values in the file and use value in the same range, say = MessageBase + 2013. #Resolved

cston · 2017-03-06T16:42:12Z

src/Compilers/CSharp/Test/Emit/CodeGen/SwitchTests.cs

@@ -5084,7 +5084,7 @@ internal enum ErrorCode
            ERR_ExpressionTreeContainsBadCoalesce = 845,
            ERR_ArrayInitializerExpected = 846,
            ERR_ArrayInitializerIncorrectLength = 847,
-            ERR_OverloadRefOutCtor = 851,
+            ERR_OverloadRefKindCtor = 851,


Minor point: The original enum value did not change. #WontFix

Yes. Is that needed? Just refactoring the name to indicate it is used now for all ref kinds instead of ref/out.

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

This instance of ErrorCode should probably match the original ErrorCode. In other words, we should rename both instances of ERR_OverloadRefOutCtor or neither. #Resolved

Ah. Good point. Will fix now.

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

cston · 2017-03-06T16:46:08Z

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

+
+        [Fact]
+        [CompilerTrait(CompilerFeature.ReadonlyReferences)]
+        public void OverloadsWithDifferentParameterModifiersShouldErrorOut_Ref_RefReadOnly()


Consider dropping ShouldErrorOut, WillErrorOut, etc. from test method names, for consistency with other tests. #Resolved

cston · 2017-03-06T16:48:16Z

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

+{
+    public override void Method1(ref readonly int x) { }
+    public override void Method2(ref int x) { }
+}";


Include properties. #Resolved

cston · 2017-03-06T16:49:55Z

Please test interface implementations, if interfaces are also covered by this PR. #Resolved

OmarTawfik · 2017-03-06T20:46:48Z

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

+                Diagnostic(ErrorCode.ERR_OverrideNotExpected, "Method1").WithArguments("ChildClass.Method1(ref readonly int)").WithLocation(9, 26));
+        }
+    }
+}


Note to self: add tests for method return type mismatch by refness #Resolved

OmarTawfik · 2017-03-07T00:01:42Z

Latest version has:

Responded to PR Comments
Tests for properties.
Tests for interfaces.
Test fixes for other places where we're using unlocalized error strings.

Please let me know if you've further guidance on the wording of the error messages.
/cc @VSadov @cston @dotnet/roslyn-compiler #Resolved

cston · 2017-03-07T15:58:36Z

src/Compilers/CSharp/Portable/CSharpResources.resx

-  <data name="ERR_CantChangeRefReturnOnOverride" xml:space="preserve">
-    <value>'{0}' must {2}return by reference to match overridden member '{1}'</value>
+  <data name="ERR_CantChangeRefnessOfReturnOnOverride" xml:space="preserve">
+    <value>Reference signature of the return type of '{0}' does not match overridden member '{1}'</value>


Reference signature sounds like a new term, and we should avoid adding terminology if not necessary. Perhaps"'{0}' must match by reference return of overridden member '{1}'". #Resolved

Sounds good.

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

cston · 2017-03-07T15:59:20Z

src/Compilers/CSharp/Portable/CSharpResources.resx

@@ -4729,8 +4729,8 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
  <data name="ERR_RefPropertyCannotHaveSetAccessor" xml:space="preserve">
    <value>Properties which return by reference cannot have set accessors</value>
  </data>
-  <data name="ERR_CantChangeRefReturnOnOverride" xml:space="preserve">
-    <value>'{0}' must {2}return by reference to match overridden member '{1}'</value>
+  <data name="ERR_CantChangeRefnessOfReturnOnOverride" xml:space="preserve">


Rename of ERR_CantChangeRefReturnOnOverride seems unnecessary. #Resolved

Because it might be "cannot change ref readonly return" as well. Need to indicate that this involves other kinds of refness as well.

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

To me, both names capture that equally. #Resolved

Alright. Will change it back. ESL here :)

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

cston · 2017-03-07T16:00:50Z

src/Compilers/CSharp/Portable/CSharpResources.resx

@@ -4742,7 +4742,7 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
    <value>The return expression must be of type '{0}' because this method returns by reference</value>
  </data>
  <data name="ERR_CloseUnimplementedInterfaceMemberWrongRefReturn" xml:space="preserve">
-    <value>'{0}' does not implement interface member '{1}'. '{2}' cannot implement '{1}' because it does not return by {3}</value>
+    <value>'{0}' does not implement interface member '{1}'. '{2}' cannot implement '{1}' because it does not have the matching return type reference signature.</value>


Perhaps "... does not have matching return by reference". #Resolved

Sounds good.

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

cston · 2017-03-07T18:23:49Z

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

+{
+    public virtual void Method(ref int x) { }
+    public virtual void Method(ref readonly int x) { }
+}";


Consider removing abstract and virtual so it's clear the overload error does not depend on virtual methods. Same comment for test below. #Resolved

cston · 2017-03-07T18:28:43Z

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

+
+        [Fact]
+        [CompilerTrait(CompilerFeature.ReadonlyReferences)]
+        public void HidingMethodWithRefReadOnlyParameterWillProduceAWarning()


Consider dropping Will and WillNotProduceAWarning suffixes, for consistency with other tests. (The tests themselves indicate whether a warning is produced.) #Resolved

cston · 2017-03-07T18:37:40Z

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

+                // (6,11): error CS0535: 'B' does not implement interface member 'A.M(ref readonly int)'
+                // class B : A
+                Diagnostic(ErrorCode.ERR_UnimplementedInterfaceMember, "A").WithArguments("B", "A.M(ref readonly int)").WithLocation(6, 11));
+        }


Are we testing different ref return type, for methods and properties? #Resolved

Yes.

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

cston · 2017-03-07T18:42:47Z

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

+                //     public override ref readonly int Property1 { get { return ref x; } }
+                Diagnostic(ErrorCode.ERR_CantChangeRefnessOfReturnOnOverride, "Property1").WithArguments("B.Property1", "A.Property1").WithLocation(10, 38));
+        }
+    }


Are ref readonly parameters supported for indexers? object this[ref readonly int i] { get; set; } #Resolved

Will add tests for both indexers parameters and return types.

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

cston · 2017-03-07T18:51:22Z

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

+            var bClass = comp.GlobalNamespace.GetMember<NamedTypeSymbol>("B");
+
+            var aMethod = aClass.GetMember<MethodSymbol>("M");
+            var bMethod = bClass.GetMember<MethodSymbol>("M");


Minor point: Can use var aMethod = comp.GetMember<MethodSymbol>("A.M"); in each case. #Resolved

cston · 2017-03-07T18:58:24Z

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

+class B : A
+{
+    public ref readonly int M() { return ref x; }
+}";


Consider testing the warning is produced, even when ref return is different. #Resolved

cston · 2017-03-07T23:41:46Z

LGTM

OmarTawfik · 2017-03-08T01:53:58Z

@dotnet/roslyn-compiler @VSadov can I get another sign off?

VSadov

LGTM

OHI tests for ref readonly

00b7b11

OmarTawfik added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Mar 4, 2017

dnfclas added the cla-already-signed label Mar 4, 2017

OmarTawfik added Area-Compilers Feature - Readonly References Readonly References and removed PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. labels Mar 5, 2017

Improve error message for CS0663

ddfbbe6

OmarTawfik commented Mar 6, 2017

View reviewed changes

cston reviewed Mar 6, 2017

View reviewed changes

PR Comments

e231706

OmarTawfik commented Mar 6, 2017

View reviewed changes

Omar Tawfik added 3 commits March 6, 2017 14:08

Added tests for ref readonly method return type

64f4589

Added tests for properties

58810a8

Added OHI tests for ref readonly interfaces

4e3ad0d

cston reviewed Mar 7, 2017

View reviewed changes

PR Feedback

34c098f

VSadov approved these changes Mar 8, 2017

View reviewed changes

OmarTawfik merged commit 3408f6a into dotnet:features/readonly-ref Mar 8, 2017

OmarTawfik deleted the overloads-hiding-inheritence-for-ref-readonly branch March 8, 2017 19:25

OmarTawfik mentioned this pull request Oct 11, 2017

Tests for readonly references overload hiding, inheritance, resolution, and generic type inference. #22283

Closed

zpodlovics mentioned this pull request May 21, 2018

RFCs FS-1050, FS-1051, FS-1052 discussion (Span, IsByRefLike, IsReadOnly) fsharp/fslang-design#287

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OHI tests for ref readonly #17547

OHI tests for ref readonly #17547

OmarTawfik commented Mar 4, 2017

OmarTawfik Mar 6, 2017 •

edited

Loading

OmarTawfik commented Mar 6, 2017 •

edited

Loading

cston Mar 6, 2017 •

edited by OmarTawfik

Loading

cston Mar 6, 2017 •

edited by OmarTawfik

Loading

cston Mar 6, 2017 •

edited by OmarTawfik

Loading

OmarTawfik Mar 6, 2017

cston Mar 6, 2017 •

edited by OmarTawfik

Loading

OmarTawfik Mar 6, 2017 •

edited

Loading

cston Mar 6, 2017 •

edited by OmarTawfik

Loading

cston Mar 6, 2017 •

edited by OmarTawfik

Loading

cston commented Mar 6, 2017 •

edited by OmarTawfik

Loading

OmarTawfik Mar 6, 2017 •

edited

Loading

OmarTawfik commented Mar 7, 2017 •

edited

Loading

cston Mar 7, 2017 •

edited by OmarTawfik

Loading

OmarTawfik Mar 7, 2017

cston Mar 7, 2017 •

edited by OmarTawfik

Loading

OmarTawfik Mar 7, 2017

cston Mar 7, 2017 •

edited by OmarTawfik

Loading

OmarTawfik Mar 7, 2017

cston Mar 7, 2017 •

edited by OmarTawfik

Loading

OmarTawfik Mar 7, 2017

cston Mar 7, 2017 •

edited by OmarTawfik

Loading

cston Mar 7, 2017 •

edited by OmarTawfik

Loading

cston Mar 7, 2017 •

edited by OmarTawfik

Loading

OmarTawfik Mar 7, 2017

cston Mar 7, 2017 •

edited by OmarTawfik

Loading

OmarTawfik Mar 7, 2017

cston Mar 7, 2017 •

edited by OmarTawfik

Loading

cston Mar 7, 2017 •

edited by OmarTawfik

Loading

cston commented Mar 7, 2017

OmarTawfik commented Mar 8, 2017

VSadov left a comment

		@@ -130,6 +130,7 @@ internal enum MessageID

		IDS_FeatureReadonlyReferences = MessageBase + 12718,

		IDS_SK_CONSTRUCTOR = MessageBase + 12719,

OHI tests for ref readonly #17547

OHI tests for ref readonly #17547

Conversation

OmarTawfik commented Mar 4, 2017

OmarTawfik Mar 6, 2017 • edited Loading

Choose a reason for hiding this comment

OmarTawfik commented Mar 6, 2017 • edited Loading

cston Mar 6, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

cston Mar 6, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

cston Mar 6, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

OmarTawfik Mar 6, 2017

Choose a reason for hiding this comment

cston Mar 6, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

OmarTawfik Mar 6, 2017 • edited Loading

Choose a reason for hiding this comment

cston Mar 6, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

cston Mar 6, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

cston commented Mar 6, 2017 • edited by OmarTawfik Loading

OmarTawfik Mar 6, 2017 • edited Loading

Choose a reason for hiding this comment

OmarTawfik commented Mar 7, 2017 • edited Loading

cston Mar 7, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

OmarTawfik Mar 7, 2017

Choose a reason for hiding this comment

cston Mar 7, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

OmarTawfik Mar 7, 2017

Choose a reason for hiding this comment

cston Mar 7, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

OmarTawfik Mar 7, 2017

Choose a reason for hiding this comment

cston Mar 7, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

OmarTawfik Mar 7, 2017

Choose a reason for hiding this comment

cston Mar 7, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

cston Mar 7, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

cston Mar 7, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

OmarTawfik Mar 7, 2017

Choose a reason for hiding this comment

cston Mar 7, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

OmarTawfik Mar 7, 2017

Choose a reason for hiding this comment

cston Mar 7, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

cston Mar 7, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

cston commented Mar 7, 2017

OmarTawfik commented Mar 8, 2017

VSadov left a comment

Choose a reason for hiding this comment

OmarTawfik Mar 6, 2017 •

edited

Loading

OmarTawfik commented Mar 6, 2017 •

edited

Loading

cston Mar 6, 2017 •

edited by OmarTawfik

Loading

cston Mar 6, 2017 •

edited by OmarTawfik

Loading

cston Mar 6, 2017 •

edited by OmarTawfik

Loading

cston Mar 6, 2017 •

edited by OmarTawfik

Loading

OmarTawfik Mar 6, 2017 •

edited

Loading

cston Mar 6, 2017 •

edited by OmarTawfik

Loading

cston Mar 6, 2017 •

edited by OmarTawfik

Loading

cston commented Mar 6, 2017 •

edited by OmarTawfik

Loading

OmarTawfik Mar 6, 2017 •

edited

Loading

OmarTawfik commented Mar 7, 2017 •

edited

Loading

cston Mar 7, 2017 •

edited by OmarTawfik

Loading

cston Mar 7, 2017 •

edited by OmarTawfik

Loading

cston Mar 7, 2017 •

edited by OmarTawfik

Loading

cston Mar 7, 2017 •

edited by OmarTawfik

Loading

cston Mar 7, 2017 •

edited by OmarTawfik

Loading

cston Mar 7, 2017 •

edited by OmarTawfik

Loading

cston Mar 7, 2017 •

edited by OmarTawfik

Loading

cston Mar 7, 2017 •

edited by OmarTawfik

Loading

cston Mar 7, 2017 •

edited by OmarTawfik

Loading

cston Mar 7, 2017 •

edited by OmarTawfik

Loading