Prefer readonlyspan properties to array fields #5548

NewellClark · 2021-09-29T16:44:17Z

Fix dotnet/runtime#33780.
I've found and fixed all violations in dotnet/runtime (already merged) and dotnet/roslyn (PR).

I've gone ahead and implemented a fixer, however because of this issue the fixer will not show up in visual studio. The testing framework is still able to run the fixer, however, so I was able to test it.

Edit: I've implemented it as a SymbolStart/SymbolEnd analyzer as requested, so it will work normally.

…-array-fields

- Fix crash for fields with initializer like `new byte[123];`

src/Utilities/Compiler/WellKnownTypeNames.cs

buyaa-n

Thanks, @NewellClark, left some suggestions/questions overall looks it is quite close to get merged.

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx

src/NetAnalyzers/Microsoft.CodeAnalysis.NetAnalyzers.md

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx

buyaa-n · 2021-11-10T01:59:39Z

...e/Microsoft.NetCore.Analyzers/Runtime/PreferReadOnlySpanPropertiesOverReadOnlyArrayFields.cs

+using Microsoft.CodeAnalysis.Diagnostics;
+using Microsoft.CodeAnalysis.Operations;
+using Microsoft.CodeAnalysis.Text;
+using Resx = Microsoft.NetCore.Analyzers.MicrosoftNetCoreAnalyzersResources;


NIT: normally we do a static import

...e/Microsoft.NetCore.Analyzers/Runtime/PreferReadOnlySpanPropertiesOverReadOnlyArrayFields.cs

buyaa-n · 2021-11-12T00:43:27Z

...rosoft.NetCore.Analyzers/Runtime/PreferReadOnlySpanPropertiesOverReadOnlyArrayFieldsTests.cs

+            };
+            return test.RunAsync();
+        }
+


The overall test coverage looks great, only suggestions: please add a test with a comment

Youssef1313 · 2021-11-12T01:06:16Z

...NetCore.Analyzers/Runtime/CSharpPreferReadOnlySpanPropertiesOverReadOnlyArrayFields.Fixer.cs

+                        //  reference to the array field) and then remove the first argument from the argument list.
+                        if (invocationSyntax.ArgumentList.Arguments.Count == invocation.Arguments.Length)
+                        {
+                            var newArgumentList = invocationSyntax.ArgumentList.WithArguments(invocationSyntax.ArgumentList.Arguments.RemoveAt(0));


What if the arguments doesn't match the parameters order?

mavasani · 2021-11-23T02:59:51Z

I've gone ahead and implemented a fixer, however because of this issue the fixer will not show up in visual studio.

@NewellClark I believe this analyzer should be written to only operate on private fields and should be written as a SymbolStart/SymbolEnd analyzer, not CompilationEnd analyzer. Once you make that change, the code fix will start working in the IDE and be available to users.

mavasani · 2021-11-23T03:11:52Z

...e/Microsoft.NetCore.Analyzers/Runtime/PreferReadOnlySpanPropertiesOverReadOnlyArrayFields.cs

+                var field = (IFieldSymbol)context.Symbol;
+                if (field.IsStatic && field.IsReadOnly && field.Type is IArrayTypeSymbol arrayType && cache.IsSupportedArrayElementType(arrayType.ElementType))
+                {
+                    cache.AddCandidate(field);


Only private fields should be candidates.

mavasani

Needs to be written as a SymbolStart/SymbolEnd analyzer.

mavasani · 2022-01-28T07:58:08Z

Ping @NewellClark - are you able to do the rewrite as a SymbolStart/End analyzer?

stephentoub · 2023-01-04T20:15:49Z

@NewellClark, are you still working on this, or should it be closed?

buyaa-n · 2023-05-08T17:10:34Z

@NewellClark are you still planning to work on this?

NewellClark · 2023-05-08T22:17:33Z

The school year just ended for me, so I will be working on this ASAP. Sorry for the delay.

NewellClark · 2023-05-12T15:19:27Z

@mavasani I will convert the analyzer to use a symbol start/end analyzer.

- Set isReportedAtCompilationEnd to false

…roperties-to-array-fields

NewellClark · 2023-05-31T21:49:03Z

I've modified the analyzer to use SymbolStart/SymbolEnd as requested.

I'd like to take a moment to apologize for taking so long to get this done. When I became swamped with school work this past year, I should have indicated that I wouldn't be able to work on it so somebody else could have picked it up. For that, I apologize, and I will make sure it doesn't happen going forward.

I am interested in contributing to this repository in the future, and I will ensure to finish assignments I pick up quickly.

stephentoub · 2023-05-31T21:56:59Z

No need to apologize! We appreciate your efforts. And school absolutely takes priority.

NewellClark · 2023-05-31T22:15:20Z

Oh dear, looks like I forgot to do something. I remember there was some command I'm supposed to run on the command line before submitting, and I can't remember what it is and I'm unable to find it. All I remember is that when I forget to run it, all the CI tests fail.

Edit: I found it. I forgot to run MSBUILD.

After this goes through, I'm submitting an issue to add the msbuild /t:pack /v:m instruction to a salient place in contributor documentation, because it took me over an hour to find these instructions (I had to create a dummy branch and pull request to get them to show up in the pull-request comment box).

codecov · 2023-06-01T01:03:11Z

Codecov Report

Merging #5548 (1af039e) into main (2b6ab8d) will increase coverage by 0.00%.
The diff coverage is 96.51%.

Additional details and impacted files

@@           Coverage Diff            @@
##             main    #5548    +/-   ##
========================================
  Coverage   96.40%   96.40%            
========================================
  Files        1379     1383     +4     
  Lines      322253   322971   +718     
  Branches    10460    10494    +34     
========================================
+ Hits       310657   311356   +699     
- Misses       9103     9112     +9     
- Partials     2493     2503    +10

buyaa-n · 2023-06-29T00:25:29Z

@NewellClark found a false positive while testing in runtime repo, I believe it should no flag below case:

public class Tests
{
    private static readonly byte[] s_protocolMismatch13 = new byte[] { (byte)TlsContentType.Alert, 3, 4, 0, 2, 2, 70 };
    private static readonly byte[] s_protocolMismatch12 = new byte[] { (byte)TlsContentType.Alert, 3, 3, 0, 2, 2, 70 };
	
    private static byte[] CreateProtocolVersionAlert(SslProtocols version) =>
        version switch
        {
            SslProtocols.Tls13 => s_protocolMismatch13,
            SslProtocols.Tls12 => s_protocolMismatch12,
            _ => Array.Empty<byte>(),
        };
}

buyaa-n · 2023-06-29T00:43:21Z

...e/Microsoft.NetCore.Analyzers/Runtime/PreferReadOnlySpanPropertiesOverReadOnlyArrayFields.cs

+        private void OnSymbolStart(SymbolStartAnalysisContext context)
+        {
+            //  Bail if we're missing required symbols.
+            if (!RequiredSymbols.TryGetRequiredSymbols(context.Compilation, out RequiredSymbols? symbols))
+                return;
+


I think this part should go to the CompilationStartAction as it will not be different for each Symbol.

Suggested change

private void OnSymbolStart(SymbolStartAnalysisContext context)

{

// Bail if we're missing required symbols.

if (!RequiredSymbols.TryGetRequiredSymbols(context.Compilation, out RequiredSymbols? symbols))

return;

private void OnSymbolStart(SymbolStartAnalysisContext context, RequiredSymbols symbols)

{

i.e public override void Initialize(AnalysisContext context) would look like this:

public override void Initialize(AnalysisContext context) { context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); context.EnableConcurrentExecution(); context.RegisterCompilationStartAction(context => { // Bail if we're missing required symbols. if (!RequiredSymbols.TryGetRequiredSymbols(context.Compilation, out RequiredSymbols? symbols)) return; context.RegisterSymbolStartAction(context => OnSymbolStart(context, symbols), SymbolKind.NamedType); }); }

alrz · 2024-03-06T08:22:44Z

Was about to file an issue for this. Is there anything that is blocking this PR?

NewellClark added 11 commits September 19, 2021 15:39

Create files and basic tests

984fc0b

Analyzer working

9659c24

Refactor analyzer visitors

b4e54a4

Fix corner cases

97fd67c

Implement fixer

e5b4c7d

Refactor and add comments

caf4ba2

Handle multiple fields in one declaration

52ef641

Merge branch 'release/7.0.1xx' into prefer-readonlyspan-properties-to…

3042a69

…-array-fields

Fix false positive with jagged arrays

93364df

Fix null reference crash

46cca7b

- Fix crash for fields with initializer like `new byte[123];`

Merge branch 'main' into prefer-readonlyspan-properties-to-array-fields

d0ec5e1

NewellClark requested a review from a team as a code owner September 29, 2021 16:44

NewellClark added 2 commits September 29, 2021 13:37

Use Environment.NewLine in fixer

60039d1

Set isReportedAtCompilationEnd to true

955a595

Youssef1313 reviewed Oct 4, 2021

View reviewed changes

src/Utilities/Compiler/WellKnownTypeNames.cs Outdated Show resolved Hide resolved

NewellClark added 2 commits October 4, 2021 07:41

Remove unused well known type name SystemSByte

d8b92bb

Merge branch 'main' into prefer-readonlyspan-properties-to-array-fields

5ad4dd9

buyaa-n reviewed Nov 12, 2021

View reviewed changes

buyaa-n requested review from mavasani and jmarolf November 12, 2021 00:45

Youssef1313 reviewed Nov 12, 2021

View reviewed changes

mavasani reviewed Nov 23, 2021

View reviewed changes

mavasani suggested changes Nov 23, 2021

View reviewed changes

mavasani assigned mavasani and buyaa-n Jan 31, 2022

Youssef1313 mentioned this pull request Apr 9, 2022

Code style to convert byte arrays to UTF8 strings dotnet/roslyn#60647

Merged

buyaa-n mentioned this pull request Nov 16, 2022

.NET 8 developers can verify more APIs for correct usage to speed up their development dotnet/runtime#78442

Closed

NewellClark force-pushed the prefer-readonlyspan-properties-to-array-fields branch from 7818fd2 to 5ad4dd9 Compare May 12, 2023 18:17

NewellClark added 6 commits May 12, 2023 14:26

Merge branch 'main' into prefer-readonlyspan-properties-to-array-fields

e1e27ce

Reimplemented private classes

ca627f8

Add unit tests enforcing private-only arrays

b7db02c

.

118e852

Refactored Cache into CandidateCollection

29f58b8

- Set isReportedAtCompilationEnd to false

Merge remote-tracking branch 'dotnet/main' into prefer-readonlyspan-p…

4da31f1

…roperties-to-array-fields

NewellClark added 2 commits June 5, 2023 15:59

Apply suggested naming changes

2f0dbc3

Forgot to run msbuild

1af039e

buyaa-n requested a review from mavasani June 28, 2023 20:41

buyaa-n reviewed Jun 29, 2023

View reviewed changes

Get required symbols on compilation start

dc430ee

alrz mentioned this pull request Mar 8, 2024

Use ReadOnlySpan<T> properties instead of static array fields dotnet/roslyn#71291

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prefer readonlyspan properties to array fields #5548

Prefer readonlyspan properties to array fields #5548

NewellClark commented Sep 29, 2021 •

edited

Loading

buyaa-n left a comment

buyaa-n Nov 10, 2021

buyaa-n Nov 12, 2021

Youssef1313 Nov 12, 2021

mavasani commented Nov 23, 2021

mavasani Nov 23, 2021

mavasani left a comment

mavasani commented Jan 28, 2022

stephentoub commented Jan 4, 2023

buyaa-n commented May 8, 2023

NewellClark commented May 8, 2023

NewellClark commented May 12, 2023

NewellClark commented May 31, 2023

stephentoub commented May 31, 2023

NewellClark commented May 31, 2023 •

edited

Loading

codecov bot commented Jun 1, 2023 •

edited

Loading

buyaa-n commented Jun 29, 2023

buyaa-n Jun 29, 2023

alrz commented Mar 6, 2024

Prefer readonlyspan properties to array fields #5548

Are you sure you want to change the base?

Prefer readonlyspan properties to array fields #5548

Conversation

NewellClark commented Sep 29, 2021 • edited Loading

buyaa-n left a comment

Choose a reason for hiding this comment

buyaa-n Nov 10, 2021

Choose a reason for hiding this comment

buyaa-n Nov 12, 2021

Choose a reason for hiding this comment

Youssef1313 Nov 12, 2021

Choose a reason for hiding this comment

mavasani commented Nov 23, 2021

mavasani Nov 23, 2021

Choose a reason for hiding this comment

mavasani left a comment

Choose a reason for hiding this comment

mavasani commented Jan 28, 2022

stephentoub commented Jan 4, 2023

buyaa-n commented May 8, 2023

NewellClark commented May 8, 2023

NewellClark commented May 12, 2023

NewellClark commented May 31, 2023

stephentoub commented May 31, 2023

NewellClark commented May 31, 2023 • edited Loading

codecov bot commented Jun 1, 2023 • edited Loading

Codecov Report

buyaa-n commented Jun 29, 2023

buyaa-n Jun 29, 2023

Choose a reason for hiding this comment

alrz commented Mar 6, 2024

NewellClark commented Sep 29, 2021 •

edited

Loading

NewellClark commented May 31, 2023 •

edited

Loading

codecov bot commented Jun 1, 2023 •

edited

Loading