Add public IsInitOnly API and use it to fix code generation of 'init' by jcouv · Pull Request #44077 · dotnet/roslyn

jcouv · 2020-05-08T15:58:54Z

Question on compiler API:

Should the set accessor of an invalid property static int Property { get; init; } have IsInitOnly true or false? Currently, it is false, to be consistent with behavior when loading such property from PE.

IDE scenarios supported by this API: generate override, generate implementation, view metadata-as-source.

Relates to #40726 (test plan for records/init-only)

gafter · 2020-05-08T21:47:18Z

initonly is an existing concept in ECMA-335. Unless this represents the same concept, please rename it.

jcouv · 2020-05-09T00:26:48Z

Tagging @jaredpar since any decision we make for public API should preferably align with the language spec

initonly is an existing concept in ECMA-335. Unless this represents the same concept, please rename it.

Yes, the runtime has the concept of initonly, but C# only has the concept of readonly (ECMA-334 only mentions readonly, not initonly). It is sad that the names don't align, but I don't think it precludes the best name available at the C# level for this feature.

FWIW, I also considered IsInit and IsExternalInit (to match the modreq type). But "init-only" is the way LDM and the spec described such members (we had an explicit discussion about why the "only" part is useful/meaningful). init accessors were thought of as a shorthand for initonly set, and we may still introduce the initonly modifier methods (question is queued).

Suggestions welcome.
I will gladly leave a question open or PROTOTYPE marker to move forward.

AlekseyTs · 2020-05-09T19:12:20Z

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

+        }
+
+        [Fact]
+        public void ConstructorAndDestructorAreNotInitOnly()


ConstructorAndDestructorAreNotInitOnly [](start = 20, length = 38)

Could also cover operators #Closed

AlekseyTs · 2020-05-09T19:15:02Z

src/Compilers/CSharp/Test/Symbol/Symbols/Retargeting/RetargetingTests.cs

+            var sourceModule = compilation.SourceModule;
+            var sourceAssembly = (SourceAssemblySymbol)sourceModule.ContainingAssembly;
+
+            var retargetingAssembly = new RetargetingAssemblySymbol(sourceAssembly, isLinked: false);


var retargetingAssembly = new RetargetingAssemblySymbol(sourceAssembly, isLinked: false); [](start = 12, length = 89)

Please do not use this legacy way of testing retargeting. Create a scenario when the system itself creates one, a real scenario and test that. #Closed

I've looked at more recent retargeting tests (Fred's recent PRs) but could not figure out the key to triggering retargeting logic to kick in. I tried tracing it back (seems to be triggered in ReferenceManager?).
What's a simple/modern test you'd recommend as a model for retargeting?

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

What's a simple/modern test you'd recommend as a model for retargeting?

For example, look at CodeGenTests.CustomFields_01. The 7th compilation, the commented out, is supposed to use retargeting symbols for comp1. The change in the target framework does the trick.

In reply to: 422651853 [](ancestors = 422651853,422534751)

Thanks a lot. Changed the retargeting test to use that model. #Resolved

AlekseyTs · 2020-05-09T19:17:10Z

src/Compilers/CSharp/Test/Symbol/Symbols/Retargeting/RetargetingTests.cs

        }

+        [Fact, CompilerTrait(CompilerFeature.InitOnlySetters)]
+        public void RetargetProperties_WithInitOnlySetter()


RetargetProperties_WithInitOnlySetter [](start = 20, length = 37)

I suggest that we keep this test next to other InitOnly tests. #Closed

AlekseyTs · 2020-05-09T19:17:55Z

Done with review pass (iteration 3) #Closed

CyrusNajmabadi · 2020-05-09T19:25:03Z

src/EditorFeatures/VisualBasic/NavigationBar/GenerateFinalizerItem.vb

                modifiers:=New DeclarationModifiers(isOverride:=True),
                returnType:=compilation.GetSpecialType(SpecialType.System_Void),
                refKind:=RefKind.None,
+                isInitOnly:=False,


this feels weird. how do we do other accessors? isn't this just a certain type of accessor, not a method? #Resolved

CyrusNajmabadi · 2020-05-09T19:25:20Z

...gs/ConvertLocalFunctionToMethod/CSharpConvertLocalFunctionToMethodCodeRefactoringProvider.cs

                modifiers: new DeclarationModifiers(isStatic, isAsync: declaredSymbol.IsAsync),
                returnType: declaredSymbol.ReturnType,
                refKind: default,
+                isInitOnly: false,


not really getting how methods are init-only. #Resolved

Currently, only accessors can be init-only. But accessors are methods.
We have a discussion queued to discuss init-only methods more generally, but that should not affect this PR (assume that we don't have generalized init-only methods).

I can make the parameter optional, so it's not in your face at most places where it could not legitimately be set to true. Would that resolve your concern?

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

Isn't init a different type of accessor entirely. I.e. a property would have three potential accessors? GetMethod/SetMethod/InitMethod? #Resolved

No, it's best thought of as a modifier on set. But we shortened initonly set to init.
https://github.com/dotnet/csharplang/blob/master/proposals/init.md #Resolved

got it 👍 can you make it a default value? thanks! #Resolved

CyrusNajmabadi · 2020-05-09T19:25:53Z

done with IDE pass. very large question about the design here. #Resolved

AlekseyTs · 2020-05-11T17:35:50Z

src/Compilers/CSharp/Test/Symbol/Symbols/Retargeting/RetargetingTests.cs

 using Roslyn.Test.Utilities;
 using Xunit;
 using Utils = Microsoft.CodeAnalysis.CSharp.UnitTests.CompilationUtils;
+using Microsoft.CodeAnalysis.Test.Utilities;


This looks like an unnecessary change, consider reverting the file. #Closed

AlekseyTs · 2020-05-11T17:39:44Z

Done with review pass (iteration 4) #Closed

AlekseyTs

LGTM (iteration 5).

jcouv · 2020-05-11T20:09:51Z

I've added a follow-up item to the test plan to resolve the naming question in API and spec.

gafter · 2020-05-11T20:24:51Z

src/Compilers/Core/Portable/Symbols/IMethodSymbol.cs

        bool IsReadOnly { get; }

+        /// <summary>
+        /// 'init' set accessors can only be invoked during construction or in the initialization phase


set [](start = 19, length = 3)

It isn't clear if this is intended to apply only to set accessors, or if other methods may be so flagged.

gafter

jcouv · 2020-05-12T14:45:24Z

FYI @AlekseyTs I made minor change to retargeting test to reflect different verification behavior on desktop versus Core

jcouv · 2020-05-12T19:16:30Z

@CyrusNajmabadi Please take another look when you can. Thanks

CyrusNajmabadi

IDE side lgtm.

Julien Couvreur added 2 commits May 7, 2020 21:22

Add public API for IsInitOnly

fa105f2

Fix code generation on init-only property

5af14a6

jcouv added Area-IDE Area-Compilers labels May 8, 2020

jcouv self-assigned this May 8, 2020

jcouv mentioned this pull request May 8, 2020

Test plan for "records" #40726

Closed

jcouv force-pushed the init-api branch 2 times, most recently from 41f4f81 to 8c9ac79 Compare May 8, 2020 18:08

tweak

0bfc351

jcouv force-pushed the init-api branch from 8c9ac79 to 0bfc351 Compare May 8, 2020 21:29

jcouv marked this pull request as ready for review May 8, 2020 21:29

jcouv requested review from a team as code owners May 8, 2020 21:29

AlekseyTs reviewed May 9, 2020

View reviewed changes

CyrusNajmabadi reviewed May 9, 2020

View reviewed changes

Address feedback

1e59e42

AlekseyTs reviewed May 11, 2020

View reviewed changes

Use modern retargeting test

257307d

jcouv requested review from AlekseyTs and CyrusNajmabadi May 11, 2020 18:02

AlekseyTs approved these changes May 11, 2020

View reviewed changes

IDE tweak

5db0e22

gafter reviewed May 11, 2020

View reviewed changes

gafter approved these changes May 11, 2020

View reviewed changes

Julien Couvreur added 3 commits May 11, 2020 14:02

Tweak doc comment

dc3bbd6

Fix incorrect cast in test

63d368a

Fixup PE verification for retargeting test

307e091

CyrusNajmabadi approved these changes May 12, 2020

View reviewed changes

jcouv closed this May 12, 2020

jcouv reopened this May 12, 2020

Merge remote-tracking branch 'dotnet/features/records' into init-api

38e9d46

jcouv force-pushed the init-api branch from 7efaa30 to 38e9d46 Compare May 13, 2020 05:48

jcouv merged commit 6e262ac into dotnet:features/records May 13, 2020

jcouv deleted the init-api branch May 13, 2020 20:15

Conversation

jcouv commented May 8, 2020

Uh oh!

gafter commented May 8, 2020

Uh oh!

jcouv commented May 9, 2020

Uh oh!

AlekseyTs May 9, 2020 • 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!

AlekseyTs May 9, 2020 • 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 May 10, 2020

Choose a reason for hiding this comment

Uh oh!

AlekseyTs May 11, 2020

Choose a reason for hiding this comment

Uh oh!

jcouv May 11, 2020 • 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!

AlekseyTs May 9, 2020 • 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!

AlekseyTs commented May 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyrusNajmabadi May 9, 2020 • 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!

CyrusNajmabadi May 9, 2020 • 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 May 10, 2020

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi May 10, 2020 • 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 May 10, 2020 • 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!

CyrusNajmabadi May 10, 2020 • 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!

CyrusNajmabadi commented May 9, 2020 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs May 11, 2020 • 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!

AlekseyTs commented May 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv commented May 11, 2020

Uh oh!

gafter May 11, 2020

Choose a reason for hiding this comment

Uh oh!

gafter left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv commented May 12, 2020

Uh oh!

jcouv commented May 12, 2020

Uh oh!

AlekseyTs May 9, 2020 •

edited

Loading

AlekseyTs May 9, 2020 •

edited

Loading

jcouv May 11, 2020 •

edited

Loading

AlekseyTs May 9, 2020 •

edited

Loading

AlekseyTs commented May 9, 2020 •

edited

Loading

CyrusNajmabadi May 9, 2020 •

edited by jcouv

Loading

CyrusNajmabadi May 9, 2020 •

edited by jcouv

Loading

CyrusNajmabadi May 10, 2020 •

edited by jcouv

Loading

jcouv May 10, 2020 •

edited

Loading

CyrusNajmabadi May 10, 2020 •

edited by jcouv

Loading

CyrusNajmabadi commented May 9, 2020 •

edited by jcouv

Loading

AlekseyTs May 11, 2020 •

edited

Loading

AlekseyTs commented May 11, 2020 •

edited

Loading