Enable consumption of init-only properties in VB. #50414

AlekseyTs · 2021-01-13T00:24:29Z

Closes dotnet#44870. Closes dotnet#49469.

AlekseyTs · 2021-01-13T14:22:20Z

@jcouv, @dotnet/roslyn-compiler Please review.

AlekseyTs · 2021-01-13T22:53:48Z

@jcouv, @dotnet/roslyn-compiler Please review.

jcouv · 2021-01-14T01:16:54Z

Public Class InitOnlyMemberTests

Do we have to do anything for displaying init-only accessor or property (ToDisplayString and QuickInfo/MetadataAsSource)?
#Closed

Refers to: src/Compilers/VisualBasic/Test/Semantic/Semantics/InitOnlyMemberTests.vb:12 in 8d75b3e. [](commit_id = 8d75b3e, deletion_comment = False)

jcouv · 2021-01-14T01:17:22Z

    B.Init(b(9), 49)

I didn't understand how this works. There is no compilation error, and yet the init-only setter doesn't seem to be invoked.
I would probably expect an error here, as b(9) = temp would be an error. #Closed

Refers to: src/Compilers/VisualBasic/Test/Semantic/Semantics/InitOnlyMemberTests.vb:752 in 8d75b3e. [](commit_id = 8d75b3e, deletion_comment = False)

jcouv · 2021-01-14T01:17:32Z

Public Shared Sub Init(ByRef p as Integer, val As Integer)

Is there any risk that the ByRef property could leak and be exercised past the construction/initialization phase? #Closed

Refers to: src/Compilers/VisualBasic/Test/Semantic/Semantics/InitOnlyMemberTests.vb:337 in 8d75b3e. [](commit_id = 8d75b3e, deletion_comment = False)

jcouv · 2021-01-14T01:17:44Z

public int this[int x] { init => _item[x] = value; get => _item[x]; }

nit: It would be simpler to follow this test if the setter simply threw if x or the value are above the expected max (9 or 49 respectively, we just need to pick one).
#Closed

Refers to: src/Compilers/VisualBasic/Test/Semantic/Semantics/InitOnlyMemberTests.vb:740 in 8d75b3e. [](commit_id = 8d75b3e, deletion_comment = False)

jcouv · 2021-01-14T01:17:49Z

  .maxstack  8

nit: we can remove all the .maxstack and IL comments to compact the test a bit #Closed

Refers to: src/Compilers/VisualBasic/Test/Semantic/Semantics/InitOnlyMemberTests.vb:2506 in 8d75b3e. [](commit_id = 8d75b3e, deletion_comment = False)

jcouv · 2021-01-14T01:17:55Z

    ox.P0 = -40

Is this something we should file an issue for, or do we just accept this as a hole (like dynamic and reflection access)? #Closed

Refers to: src/Compilers/VisualBasic/Test/Semantic/Semantics/InitOnlyMemberTests.vb:3055 in 8d75b3e. [](commit_id = 8d75b3e, deletion_comment = False)

jcouv · 2021-01-14T01:18:01Z

src/Compilers/VisualBasic/Test/CommandLine/CommandLineTests.vb

@@ -2027,7 +2031,7 @@ End Module").Path
            ' - update project-system to recognize the new value and pass it through


The line about IDE drop-down can be removed (the drop-down was removed for C#, so won't be added for VB) and I think we didn't need to modify project-system repo anymore (I don't see a PR there for 15.3 or 15.6). #Closed

jcouv · 2021-01-14T01:18:05Z

src/Compilers/VisualBasic/Test/CommandLine/CommandLineTests.vb

@@ -2027,7 +2031,7 @@ End Module").Path
            ' - update project-system to recognize the new value and pass it through
            ' - update all the tests that call this canary
            ' - update the command-line documentation (CommandLine.md)


' - update the command-line documentation (CommandLine.md) [](start = 12, length = 58)

Let's update the doc.
I think vbc -langversion:? will automatically pick up the new version, but may be worth a try. #Closed

Let's update the doc.

I am not sure what is there to update. It doesn't look like the doc lists specific versions or I am looking at a wrong document.

I think vbc -langversion:? will automatically pick up the new version, but may be worth a try.

It does.

Supported language versions: default 9 10 11 12 14 15 15.3 15.5 16 (default) 16.9 (latest) latest

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

Right, it looks like the bullet for CommandLine.md is obsolete too. We'll remove it next time. Thanks

In reply to: 558429780 [](ancestors = 558429780,556982209)

jcouv · 2021-01-14T01:18:23Z

src/Compilers/VisualBasic/Test/CommandLine/CommandLineTests.vb

@@ -2068,8 +2073,9 @@ End Module").Path
            Assert.Equal(LanguageVersion.VisualBasic15_3, LanguageVersion.VisualBasic15_3.MapSpecifiedToEffectiveVersion())
            Assert.Equal(LanguageVersion.VisualBasic15_5, LanguageVersion.VisualBasic15_5.MapSpecifiedToEffectiveVersion())
            Assert.Equal(LanguageVersion.VisualBasic16, LanguageVersion.VisualBasic16.MapSpecifiedToEffectiveVersion())
+            Assert.Equal(LanguageVersion.VisualBasic16_9, LanguageVersion.VisualBasic16_9.MapSpecifiedToEffectiveVersion())
            Assert.Equal(LanguageVersion.VisualBasic16, LanguageVersion.Default.MapSpecifiedToEffectiveVersion())


LanguageVersion.Default [](start = 56, length = 23)

Let's confirm what version Default should map to. I think we could have it point to 16.9 now. #Resolved

Prior to moving to VB16 we had mapping work this way:

Select Case version Case LanguageVersion.Latest Return LanguageVersion.VisualBasic15_5 Case LanguageVersion.Default Return LanguageVersion.VisualBasic15 Case Else Return version End Select

I am following the same pattern.

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

I don't believe we can update Default until we have a new major VS version. @jaredpar, can you weigh in as well?

In reply to: 557448246 [](ancestors = 557448246,556982329)

jcouv · 2021-01-14T01:18:30Z

src/Compilers/VisualBasic/Portable/Symbols/PropertySymbol.vb

+
+                        Return False
+
+                    Case BoundKind.MeReference, BoundKind.MyBaseReference, BoundKind.MyClassReference


In C# we're also going to allow certain conversions on this.
See #50424 #Closed

In C# we're also going to allow certain conversions on this.

I would prefer to deal with this recent design change (BTW, has the spec been updated?) separately. Please don't close #50053 once you are done with C#, instead add a comment that VB should also be adjusted and assign the issue to me.

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

jcouv · 2021-01-14T01:18:38Z

src/Compilers/VisualBasic/Portable/Symbols/PropertySymbol.vb

+                    Return False
+                End If
+
+                ' ok: New C() With { .InitOnlyProperty = ... }


In C# we had to handle nested initializers differently. Does VB also support nested initializers? (same question for With expression) #Closed

In C# we had to handle nested initializers differently. Does VB also support nested initializers? (same question for With expression)

There is no such thing as a nested initializer in VB. Nested with statements are allowed but only the immediate one is in effect at any given point. In other words, there is no fallback to the outer With statement, the receiver is overwritten.

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

jcouv

Done with review pass (iteration 1)

AlekseyTs · 2021-01-14T14:58:26Z

Public Class InitOnlyMemberTests

Do we have to do anything for displaying init-only accessor or property (ToDisplayString and QuickInfo/MetadataAsSource)?

I don't think so. There is no such thing as init-only properties in VB. No special syntax for them, etc. This is just another case of metadata that are outside of the language specification that we are simply doing our best to consume.

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

Refers to: src/Compilers/VisualBasic/Test/Semantic/Semantics/InitOnlyMemberTests.vb:12 in 8d75b3e. [](commit_id = 8d75b3e, deletion_comment = False)

AlekseyTs · 2021-01-14T15:02:11Z

    B.Init(b(9), 49)

I didn't understand how this works. There is no compilation error, and yet the init-only setter doesn't seem to be invoked.
I would probably expect an error here, as b(9) = temp would be an error.

This is how ByRef works in VB. If it cannot write, it doesn't write and only reads. This is how regular RValues, readonly fields, and readonly properties (including auto-properties) are handled. No errors reported.

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

Refers to: src/Compilers/VisualBasic/Test/Semantic/Semantics/InitOnlyMemberTests.vb:752 in 8d75b3e. [](commit_id = 8d75b3e, deletion_comment = False)

AlekseyTs · 2021-01-14T15:08:04Z

Public Shared Sub Init(ByRef p as Integer, val As Integer)

Is there any risk that the ByRef property could leak and be exercised past the construction/initialization phase?

Perhaps I am misinterpreting the question. This function doesn't get direct "access" to the property. Compiler passes a reference to a local and writes back to the property after the call when writing back is expected.

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

Refers to: src/Compilers/VisualBasic/Test/Semantic/Semantics/InitOnlyMemberTests.vb:337 in 8d75b3e. [](commit_id = 8d75b3e, deletion_comment = False)

AlekseyTs · 2021-01-14T15:11:45Z

    ox.P0 = -40

Is this something we should file an issue for, or do we just accept this as a hole (like dynamic and reflection access)?

This is VB "dynamic". Latebinder is inside VB runtime library and I don't think we are going to update it.

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

Refers to: src/Compilers/VisualBasic/Test/Semantic/Semantics/InitOnlyMemberTests.vb:3055 in 8d75b3e. [](commit_id = 8d75b3e, deletion_comment = False)

AlekseyTs · 2021-01-14T17:02:30Z

@dotnet/roslyn-compiler Please review.

333fred · 2021-01-14T18:43:37Z

Looking. #Resolved

333fred · 2021-01-14T18:47:37Z

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

@@ -933,7 +933,7 @@ internal bool CalculateUseSiteDiagnostic(ref DiagnosticInfo result)

            // Check return type, custom modifiers, parameters
            if (DeriveUseSiteDiagnosticFromType(ref result, this.ReturnTypeWithAnnotations,
-                                                MethodKind == MethodKind.PropertySet ?
+                                                IsInitOnly ?


Nit: in the future, it would be great to have this bug fix as either a separate commit or a separate PR, as it doesn't really seem related to VB consuming such properties. #Resolved

Nit: in the future, it would be great to have this bug fix as either a separate commit or a separate PR, as it doesn't really seem related to VB consuming such properties.

I think the change is related to the feature I am working on. And I don't think it makes sense to separate this change and have compilers behave inconsistently.

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

333fred · 2021-01-14T18:50:17Z

src/Compilers/VisualBasic/Portable/VBResources.resx

+    <value>'{0}' cannot override init-only '{1}'.</value>
+  </data>
+  <data name="ERR_PropertyDoesntImplementInitOnly" xml:space="preserve">
+    <value>Init-only '{0}' cannot be implemented.</value>


Init-only [](start = 11, length = 9)

Perhaps Init-only property? #Resolved

Perhaps Init-only property?

Symbol display does that. Similar exiting diagnostics doesn't duplicate member kind in suggested way.

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

333fred · 2021-01-14T18:55:02Z

src/Compilers/VisualBasic/Portable/Binding/Binder_ObjectInitializer.vb

+                                propertyAccess = propertyAccess.Update(propertyAccess.PropertySymbol, propertyAccess.PropertyGroupOpt, propertyAccess.AccessKind, isWriteable:=True,
+                                                                       propertyAccess.IsLValue, propertyAccess.ReceiverOpt, propertyAccess.Arguments, propertyAccess.DefaultArguments,
+                                                                       propertyAccess.Type)
+                                target = propertyAccess


Do we need to check LangVersion here? #Resolved

If we don't, a comment as to why would be appreciated.

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

If we don't, a comment as to why would be appreciated.

We are simply deciding if the property is writable or not, the property is writable or not writable regardless of a language version. I don't think any comments about why we don't do something here are necessary, we are not doing a lot of things here.

In reply to: 557618498 [](ancestors = 557618498,557618151)

333fred · 2021-01-14T21:35:54Z

        Dim ilSource = <![CDATA[

It would be good to call out what is special about this IL that can't be written in C# so I don't have reverse-engineer the test when looking at it.

Refers to: src/Compilers/VisualBasic/Test/Semantic/Semantics/InitOnlyMemberTests.vb:1995 in 8d75b3e. [](commit_id = 8d75b3e, deletion_comment = False)

333fred

LGTM (commit 1), with Julien's suggestions addressed.

jcouv · 2021-01-14T23:22:20Z

    B.Init(b(9), 49)

Cool, so this remains safe (it's not bypassing init rule). Thanks
Btw, Jared shared this summary on ByRef with me. May be useful to others as well: https://devblogs.microsoft.com/vbteam/the-many-cases-of-byref/

In reply to: 760252469 [](ancestors = 760252469,759860715)

Refers to: src/Compilers/VisualBasic/Test/Semantic/Semantics/InitOnlyMemberTests.vb:752 in 8d75b3e. [](commit_id = 8d75b3e, deletion_comment = False)

AlekseyTs · 2021-01-15T17:25:57Z

@jcouv Addressed feedback.

jcouv · 2021-01-15T20:05:06Z

Not for this PR: We'll probably want some documentation for team feature review next week.

jcouv

LGTM Thanks (iteration 3)

Enable consumption of init-only properties in VB.

8d75b3e

Closes dotnet#44870. Closes dotnet#49469.

AlekseyTs added the Area-Compilers label Jan 13, 2021

AlekseyTs requested review from jcouv and a team January 13, 2021 00:24

jcouv self-assigned this Jan 13, 2021

jcouv reviewed Jan 14, 2021

View reviewed changes

jcouv added the New Language Feature - Records Records label Jan 14, 2021

jcouv reviewed Jan 14, 2021

View reviewed changes

jcouv added this to the 16.9 milestone Jan 14, 2021

333fred reviewed Jan 14, 2021

View reviewed changes

333fred approved these changes Jan 14, 2021

View reviewed changes

AlekseyTs added 2 commits January 15, 2021 07:51

Merge remote-tracking branch 'dotnet/master' into VBInitOnly

38855b5

PR feedback

40a5e71

jcouv approved these changes Jan 15, 2021

View reviewed changes

AlekseyTs merged commit 0a39566 into dotnet:master Jan 15, 2021

ghost modified the milestones: 16.9, Next Jan 15, 2021

jcouv mentioned this pull request Jan 26, 2021

Test plan for "consumption of init-only properties" #50792

Open

27 tasks

Cosifne modified the milestones: Next, 16.9 Jan 27, 2021

JoeRobich modified the milestones: 16.9, 16.9.P4 Jan 27, 2021

		@@ -2027,7 +2031,7 @@ End Module").Path
		' - update project-system to recognize the new value and pass it through


		Return False

		Case BoundKind.MeReference, BoundKind.MyBaseReference, BoundKind.MyClassReference

Enable consumption of init-only properties in VB. #50414

Enable consumption of init-only properties in VB. #50414

Conversation

AlekseyTs commented Jan 13, 2021

AlekseyTs commented Jan 13, 2021

AlekseyTs commented Jan 13, 2021

jcouv commented Jan 14, 2021 • edited Loading

jcouv commented Jan 14, 2021 • edited Loading

jcouv commented Jan 14, 2021 • edited Loading

jcouv commented Jan 14, 2021 • edited Loading

jcouv commented Jan 14, 2021 • edited Loading

jcouv commented Jan 14, 2021 • edited Loading

jcouv Jan 14, 2021 • edited Loading

Choose a reason for hiding this comment

jcouv Jan 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Jan 14, 2021 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Jan 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Jan 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

AlekseyTs commented Jan 14, 2021

AlekseyTs commented Jan 14, 2021

AlekseyTs commented Jan 14, 2021

AlekseyTs commented Jan 14, 2021

AlekseyTs commented Jan 14, 2021

333fred commented Jan 14, 2021 • edited by AlekseyTs Loading

333fred Jan 14, 2021 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

333fred Jan 14, 2021 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

333fred Jan 14, 2021 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Jan 14, 2021 • edited Loading

Choose a reason for hiding this comment

333fred commented Jan 14, 2021

333fred left a comment

Choose a reason for hiding this comment

jcouv commented Jan 14, 2021

AlekseyTs commented Jan 15, 2021

jcouv commented Jan 15, 2021

jcouv left a comment

Choose a reason for hiding this comment

jcouv commented Jan 14, 2021 •

edited

Loading

jcouv commented Jan 14, 2021 •

edited

Loading

jcouv commented Jan 14, 2021 •

edited

Loading

jcouv commented Jan 14, 2021 •

edited

Loading

jcouv commented Jan 14, 2021 •

edited

Loading

jcouv commented Jan 14, 2021 •

edited

Loading

jcouv Jan 14, 2021 •

edited

Loading

jcouv Jan 14, 2021 •

edited

Loading

jcouv Jan 14, 2021 •

edited by AlekseyTs

Loading

jcouv Jan 14, 2021 •

edited

Loading

jcouv Jan 14, 2021 •

edited

Loading

333fred commented Jan 14, 2021 •

edited by AlekseyTs

Loading

333fred Jan 14, 2021 •

edited by AlekseyTs

Loading

333fred Jan 14, 2021 •

edited by AlekseyTs

Loading

333fred Jan 14, 2021 •

edited by AlekseyTs

Loading

AlekseyTs Jan 14, 2021 •

edited

Loading