Readonly members symbol display #34876

RikkiGibson · 2019-04-09T19:26:01Z

src/Compilers/CSharp/Portable/Symbols/PropertySymbolExtensions.cs

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

cston · 2019-04-10T00:37:19Z

src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.Members.cs

@@ -168,6 +173,11 @@ public override void VisitEvent(IEventSymbol symbol)
            AddAccessibilityIfRequired(symbol);
            AddMemberModifiersIfRequired(symbol);

+            if ((symbol.AddMethod ?? symbol.RemoveMethod)?.IsReadOnly == true)


(symbol.AddMethod ?? symbol.RemoveMethod)?.IsReadOnly == true [](start = 16, length = 61)

What if add is readonly but remove is not?

We don't provide a way to declare such a combination of event accessors in the language, so I'd write it off as a malformed symbol that we can't display reliably--we just need to avoid crashing.

src/Compilers/CSharp/Portable/Symbols/PropertySymbolExtensions.cs

…symbol-display

RikkiGibson · 2019-04-10T21:45:46Z

So it looks like a lot of modifiers such as public, virtual, static, etc. don't show in Quick Info for members. I decided that readonly should show because it affects the ref kind of the this parameter and we show ref modifiers on parameter and return types in general. Let me know what you think.

RikkiGibson · 2019-04-11T03:05:18Z

Looks like changing symbol display might have broken a bunch of IL tests?

jaredpar · 2019-04-11T03:24:57Z

Part of the print out of IL includes symbol display and the methods used here have some readonly in them. Not members so it's suprising that this broke.

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

src/Compilers/CSharp/Portable/Symbols/PropertySymbolExtensions.cs

RikkiGibson · 2019-04-11T20:57:11Z

Putting this on hold until we have a chance to discuss the design

RikkiGibson · 2019-04-16T18:30:56Z

We concluded in design that "explicitly readonly" members should show a 'readonly' keyword in Quick Info. If the member is only readonly due to the containing struct being 'readonly', then we don't show the keyword. /cc @sharwell @jasonmalinowski. Will revise accordingly when I get the chance (focusing on #34778 first)

src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.Members.cs

src/Compilers/CSharp/Portable/Symbols/PropertySymbolExtensions.cs

jcouv

Done with review pass (iteration 6)

jcouv · 2019-04-23T23:09:52Z

Marked PR as "private" for now. Remove the tag once ready for another look. Thanks

…symbol-display

…truct

RikkiGibson · 2019-04-30T23:45:15Z

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

@@ -367,7 +367,7 @@ .maxstack  2
  IL_0003:  stfld      ""int S.<X>k__BackingField""
  IL_0008:  ldarg.0
  IL_0009:  ldarg.0
-  IL_000a:  call       ""int S.X.get""
+  IL_000a:  call       ""readonly int S.X.get""


One quirk of this work: after a property is emitted we have no decent way of knowing whether it was implicitly readonly in source. So when the member is in a non-readonly type we just display readonly if the IsReadOnlyAttribute is present on the member.

I tried to avoid displaying readonly where it was not present in source, and for members of readonly types, because it minimizes test churn and limits showing the keyword to cases where its presence or absence will be more informative.

RikkiGibson · 2019-04-30T23:46:41Z

src/Compilers/Core/Portable/SymbolDisplay/SymbolDisplayMemberOptions.cs

@@ -68,6 +68,8 @@ public enum SymbolDisplayMemberOptions

        /// <summary>
        /// Includes the <c>ref</c>, <c>ref readonly</c>, <c>ByRef</c> keywords for ref-returning methods and properties/indexers.
+        /// Also includes the <c>readonly</c> keyword on methods, properties/indexers, and events due to the keyword
+        /// changing the <c>this</c> parameter's ref kind from <c>ref</c> to <c>ref readonly</c>.


This seems fishy. Is this a name we regret and should fix?

RikkiGibson · 2019-05-02T20:09:33Z

PTAL @dotnet/roslyn-compiler @dotnet/roslyn-ide

jasonmalinowski

IDE parts look fine; there's some suspicious bits in the compiler code to me. Also tagging @sharwell since I know he was doing some work on quick info tests. If this is the new structure, great; if not then maybe there's work needed.

jasonmalinowski · 2019-05-03T21:41:50Z

src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.Members.cs

+                return false;
+            }
+
+            if (method is SourcePropertyAccessorSymbol sourceAccessor && propertyOpt is SourcePropertySymbol sourceProperty)


Should this be using method.AssociatedSymbol to get the source property instead? My guess being one of three things are true:

ShouldMethodDisplayReadOnly gets called directly with an accessor and not just from ShouldPropertyDisplayReadOnly, at which point the callers are having to get the source property,

This is only being passed by ShouldPropertyDisplayReadOnly, at which point we could just inline this into that, or:

We're trying to do something fancier here, and we probably should have a comment.

I believe (1) is the answer here. See the AddAccessor method in this file.

jasonmalinowski · 2019-05-03T21:45:05Z

src/Compilers/Core/Portable/SymbolDisplay/SymbolDisplayMemberOptions.cs

@@ -68,6 +68,8 @@ public enum SymbolDisplayMemberOptions

        /// <summary>
        /// Includes the <c>ref</c>, <c>ref readonly</c>, <c>ByRef</c> keywords for ref-returning methods and properties/indexers.
+        /// Also includes the <c>readonly</c> keyword on methods, properties/indexers, and events due to the keyword
+        /// changing the <c>this</c> parameter's ref kind from <c>ref</c> to <c>ref readonly</c>.


This seems fishy. Is this a name we regret and should fix?

RikkiGibson · 2019-05-03T22:09:13Z

Responding to @jasonmalinowski's comment:

This seems fishy. Is this a name we regret and should fix?

Maybe! I approached the situation by looking for an existing SymbolDisplayMemberOption which would:

make the keyword get displayed in the contexts I wanted (quick info)
basically fit in the category implied by the option

I felt throwing it under the IncludeRef category made enough sense because the display of both ref readonly and ref are affected by it today, and the readonly keyword on methods/properties/events has a similar effect of changing the RefKind of a parameter.

It's possible that the name of the option should change, but I can't readily think of a more appropriate name. (I think it would also be a breaking change to rename it.)

RikkiGibson · 2019-05-03T23:00:10Z

It sounds like I'm using the right pattern for Quick Info tests. Are there any remaining concerns about the use or documentation of SymbolDisplayMemberOptions.IncludeRef?

RikkiGibson · 2019-05-06T21:05:17Z

I'm going to merge now as it feels like we've resolved the concerns. Please let me know if there's anything that should be revised in a separate PR.

RikkiGibson added 3 commits April 8, 2019 15:39

Symbol display for readonly members

2b7b94b

Add coverage

fd77640

Test malformed properties and events

e7a4487

RikkiGibson requested a review from a team as a code owner April 9, 2019 19:26

RikkiGibson added Area-Compilers Area-IDE IDE-IntelliSense Completion, Signature Help, Quick Info labels Apr 9, 2019

CyrusNajmabadi reviewed Apr 9, 2019

View reviewed changes

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

CyrusNajmabadi reviewed Apr 9, 2019

View reviewed changes

src/Compilers/CSharp/Test/Symbol/SymbolDisplay/SymbolDisplayTests.cs Show resolved Hide resolved

cston reviewed Apr 10, 2019

View reviewed changes

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

RikkiGibson added 2 commits April 10, 2019 12:49

Merge remote-tracking branch 'upstream/master' into readonly-members-…

3dcc156

…symbol-display

Show readonly keyword on non-field members in QuickInfo

0179928

RikkiGibson requested a review from a team as a code owner April 10, 2019 21:42

RikkiGibson requested a review from jmarolf April 10, 2019 21:45

jaredpar reviewed Apr 11, 2019

View reviewed changes

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

Use IsDeclaredReadOnly for symbol display (WIP)

bc2e446

jcouv reviewed Apr 18, 2019

View reviewed changes

src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.Members.cs Show resolved Hide resolved

jcouv reviewed Apr 18, 2019

View reviewed changes

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

jcouv reviewed Apr 18, 2019

View reviewed changes

jcouv self-assigned this Apr 18, 2019

jcouv added this to the 16.2 milestone Apr 23, 2019

jcouv added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Apr 23, 2019

Merge remote-tracking branch 'upstream/master' into readonly-members-…

1792c17

…symbol-display

RikkiGibson added 6 commits April 25, 2019 14:32

Update based on design and fix some test failures

539ae16

Add readonly keywords to various IL tests

6e40f15

Fix more tests, respond to feedback

21f51d0

Fix compile error

5946c98

Don't display readonly keyword for implicit readonly getters from source

41c75c2

Don't show readonly keyword for members contained within a readonly s…

6b0b6f7

…truct

RikkiGibson commented Apr 30, 2019

View reviewed changes

Move from extension methods to private statics

b83963d

RikkiGibson removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label May 2, 2019

cston approved these changes May 2, 2019

View reviewed changes

RikkiGibson closed this May 2, 2019

RikkiGibson reopened this May 2, 2019

RikkiGibson closed this May 3, 2019

RikkiGibson reopened this May 3, 2019

jasonmalinowski approved these changes May 3, 2019

View reviewed changes

RikkiGibson merged commit f99a821 into dotnet:master May 6, 2019

RikkiGibson deleted the readonly-members-symbol-display branch May 6, 2019 21:05

RikkiGibson mentioned this pull request Jul 26, 2019

Simplify readonly MetadataAsSource #37486

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Readonly members symbol display #34876

Readonly members symbol display #34876

RikkiGibson commented Apr 9, 2019 •

edited

Loading

cston Apr 10, 2019 •

edited

Loading

RikkiGibson Apr 10, 2019

RikkiGibson commented Apr 10, 2019

RikkiGibson commented Apr 11, 2019

jaredpar commented Apr 11, 2019

RikkiGibson commented Apr 11, 2019

RikkiGibson commented Apr 16, 2019

jcouv left a comment

jcouv commented Apr 23, 2019

RikkiGibson Apr 30, 2019 •

edited

Loading

RikkiGibson Apr 30, 2019

jasonmalinowski May 3, 2019

RikkiGibson commented May 2, 2019

jasonmalinowski left a comment

jasonmalinowski May 3, 2019

RikkiGibson May 3, 2019

jasonmalinowski May 3, 2019

RikkiGibson commented May 3, 2019 •

edited

Loading

RikkiGibson commented May 3, 2019

RikkiGibson commented May 6, 2019

Readonly members symbol display #34876

Readonly members symbol display #34876

Conversation

RikkiGibson commented Apr 9, 2019 • edited Loading

cston Apr 10, 2019 • edited Loading

Choose a reason for hiding this comment

RikkiGibson Apr 10, 2019

Choose a reason for hiding this comment

RikkiGibson commented Apr 10, 2019

RikkiGibson commented Apr 11, 2019

jaredpar commented Apr 11, 2019

RikkiGibson commented Apr 11, 2019

RikkiGibson commented Apr 16, 2019

jcouv left a comment

Choose a reason for hiding this comment

jcouv commented Apr 23, 2019

RikkiGibson Apr 30, 2019 • edited Loading

Choose a reason for hiding this comment

RikkiGibson Apr 30, 2019

Choose a reason for hiding this comment

jasonmalinowski May 3, 2019

Choose a reason for hiding this comment

RikkiGibson commented May 2, 2019

jasonmalinowski left a comment

Choose a reason for hiding this comment

jasonmalinowski May 3, 2019

Choose a reason for hiding this comment

RikkiGibson May 3, 2019

Choose a reason for hiding this comment

jasonmalinowski May 3, 2019

Choose a reason for hiding this comment

RikkiGibson commented May 3, 2019 • edited Loading

RikkiGibson commented May 3, 2019

RikkiGibson commented May 6, 2019

RikkiGibson commented Apr 9, 2019 •

edited

Loading

cston Apr 10, 2019 •

edited

Loading

RikkiGibson Apr 30, 2019 •

edited

Loading

RikkiGibson commented May 3, 2019 •

edited

Loading