DotNetTypeAdapter - treat init-only properties as non-settable #16520

IISResetMe · 2021-11-25T21:31:11Z

PR Summary

This PR modifies the .NET type adapter to respect the IsExternalInit modifier on property setters.

IsExternalInit is the mechanism through which the C# 9.0 specification implements support for init-only setters - properties that can only be set during instantiation, after which they're considered read-only by the compiler.

By inspecting the property setter's return parameter we can similarly discover the IsExternalInit modifier on runtime types, and change the behavior of ETS to reject attempts to set init-only properties by having Adapter.PropertyIsSettable() reflect whether the modifier was found.

PR Context

As reported in #13819, PowerShell has no current facility for detecting init-only
properties, and what the C# compiler would now treat as read-only properties are suddenly writable/settable at runtime:

PS ~> $PSVersionTable['PSVersion'] -as [string]
7.2.0
PS ~> Add-Type @'
>> public class InitOnlyValue {public long Value {get; init;}}
>> '@
PS ~> $value = [InitOnlyValue]@{ Value = 123 }
PS ~> $value

Value
-----
  123

PS ~> $value.Value = 456
PS ~> $value

Value
-----
  456

PS ~> $value |Get-Member -MemberType Properties

   TypeName: InitOnlyValue

Name  MemberType Definition
----  ---------- ----------
Value Property   long Value {get;set;}

After we apply the changes in this PR, PowerShell's behavior reflects that of C# (and VB.NET):

PS ~> $value = [InitOnlyValue]@{ Value = 123 }
PS ~> $value

Value
-----
  123

PS ~> $value.Value = 456
InvalidOperation: 'Value' is a ReadOnly property.
PS ~> $value |Get-Member -MemberType Property

   TypeName: InitOnlyValue

Name  MemberType Definition
----  ---------- ----------
Value Property   long Value {get;}

PR Checklist

…e by ETS

iSazonov · 2021-11-26T03:19:26Z

@SeeminglyScience @JustinGrote Please review.

src/System.Management.Automation/engine/CoreAdapter.cs

test/powershell/engine/ETS/Adapter.Tests.ps1

src/System.Management.Automation/engine/CoreAdapter.cs

SeeminglyScience

Looks great! I echo @iSazonov's comment about maybe omitting the initOnly field. Did the hashtable conversion path (aka object init) code path not need any changes?

src/System.Management.Automation/engine/CoreAdapter.cs

Co-authored-by: Patrick Meinecke <SeeminglyScience@users.noreply.github.com>

IISResetMe · 2021-11-26T14:48:51Z

Looks great! I echo @iSazonov's comment about maybe omitting the initOnly field. Did the hashtable conversion path (aka object init) code path not need any changes?

I'm reluctant since it makes the adapter code more readable to have the distinction IMO, but I'm happy to remove it if there are any arguments better than "technically it's not strictly needed" available.

WRT hashtable-initialization: Nope - this is the beauty of using the type adapter - from a hashtable cast, the compiler will generate an expression tree that 1) invokes the default constructor on the target type and then 2) sets the properties and fields subsequently. Since this happens directly against the runtime and not via ETS, our new settability-restriction doesn't apply

Omit test for specific (localized) error message text, not reliable

ghost · 2021-12-08T02:00:42Z

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

IISResetMe · 2021-12-31T00:28:33Z

What are we waiting for here? Anything I can do to move it forward?

src/System.Management.Automation/engine/CoreAdapter.cs

PaulHigin · 2022-01-03T18:06:21Z

@daxian-dbw Can you please review?

IISResetMe · 2022-01-07T16:00:34Z

Looks like the last commit pushed the "total complexity" of CoreAdapter.cs over some arbitrary threshold determined by CodeFactor.

Just to set expectations: I am NOT going to refactor 6000 lines of half-a-decade-old code as part of this PR ^_^

ghost · 2022-01-15T02:00:42Z

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

IISResetMe · 2022-01-20T20:50:05Z

@daxian-dbw *nudge nudge* :)

daxian-dbw · 2022-01-20T21:39:48Z

@IISResetMe Sorry about the delays. I will try to get to this PR next week.

daxian-dbw · 2022-01-26T20:27:34Z

@IISResetMe Sorry that I won't be able to get to this PR this week. All my pre-allocated PR review time will most likely be spent on #12412 this week (I started the review last week and it took more time than I thought). Hopefully, I will get to this PR the next week.

iSazonov · 2022-01-27T05:21:26Z

@IISResetMe Sorry that I won't be able to get to this PR this week. All my pre-allocated PR review time will most likely be spent on #12412 this week (I started the review last week and it took more time than I thought). Hopefully, I will get to this PR the next week.

Ah! Who is the person who could double that time besides God? 😄

IISResetMe · 2022-01-27T12:41:16Z

Time to commence the @daxian-dbw cloning program :)

Thanks for the heads up, I fully understand

ghost · 2022-02-03T14:00:48Z

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

src/System.Management.Automation/engine/CoreAdapter.cs

daxian-dbw · 2022-02-05T00:10:44Z

src/System.Management.Automation/engine/CoreAdapter.cs

@@ -2788,6 +2788,14 @@ internal PropertyCacheEntry(PropertyInfo property)
                if (propertySetter != null && (propertySetter.IsPublic || propertySetter.IsFamily))
                {
                    this.isStatic = propertySetter.IsStatic;
+                    foreach (Type requiredCustomModifier in propertySetter.ReturnParameter.GetRequiredCustomModifiers())
+                    {
+                        if (!requiredCustomModifier.IsNested && requiredCustomModifier.FullName == "System.Runtime.CompilerServices.IsExternalInit")


Please add comments here to indicate that this is for handling the init concept introduced in C# 9.

daxian-dbw · 2022-02-05T01:04:11Z

src/System.Management.Automation/engine/CoreAdapter.cs

@@ -2788,6 +2788,14 @@ internal PropertyCacheEntry(PropertyInfo property)
                if (propertySetter != null && (propertySetter.IsPublic || propertySetter.IsFamily))
                {
                    this.isStatic = propertySetter.IsStatic;
+                    foreach (Type requiredCustomModifier in propertySetter.ReturnParameter.GetRequiredCustomModifiers())


@IISResetMe Sorry for taking so long. The changes in this PR do not prevent PSProperty from being changed through PSProperty.Value = xxx for an init property. Example (with changes in this PR):

> Add-Type -TypeDefinition @' public class InitOnlyTestClass { // default constructor to allow cast-initialization public InitOnlyTestClass() { } public long Value { get; init; } } '@ > $s = [InitOnlyTestClass]::new() > $p = $s.psobject.Properties['Value'] > $p.IsSettable False > $p.Value = 1000 > $p MemberType : Property Value : 1000 IsSettable : False IsGettable : True TypeNameOfValue : System.Int64 Name : Value IsInstance : True

The hashtable-to-object conversion actually goes through ConvertViaNoArgumentConstructor.Convert, and depends on setting PSProperty.Value.
This is the tricky part, for an init property, PSProperty.Value should throw when setting value to it, but the [type]@{ property1 = value1; property2 = value2 } depends on setting PSProperty.Value to work. So, here we got a contradiction :(

I'm almost inclined to say that's a restriction we could document. Going through PSObject to target the underlying metadata and modify the PSProperty directly is almost akin to using Reflection to modify the value in C# -- and in Reflection, too, I don't think init-only holds any water.

I personally don't think we should introduce a language feature that is inconsistent. If a user is doing reflection directly, then it's out of our hands. But if the user is doing it in the PowerShell way, then consistent behavior should be expected.

@daxian-dbw I agree behavior should be consistent - but I'm inclined to argue that inconsistent is consistent with the behavior we're trying to mimic here.

Init-only properties are an illusion, in that it requires the consenting participation of all compilers. The C#, VB and F# implementations all introduce compile-time guardrails, no actual runtime-constraints or CTS-specific flags to indicate any behaviorial difference between properties defined with init; and auto properties. Because there is no such flags. The core design of CTS anticipates no such distinction :)

We can mimic the runtime-inconsistent behavior that the dominant .NET languages have converged on (which is what this PR attempts), or we can decide to adapt differently (because PowerShell is different and its runtime has a different lifecycle model).

Perhaps I should close this PR and leave that question up to the language WG? 😄

Co-authored-by: Dongbo Wang <dongbow@microsoft.com>

pull-request-quantifier-deprecated · 2022-02-05T15:55:45Z

This PR has 29 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

Quantification details

Label      : Extra Small
Size       : +27 -2
Percentile : 11.6%

Total files changed: 2

Change summary by file extension:
.cs : +8 -2
.ps1 : +19 -0

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

Fast and predictable releases to production:
- Optimal size changes are more likely to be reviewed faster with fewer
  iterations.
- Similarity in low PR complexity drives similar review times.
Review quality is likely higher as complexity is lower:
- Bugs are more likely to be detected.
- Code inconsistencies are more likely to be detetcted.
Knowledge sharing is improved within the participants:
- Small portions can be assimilated better.
Better engineering practices are exercised:
- Solving big problems by dividing them in well contained, smaller problems.
- Exercising separation of concerns within the code changes.

What can I do to optimize my changes

Use the PullRequestQuantifier to quantify your PR accurately
- Create a context profile for your repo using the context generator
- Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
- Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
- Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
Change your engineering behaviors
- For PRs that fall outside of the desired spectrum, review the details and check if:
  - Your PR could be split in smaller, self-contained PRs instead
  - Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

One line was added: +1 -0
One line was deleted: +0 -1
One line was modified: +1 -1 (git diff doesn't know about modified, it will
interpret that line like one addition plus one deletion)
Change percentiles: Change characteristics (addition, deletion, modification)
of this PR in relation to all other PRs within the repository.

Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.

IISResetMe · 2022-02-15T14:42:50Z

Closed based on @daxian-dbw's comments about consistency (and lack of consensus around what that means in the context of PowerShell for this specific feature)

.NET Properties with init; setters should be treated as non-settabl…

053bb31

…e by ETS

ghost assigned PaulHigin Nov 25, 2021

pull-request-quantifier-deprecated bot added the Extra Small label Nov 25, 2021

IISResetMe added 2 commits November 25, 2021 23:08

Add init-only property tests

de1db16

Fix init-only negative test

3ccea20

IISResetMe changed the title ~~WIP: DotNetTypeAdapter - treat init-only properties as non-settable~~ DotNetTypeAdapter - treat init-only properties as non-settable Nov 25, 2021

IISResetMe marked this pull request as ready for review November 25, 2021 22:48

iSazonov requested a review from daxian-dbw November 26, 2021 03:17

iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Nov 26, 2021

iSazonov requested a review from vexx32 November 26, 2021 03:18

iSazonov reviewed Nov 26, 2021

View reviewed changes

src/System.Management.Automation/engine/CoreAdapter.cs Outdated Show resolved Hide resolved

test/powershell/engine/ETS/Adapter.Tests.ps1 Outdated Show resolved Hide resolved

iSazonov reviewed Nov 26, 2021

View reviewed changes

src/System.Management.Automation/engine/CoreAdapter.cs Outdated Show resolved Hide resolved

SeeminglyScience reviewed Nov 26, 2021

View reviewed changes

src/System.Management.Automation/engine/CoreAdapter.cs Outdated Show resolved Hide resolved

src/System.Management.Automation/engine/CoreAdapter.cs Outdated Show resolved Hide resolved

IISResetMe and others added 2 commits November 26, 2021 15:29

Update src/System.Management.Automation/engine/CoreAdapter.cs

5ceb9c6

Co-authored-by: Patrick Meinecke <SeeminglyScience@users.noreply.github.com>

Update src/System.Management.Automation/engine/CoreAdapter.cs

74c3afd

Co-authored-by: Patrick Meinecke <SeeminglyScience@users.noreply.github.com>

Update Adapter.Tests.ps1

5b8b0c7

Omit test for specific (localized) error message text, not reliable

ghost added the Review - Needed The PR is being reviewed label Dec 8, 2021

PaulHigin added WG-Language parser, language semantics WG-Engine core PowerShell engine, interpreter, and runtime and removed WG-Language parser, language semantics labels Jan 3, 2022

ghost removed the Review - Needed The PR is being reviewed label Jan 3, 2022

PaulHigin suggested changes Jan 3, 2022

View reviewed changes

src/System.Management.Automation/engine/CoreAdapter.cs Show resolved Hide resolved

src/System.Management.Automation/engine/CoreAdapter.cs Outdated Show resolved Hide resolved

ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Jan 3, 2022

ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Jan 6, 2022

Extract repeat PropertCacheEntry-cast in IsSettable adapter

989839a

ghost added the Review - Needed The PR is being reviewed label Jan 15, 2022

ghost removed the Review - Needed The PR is being reviewed label Jan 20, 2022

IISResetMe mentioned this pull request Jan 21, 2022

Implement init-only property syntax IISResetMe/PSType-vNext#6

Open

3 tasks

ghost added the Review - Needed The PR is being reviewed label Feb 3, 2022

daxian-dbw requested changes Feb 5, 2022

View reviewed changes

ghost added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Review - Needed The PR is being reviewed labels Feb 5, 2022

Update src/System.Management.Automation/engine/CoreAdapter.cs

28fbf35

Co-authored-by: Dongbo Wang <dongbow@microsoft.com>

ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Feb 5, 2022

daxian-dbw added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Feb 8, 2022

ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Feb 8, 2022

IISResetMe closed this Feb 15, 2022

DotNetTypeAdapter - treat init-only properties as non-settable #16520

DotNetTypeAdapter - treat init-only properties as non-settable #16520

Uh oh!

Conversation

IISResetMe commented Nov 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Context

PR Checklist

Uh oh!

iSazonov commented Nov 26, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SeeminglyScience left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

IISResetMe commented Nov 26, 2021

Uh oh!

ghost commented Dec 8, 2021

Uh oh!

IISResetMe commented Dec 31, 2021

Uh oh!

Uh oh!

Uh oh!

PaulHigin commented Jan 3, 2022

Uh oh!

IISResetMe commented Jan 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jan 15, 2022

Uh oh!

IISResetMe commented Jan 20, 2022

Uh oh!

daxian-dbw commented Jan 20, 2022

Uh oh!

daxian-dbw commented Jan 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov commented Jan 27, 2022

Uh oh!

IISResetMe commented Jan 27, 2022

Uh oh!

ghost commented Feb 3, 2022

Uh oh!

Uh oh!

daxian-dbw Feb 5, 2022

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Feb 5, 2022 • 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!

vexx32 Feb 5, 2022

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Feb 7, 2022

Choose a reason for hiding this comment

Uh oh!

IISResetMe Feb 8, 2022

Choose a reason for hiding this comment

Uh oh!

pull-request-quantifier-deprecated bot commented Feb 5, 2022

What can I do to optimize my changes

How to interpret the change counts in git diff output

Uh oh!

IISResetMe commented Feb 15, 2022

Uh oh!

Uh oh!

IISResetMe commented Nov 25, 2021 •

edited

Loading

IISResetMe commented Jan 7, 2022 •

edited

Loading

daxian-dbw commented Jan 26, 2022 •

edited

Loading

daxian-dbw Feb 5, 2022 •

edited

Loading