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

Fix warnings found with CA1861 Avoid constant arrays as arguments #86229

Merged
merged 16 commits into from
May 31, 2023

Conversation

buyaa-n
Copy link
Contributor

@buyaa-n buyaa-n commented May 14, 2023

Enable new analyzer CA1861: Avoid constant arrays as arguments. And fix warnings found in runtime.

@ghost
Copy link

ghost commented May 14, 2023

Tagging subscribers to this area: @dotnet/area-meta
See info in area-owners.md if you want to be subscribed.

Issue Details

Enable new analyzer CA1861: Avoid constant arrays as arguments. And fix warnings found in runtime.

Author: buyaa-n
Assignees: buyaa-n
Labels:

area-Meta

Milestone: -

@danmoseley
Copy link
Member

This is a potential deoptimization if the call is rarely made, right?

@buyaa-n buyaa-n requested review from lewing, pavelsavara and kg as code owners May 15, 2023 22:20
@kg
Copy link
Member

kg commented May 15, 2023

Mono parts look fine

@jkotas
Copy link
Member

jkotas commented May 16, 2023

I see that the analyzer has auto-fixer. Is the auto-fixer smart enough to skip cases where the array is mutated later? I am just wondering about potential correctness problems introduced by the auto-fixer.

@buyaa-n
Copy link
Contributor Author

buyaa-n commented May 16, 2023

I see that the analyzer has auto-fixer. Is the auto-fixer smart enough to skip cases where the array is mutated later?

It only flag literal arrays and suggest a fixer

@radical
Copy link
Member

radical commented May 16, 2023

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@stephentoub
Copy link
Member

I see that the analyzer has auto-fixer. Is the auto-fixer smart enough to skip cases where the array is mutated later?

It only flag literal arrays and suggest a fixer

I believe Jan is expressing concern about a case like this:

char[] array = new char[] { 'a', 'b', 'c' };
array[0] = Read();
Use(array);

Will the analyzer flag that and auto-fix it to:

private static readonly char[] s_cached = new char[] { 'a', 'b', 'c' };
...
char[] array = s_cached;
array[0] = Read();
Use(array);

? If it would, that's obviously not safe.

@buyaa-n
Copy link
Contributor Author

buyaa-n commented May 30, 2023

I believe now there is no any blocker or open question/comment for this PR, please finish the review @danmoseley @jkotas @kg @stephentoub, thank you!

@buyaa-n buyaa-n requested a review from danmoseley May 30, 2023 23:32
@buyaa-n
Copy link
Contributor Author

buyaa-n commented May 31, 2023

Failures unrelated and looks all known including #86919

@buyaa-n buyaa-n merged commit 5e67657 into main May 31, 2023
@buyaa-n buyaa-n deleted the fix-CA1861 branch May 31, 2023 21:01
@ghost ghost locked as resolved and limited conversation to collaborators Jul 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants