Methods "readonly" annotation

[hrrrrustic]

Close #1718
I wrote a little Roslyn-based program that automate most of the work (I'm only rechecking changes in git diff) and pushed one file to init. But I'm ready to mark all methods (and properties) in src\libraries\ in NOT readonly struct with readonly keyword.
But I'm not sure should I mark method/property as readonly or not if it is a explicit interface implementation

Also how it would be better to commit changes if it's important? Commit per file or one huge commit?
As I see now, there will be at least 1300 methods in 307 files (without tests directories)

[ghost]

Tagging subscribers to this area: @eerhardt, @maryamariyan
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Close #1718
I wrote a little Roslyn-based program that automate most of the work (I'm only rechecking changes in git diff) and pushed one file to init. But I'm ready to mark all methods (and properties) in src\libraries\ in NOT readonly struct with readonly keyword.
So there are a few cases where I'm not sure should I mark method/property as readonly or not

1. Method is just throwing exception (e.g. NotImplementedException)
2. Explicit interface implantation
3. Unsafe (method or the whole struct)
4. Marked as obsolete (Not sure that those exist, but have no idea what to do with if I meet them)
5. Property with auto {get; set;} . It seems a little bit weird :D
6. \tests\ directories

Also how it would be better to commit changes if it's important? Commit per file or one huge commit?
As I see now, there will be at least 1300 methods in 307 files (without tests directories)

Author:	hrrrrustic
Assignees:	-
Labels:	area-Extensions-DependencyInjection
Milestone:	-

[stephentoub]

Thanks for working on this.

This was mentioned in the issue, but I want to call it out here again. We need to be careful with this; once a method is marked as readonly, that becomes part of the contract, and it's a breaking change to remove it, so just because the implementation enables it to be readonly today doesn't mean we actually want it to be constrained as such forever. We need to review every method to which readonly is added and confirm we're ok with the constraint. @tannergooding's key statement from the linked issue: "annotate methods on non-readonly structs which do not and will never mutate the state of the instance" (emphasis is mine).

[stephentoub]

This is an example of a kind of change that makes me a tad nervous (even if ultimately we decide this case is ok). This method does mutate, it's just mutating state in a stored reference type. If the implementation were to change in the future, such that some of this state were actually stored as fields in the struct, it would no longer be readonly.

[hrrrrustic]

Yeah, that's true, but...mostly I can't decide by myself is it ok or not :( . So it seems like I have to mark those methods anyway and wait here for review, isn't it?
But for the start I can skip all methods that contains .Add/.Append and so on to minimize false positive cases

[stephentoub]

So it seems like I have to mark those methods anyway and wait here for review, isn't it?

Yup, a reasonable approach as you suggest is to programmatically mark for review all that could potentially be made readonly, and then back off from that list based on fully reviewing it prior to committing.

[hrrrrustic]

rebase on top master

[hrrrrustic]

No idea why CI failed

[hrrrrustic]

Btw can't mark cases like that:

runtime/src/libraries/System.Speech/src/Synthesis/TTSEngine/TTSEngineTypes.cs

Line 62 in 308ae6a

public short EventId { get { return _eventId; } internal set { _eventId = value; } }

There will be an error in ref file that property getter can't be marked readonly without setter (ref file contains only public getter)

[jeffhandley]

Also how it would be better to commit changes if it's important? Commit per file or one huge commit?

We need to review every method to which readonly is added and confirm we're ok with the constraint.

Instead of having 1 large PR, we'd probably be better off having multiple PRs separated by area so that each set of area owners could review the changes to the APIs within their areas. We recently did something similar for backporting API docs into /// comments where we created an issue for each library with instructions for area owners to run the tool.

@hrrrrustic, how flexible is the tool you built in terms of breaking this down? If we gave you a list of areas to cluster together into separate PRs (grouping by "area pod"), would you be able to create separate PRs to match that mapping?

[jeffhandley]

@hrrrrustic, how flexible is the tool you built in terms of breaking this down? If we gave you a list of areas to cluster together into separate PRs (grouping by "area pod"), would you be able to create separate PRs to match that mapping?

@hrrrrustic Would you be able to split the PR up like that? If so, I can provide the list. Thanks!

[hrrrrustic]

@jeffhandley Sorry for delay, create some separated PR's. That's not all, got some troubles with splitting, but remaining changes are on another computer :(
I'll force push them here on Monday

[jeffhandley]

@hrrrrustic No worries on the delay at all! This is really helpful -- thanks for splitting the PR up like that. 💯

With all of those PRs getting filed, this got the attention of several area owners across the libraries and we've spawned off some discussions after cursory reviews. We are realizing that we've never really thought about what our strategy around readonly should be.

Some aspects we're considering:

1. For every API touched, we have to evaluate it through the lens of whether we'd ever want to allow mutation to occur -- it would be a breaking change to remove readonly later. For example, adding caching to a readonly method later would be a breaking change. 😮
2. How we quantify the value that readonly provides across different areas relative to that possibility
3. If we can define and document API design guidance to follow both with these PRs and for all APIs going forward

There are some areas such as numerics where we think the conclusions will be easier to reach than others. Even for the easier ones though, we might still hold off on the PRs until we can document and review such a strategy. With that realization, it's likely to be a while before we can move the PRs forward. In the meantime, I'd like to:

1. Update Determine Libraries strategy for using the readonly members annotation #1718 to indicate that we need to create documented guidance for when/where we should/shouldn't apply readonly
2. Mark all of the PRs produced here as DRAFT and blocked for the moment, commenting that we need to solidify our guidance first
3. If we're not able to document that guidance within the next 30 days, close those PRs (with the intent of re-opening PRs later where we deem it appropriate)

It seems like the tool you created to do this scan and to produce the PRs was really powerful. Is it something you'd be willing to push to a repo and link to?

Again, these discussions weren't on our radar until you split your PR up like this, so we're very thankful for your effort here. Please let me know if you have any concerns or feedback on the action plan above.

Thanks!

[hrrrrustic]

@jeffhandley

If we're not able to document that guidance within the next 30 days, close those PRs (with the intent of re-opening PRs later where we deem it appropriate)

There is an issue about this

It seems like the tool you created to do this scan and to produce the PRs was really powerful

It's only do scanning and marking. I believe that producing separates PR could be done programmatically, but I've never tried github api. So I decided that it would be faster for me to simply did it manually via soft resetting this PR and grouping changes by folders in git GUI :)

Is it something you'd be willing to push to a repo and link to?

Yup, but I need a little cleanup,
Actually marking was iterative - I ran it, committed changes, saw some cases where it didn't triggered, fixed some conditions and ran it again. So TBH I'm not sure that if I rerun final version it will produce same result xD

Please let me know if you have any concerns or feedback on the action plan above.

Not actually about plan, but maybe it would be great to add kind of reminder in bot messages about splitting changes for big PRs where area owner wasn't figured out (looks like PR spread for several areas)

[jeffhandley]

Thanks for the flexibility, @hrrrrustic. As you saw, we had a couple of areas where we were able to move forward because the decisions were straightforward. As the issue for closing/re-opening PRs uncovered, re-opening will require area owners to take that action.

Thanks also for sharing notes on how you approached this. There's no obligation or pressure for you sharing the code you had for this--you've already helped out a bunch on this.

I also appreciate the notes about the experience of creating a PR that affects multiple areas like this; that's helpful feedback. 👍

[ghost]

Draft Pull Request was automatically closed for inactivity. Please let us know if you'd like to reopen it.