GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Mark applicable structs as readonly and use in-modifier by xtqqczze · Pull Request #13919 · PowerShell/PowerShell

Conversation

xtqqczze commented Oct 28, 2020 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

Work toward #13915.

Changes touch public APIs but do not cause any overload resolution changes, so in theory are non-breaking:

Uh oh!

There was an error while loading. Please reload this page.

All reactions

ghost assigned Rob Holt (rjmholt)

Oct 28, 2020

xtqqczze mentioned this pull request

Oct 28, 2020

Fix IDE0044: MakeFieldReadonly part 6 #13889

Merged

xtqqczze force-pushed the GH13915 branch 2 times, most recently from f49168e to a739002 Compare

October 29, 2020 00:20

xtqqczze changed the title ~~Mark non-mutable structs readonly~~ Mark applicable structs as readonly

Oct 29, 2020

xtqqczze force-pushed the GH13915 branch from 989aa4a to 471b494 Compare

October 29, 2020 02:41

xtqqczze changed the title ~~Mark applicable structs as readonly~~ Mark applicable structs as readonly and use in-modifier

Oct 30, 2020

xtqqczze added 8 commits

October 30, 2020 00:16


          Fix IDE0044: MakeFieldReadonly

3e3f879


          Fix EPS01: Struct can be made readonly

b16e94a


          Fix EPS05: Use in-modifier for passing a readonly struct

e3fde33


          Fix IDE0044: MakeFieldReadonly

458d2e6


          Replace ref-modifier with in-modifier in method signatures

656a101


          Fix CS8329

bd43a69


          Fix CS1615

301f083


          Revert addition of readonly modifier for no-member types

be45267

xtqqczze force-pushed the GH13915 branch from 9645274 to be45267 Compare

October 30, 2020 00:21

xtqqczze marked this pull request as ready for review

October 30, 2020 00:28

xtqqczze requested review from Aditya Patwardhan (adityapatwardhan), Andrew (anmenaga) and Dongbo Wang (daxian-dbw) as code owners

October 30, 2020 00:28

ghost added the Review - Needed The PR is being reviewed label

Nov 6, 2020

Deleted user (ghost) commented Nov 6, 2020

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Rob Holt (rjmholt) reviewed

Nov 10, 2020

View reviewed changes

Comment thread

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

		/// can only be used as a switch.
		/// </summary>
		public struct SwitchParameter
		public readonly struct SwitchParameter

Rob Holt (rjmholt) Nov 10, 2020

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one could be an issue -- SwitchParameter is a very widely used public API

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

xtqqczze Nov 10, 2020

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But SwitchParameter is immutable by design, and adding the readonly-modifier doesn't cause any overload resolution changes [1], so I think this is not a breaking change.

[1] https://blog.paranoidcoding.com/2019/03/27/readonly-struct-breaking-change.html

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Rob Holt (rjmholt) Nov 11, 2020

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I'm fairly convinced by this

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/Microsoft.PowerShell.CoreCLR.Eventing/DotNetCode/Eventing/EventDescriptor.cs

		{
		[StructLayout(LayoutKind.Explicit, Size = 16)]
		public struct EventDescriptor
		public readonly struct EventDescriptor

Rob Holt (rjmholt) Nov 10, 2020

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about changing public structs to readonly -- existing code that relies on the previous form may be affected

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

xtqqczze Nov 10, 2020

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This struct is immutable by design.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/Microsoft.PowerShell.CoreCLR.Eventing/DotNetCode/Eventing/EventProvider.cs

		/// <param name="eventPayload">
		/// </param>
		public bool WriteEvent(ref EventDescriptor eventDescriptor, params object[] eventPayload)
		public bool WriteEvent(in EventDescriptor eventDescriptor, params object[] eventPayload)

Rob Holt (rjmholt) Nov 10, 2020

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing the signature on a public method like this is a breaking change

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

xtqqczze Nov 10, 2020

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The compiler doesn't permit overloading on ref/out/in differences, so this change is non-breaking.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Rob Holt (rjmholt) Nov 11, 2020

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically the issue isn't with the compiler, but with the runtime. Modules compiled against the old API surface must continue to work after any changes are made to the API.

But I tested it out with a toy project (an exe and a lib, where I built an in version of the lib and then changed it so the exe is compiled against a ref version) and it seems to work.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

xtqqczze Nov 11, 2020

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The in, ref, and out keywords are not considered part of the method signature for the purpose of overload resolution. Therefore, methods cannot be overloaded if the only difference is that one method takes a ref or in argument and the other takes an out argument.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Rob Holt (rjmholt) Nov 11, 2020

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's still C# compiler rules, where the compiler statically decides which overload to resolve for a call. My concern was that the resolved method call in the IL might not match what the library was originally compiled against when its loaded at runtime (so even though the original program isn't broken it still requires recompilation, which is a dealbreaker for modules). It makes sense that that's not the case though.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

xtqqczze reacted with thumbs up emoji

All reactions

👍 1 reaction

Rob Holt (rjmholt) commented Nov 11, 2020

Copy link

Copy Markdown

Collaborator

Dongbo Wang (@daxian-dbw) can you review please?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ghost removed the Review - Needed The PR is being reviewed label

Nov 11, 2020

xtqqczze mentioned this pull request

Nov 11, 2020

Enable IDE0044: MakeFieldReadonly #13880

Merged

ghost added the Review - Needed The PR is being reviewed label

Nov 19, 2020

Deleted user (ghost) commented Nov 19, 2020

Copy link

Copy Markdown

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          Merge branch 'master' into GH13915

b5ed5ef

Rob Holt (rjmholt) approved these changes

Dec 9, 2020

View reviewed changes

Rob Holt (rjmholt) merged commit 41ab20c into PowerShell:master

Dec 9, 2020

ghost removed the Review - Needed The PR is being reviewed label

Dec 9, 2020

Rob Holt (rjmholt) added the CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log label

Dec 9, 2020

xtqqczze deleted the GH13915 branch

December 9, 2020 20:35

Deleted user (ghost) commented Dec 15, 2020

Copy link

Copy Markdown

🎉v7.2.0-preview.2 has been released which incorporates this pull request.:tada:

Handy links:

Release Notes

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request

Aug 6, 2026


          Mark applicable structs as readonly and use in-modifier (PowerShell#1…

3b13543

…3919)

Reviewers

Aditya Patwardhan (adityapatwardhan)

Awaiting requested review from adityapatwardhan

Andrew (anmenaga)

Awaiting requested review from anmenaga

Dongbo Wang (daxian-dbw)

Awaiting requested review from daxian-dbw

1 more reviewer

Rob Holt (rjmholt)

rjmholt approved these changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

Rob Holt (rjmholt)

Labels

CL-CodeCleanup

Indicates that a PR should be marked as a Code Cleanup change in the Change Log

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

xtqqczze commented Oct 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Deleted user (ghost) commented Nov 6, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rob Holt (rjmholt) commented Nov 11, 2020

Uh oh!

Deleted user (ghost) commented Nov 19, 2020

Uh oh!

Deleted user (ghost) commented Dec 15, 2020

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

xtqqczze commented Oct 28, 2020 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.