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

Use ReadOnlySpan properties over array fields #56845

Conversation

NewellClark
Copy link
Contributor

While working on this analyzer I found and fixed several violations in dotnet/roslyn.

@NewellClark NewellClark requested a review from a team as a code owner September 29, 2021 13:44
@ghost ghost added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Sep 29, 2021
@CyrusNajmabadi
Copy link
Member

There are likely similar hits in the non-compiler layers. If so, can you make a PR for that as well (independent of this one)? I'd be happy to review it and get it in asap for the higher layers.

@NewellClark
Copy link
Contributor Author

There are likely similar hits in the non-compiler layers. If so, can you make a PR for that as well (independent of this one)? I'd be happy to review it and get it in asap for the higher layers.

I ran the analyzer on the entire dotnet/roslyn repository, not just the projects in the Compiler folder. I think the bot mislabeled this PR.

@CyrusNajmabadi
Copy link
Member

I think the bot mislabeled this PR.

THe bot is correct here (since you only touched the compiler layer). I am surprised though as i'm sure we have read-only arrays at the IDE layer that could benefit from this. For example:

private static readonly char[] s_dotCharacterArray = { '.' };

@NewellClark
Copy link
Contributor Author

NewellClark commented Sep 29, 2021

I think the bot mislabeled this PR.

THe bot is correct here (since you only touched the compiler layer). I am surprised though as i'm sure we have read-only arrays at the IDE layer that could benefit from this. For example:

private static readonly char[] s_dotCharacterArray = { '.' };

At the moment, the analyzer only reports arrays for byte, sbyte, and bool because at present only arrays of those types containing all constant members are optimized by the compiler.

@CyrusNajmabadi
Copy link
Member

At the moment, the analyzer only reports arrays for byte, sbyte, and bool because at present the inlining optimization only happens for 1-byte primitive types.

Ah. I'd like this not for any sort of optimization, but just for general safety. Those arrays should be immutable or ROS's so that no one can accidentally change their contents.

@NewellClark
Copy link
Contributor Author

@CyrusNajmabadi It may be a good idea to create a separate rule that finds all static readonly array fields or auto-properties that are never mutated and suggests converting them to ImmutableArray. I'd be happy to implement it if it gets approved.

@CyrusNajmabadi
Copy link
Member

Tagging @mavasani I think it would be great to have that. If he agrees then I think roslyn-analyzers would be the right place.

@jaredpar
Copy link
Member

jaredpar commented Oct 7, 2021

@RikkiGibson, @chsienki PTAL

@jaredpar jaredpar self-assigned this Oct 7, 2021
@chsienki chsienki self-assigned this Oct 7, 2021
Copy link
Contributor

@RikkiGibson RikkiGibson left a comment

Choose a reason for hiding this comment

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

LGTM, you can resolve the nit or not at your discretion and let us know when the PR is ready for merge.

src/Compilers/Test/Core/Assert/AssertEx.cs Outdated Show resolved Hide resolved
@RikkiGibson
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@RikkiGibson RikkiGibson self-assigned this Jan 8, 2022
@RikkiGibson RikkiGibson reopened this Jan 10, 2022
@RikkiGibson
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@RikkiGibson
Copy link
Contributor

Thanks for the contribution @NewellClark!

@RikkiGibson RikkiGibson merged commit f29f937 into dotnet:main Jan 11, 2022
@ghost ghost modified the milestones: 17.1, Next Jan 11, 2022
@RikkiGibson RikkiGibson modified the milestones: Next, 17.2.P1 Feb 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants