Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Results of running 'make member readonly' on IDE #66957

Merged
merged 4 commits into from
Feb 27, 2023

Conversation

CyrusNajmabadi
Copy link
Member

No description provided.

{
}

object? IEnumerator.Current
readonly object? IEnumerator.Current
Copy link
Member

Choose a reason for hiding this comment

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

❔ What's the point of this, considering there is no way to directly invoke this member such that it will benefit from the readonly designation?

Copy link
Member Author

Choose a reason for hiding this comment

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

marks intent that it should not mutate state. can go along with anohter analyzer (or the compiler) that tells you if you violate this and actually mutate this guy (or cause it to be copied).

Copy link
Member

Choose a reason for hiding this comment

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

I'm concerned this may add noise and/or confusion for people that don't understand how interface dispatch works today. In almost all cases where an explicitly interface implementation is called, the member is already operating on a copy that was created for that one call. Exceptions to this include:

  1. The user is operating on a storage location which is statically-typed to the interface and not to the value type. In this case, the use of readonly isn't apparent to the caller at all.
  2. The user has gone to strange lengths to ensure a constrained call is made without copying/boxing a value.

I see arguments for both keeping this behavior and skipping this behavior. Maybe discuss with the team before we merge?

Copy link
Member Author

Choose a reason for hiding this comment

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

In almost all cases where an explicitly interface implementation is called, the member is already operating on a copy that was created for that one call. Exceptions to this include:

I actually disagree with this. :) Passing a value type to a generic that is constrained on an interface is a normal pattern, and we expect to push it further in C#.

The purpose of readonly here is important as a documentation/protection mechanism (similar to 'static' on a lambda). The presence of it means no chance of mutation happening in the future (similar to how 'static' prevents capture in the future).

I can bring to team to see. But this seems like a pure benefit. I'm not sure what could be bad/undesirable about marking these members this way :)

@@ -386,10 +386,10 @@ public Enumerator(SegmentedArray<T> array)
_current = default!;
}

public T Current => _current;
public readonly T Current => _current;
object? IEnumerator.Current => Current;
Copy link
Member

@sharwell sharwell Feb 20, 2023

Choose a reason for hiding this comment

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

❔ Why is this one not marked readonly, but the one below (SegmentedDictionary`2.cs) is?

Copy link
Member Author

Choose a reason for hiding this comment

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

so the system is not multi-pass. so first pass mae T Current readonly. Another pass would then let followup members become readonly. This is similar to how 'make static' and otehr similar features work.

@@ -29,7 +29,7 @@ protected struct DocumentWithInfo
public string Text { get; set; }
public string DocumentName { get; set; }
public string DocumentFilePath { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

❔ Is there any difference between { get; set; } and { readonly get; set; }?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes. it will allow hte compiler/analyzers to actually validate that the getter does not mutate/copy this.

Copy link
Member

Choose a reason for hiding this comment

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

Note that I'm asking about literally this exact form, where in all cases the contents of both accessors are compiler-generated.

Copy link
Member Author

Choose a reason for hiding this comment

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

@CyrusNajmabadi CyrusNajmabadi marked this pull request as ready for review February 21, 2023 19:30
@CyrusNajmabadi CyrusNajmabadi requested review from a team as code owners February 21, 2023 19:30
@@ -26,7 +26,7 @@ internal BitHelper(Span<int> span, bool clear)
_span = span;
}

internal void MarkBit(int bitPosition)
internal readonly void MarkBit(int bitPosition)
Copy link
Member

Choose a reason for hiding this comment

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

❔ Why mark the members readonly instead of the containing type?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, you get both messages here. and making the type readonly def makes sense.

@CyrusNajmabadi
Copy link
Member Author

@sharwell have rerun using the updated logic you suggested.

@CyrusNajmabadi
Copy link
Member Author

@sharwell ptal.

@CyrusNajmabadi CyrusNajmabadi merged commit e3d76d0 into dotnet:main Feb 27, 2023
@ghost ghost added this to the Next milestone Feb 27, 2023
@CyrusNajmabadi CyrusNajmabadi deleted the makeMemberReadOnly2 branch February 27, 2023 22:04
@RikkiGibson RikkiGibson modified the milestones: Next, 17.6 P2 Mar 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants