Detect when a range indexer is used on strings or arrays, then immediately implicitly converted to a Span or Memory type. by bartonjs · Pull Request #3464 · dotnet/roslyn-analyzers

bartonjs · 2020-04-01T21:39:06Z

Detects

CA1381: ReadOnlySpan<char> slice = str[a..b];
CA1382: ReadOnlySpan<SomeT> slice = arr[a..b];
CA1382: ReadOnlyMemory<SomeT> slice = arr[a..b];
CA1383: Span<SomeT> slice = arr[a..b];
CA1383: Memory<SomeT> slice = arr[a..b];

as well as using the expression as an argument to an appropriate parameter type. It would also detect str[a..b] to ReadOnlyMemory<char>, but there doesn't seem to be an implicit conversion there.

Does not detect

Explicit conversions:

ReadOnlySpan<char> slice = (ReadOnlySpan<char>)str[a..b];
ReadOnlySpan<SomeT> slice = (ReadOnlySpan<SomeT>)arr[a..b];
ReadOnlySpan<SomeT> slice = (Span<SomeT>)arr[a..b];
ReadOnlyMemory<SomeT> slice = (ReadOnlyMemory<SomeT>)arr[a..b];
ReadOnlyMemory<SomeT> slice = (Memory<SomeT>)arr[a..b];
Span<SomeT> slice = (Span<SomeT>)arr[a..b];
Memory<SomeT> slice = (Memory<SomeT>)arr[a..b];

Fixes

CA1381: ReadOnlySpan<char> slice = str.AsSpan()[a..b];
CA1382: ReadOnlySpan<SomeT> slice = arr.AsSpan()[a..b];
CA1382: ReadOnlyMemory<SomeT> slice = arr.AsMemory()[a..b];
CA1383: Span<SomeT> slice = arr.AsSpan()[a..b];
CA1383: Memory<SomeT> slice = arr.AsMemory()[a..b];

Fixes dotnet/runtime#33829

Because the string type is immutable, ReadOnlySpan<char> from a string is also immutable, therefore that diagnostic has a very low chance of being a false positive.

For arrays it's possible (but unlikely) that the caller wants the copy to avoid side effects, such as GC lifetime or P/Invokes disagreeing on how ReadOnly the Span was. Because of that, the array to ReadOnly{Span|Memory} and writable {Span|Memory} are different codes from the string one; and since the read-only version feels like it's almost never wrong, it's yet a third code.

For each different kind of error, the "Local" variant uses the long form, and verifies the message arguments.

bartonjs · 2020-04-01T21:45:29Z

Right now the tests don't pass on my machine, because it says the fixed code doesn't compile. If not for that, the change would be ready... so I'm opening the PR to see if it's me, or the repository state.

The fixer could certainly be smarter, and write prettier code.. e.g. str[3..5] => str.AsSpan(3, 2); but I couldn't figure out how to constant fold and it seemed like a nice to have on top, vs the essence of the feature.

bartonjs · 2020-04-01T21:55:56Z

cc @dotnet/dotnet-analyzers

bartonjs · 2020-04-01T22:41:40Z

OK, looks like it's because the tests are using the 3.0.0 version of the compiler package (

roslyn-analyzers/src/Test.Utilities/Test.Utilities.csproj

Line 9 in d02078f

    
           <PackageReference Include="Microsoft.CodeAnalysis" Version="$(MicrosoftCodeAnalysisVersion)" />

). Upgrading it to 3.5.0 (the latest version with no hyphens in it) is now not reporting all of the diagnostics I expect... so now I'm looking into what's up with that.

Thanks for the version pointer, @sharwell.

mavasani · 2020-04-01T22:46:15Z

@bartonjs The core analyzer packages are currently built against Microsoft.CodeAnalysis version 3.0.0 as they do not use any Roslyn APIs from a later version. This enables folks to install the analyzer packages on older compiler/VS. It should be fine to upgrade the test layer to newer Microsoft.CodeAnalysis version, we do this as and when we encounter a scenario such as this one.

mavasani · 2020-04-01T22:50:39Z

@jcouv did something similar for his changes that required the tests to run against latest Microsoft.CodeAnalysis:

roslyn-analyzers/eng/Versions.props

Line 24 in d02078f

    
           <MicrosoftCodeAnalysisForShippedApisVersion>3.5.0-beta2-20056-01</MicrosoftCodeAnalysisForShippedApisVersion>

We should probably rename that property to MicrosoftCodeAnalysisForTestsVersion or some such, change it to 3.5.0 and move the test project to that version.

stephentoub · 2020-04-01T23:12:02Z

Why so many diagnostic IDs? Why not just have one? If the goal is just to provide different messages, there are other ways. Similarly, the single rule could be configured for different types.

bartonjs · 2020-04-02T00:00:15Z

Why so many diagnostic IDs?

That was in my intro. I believe the string version is "always right" so I have it on as a warning. Array->ReadOnly as "almost always right" and Array->Writable as "probably right". Since they had different false positive rates i went with different IDs. if there're ways to control nuance for the same ID then I'm happy with that model.. I just don't know what it looks like in code or UX.

mavasani · 2020-04-02T00:13:24Z

Since they had different false positive rates i went with different IDs. if there're ways to control nuance for the same ID then I'm happy with that model.. I just don't know what it looks like in code or UX.

If you want different default severities and/or enabled by default values, using different IDs is the simplest possible approach. We can use options, but that is primarily used if user wants to control some configuration and is not always discoverable.

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Performance/UseAsSpanInsteadOfRangeIndexer.cs

...ers/UnitTests/Microsoft.NetCore.Analyzers/Performance/UseAsSpanInsteadOfRangeIndexerTests.cs

...alyzers/Core/Microsoft.NetCore.Analyzers/Performance/UseAsSpanInsteadOfRangeIndexer.Fixer.cs

mavasani · 2020-04-02T15:14:32Z

...CSharp/Microsoft.NetCore.Analyzers/Performance/CSharpUseAsSpanInsteadOfRangeIndexer.Fixer.cs

+    [ExportCodeFixProvider(LanguageNames.CSharp), Shared]
+    public sealed class CSharpUseAsSpanInsteadOfRangeIndexerFixer : UseAsSpanInsteadOfRangeIndexerFixer
+    {
+        protected override bool TrySplitExpression(


I was going to suggest you to use language-agnostic IOperation based approach here, but it seems that input[3..5] actually generates an OperationKind.None node: https://github.com/dotnet/roslyn/blob/master/src/Compilers/CSharp/Test/IOperation/IOperation/IOperationTests_IFromEndIndexOperation_IRangeOperation.cs#L52

@333fred Do we have a work item tracking the IOperation implementation for this case?

@agocke did we not actually implement Range/Index in IOperation? I don't think we blocked the master merge on that, but iirc it was expected that IOperation/CFG would be implemented shortly thereafter. dotnet/roslyn#31545

I seem to remember a substantial amount was implemented, but we may have left it as OperationKind.None to prevent considering it "final." I don't remember what still needs to be implemented, if anything.

I've renamed that issue to clarify that we need to confirm the implemention, not just add tests

...ers/UnitTests/Microsoft.NetCore.Analyzers/Performance/UseAsSpanInsteadOfRangeIndexerTests.cs

…ormed to markup

mavasani · 2020-04-02T17:15:38Z

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Performance/UseAsSpanInsteadOfRangeIndexer.cs

+                            return;
+                        }
+
+                        if (operationContext.Operation.Syntax.RawKind != (int)Microsoft.CodeAnalysis.CSharp.SyntaxKind.ElementAccessExpression)


We should not use Microsoft.CodeAnalysis.CSharp.SyntaxKind in shared layer. This will cause C# assemblies to be loaded from VB only solution. If you need to access C# specific types, you should change this type to abstract and have the C# sub-type have C# specific logic.

It shouldn't cause an assembly load, since it's an enum (const values are burned in to the caller). If we're concerned about the ease of creep I could change it to a literal and a comment; which feels better to me than over-engineering a hierarchy here.

mavasani

Marking as request changes so we accidentally do not access language-specific types in language agnostic layer, which will be an assembly load problem. Overall, the PR looks good to me.

eng/Versions.props

…mi-neutral code

…cases

bartonjs · 2020-04-06T17:25:23Z

It looks like there's a runtime breaking change in the constructors for Microsoft.CodeAnalysis.CSharp.CSharpCompilationOptions between whatever Test.Utilities is building with and 3.5.0, so changing just the one dependency is failing:

  Error Message:
   System.TypeInitializationException : The type initializer for 'Test.Utilities
.CodeMetrics.CodeMetricsTestBase' threw an exception.
---- System.MissingMethodException : Method not found: '
Void Microsoft.CodeAnalysis.CSharp.CSharpCompilationOptions..ctor(
 Microsoft.CodeAnalysis.OutputKind,
 Boolean,
 System.String,
 System.String,
 System.String,
 System.Collections.Generic.IEnumerable`1<System.String>,
 Microsoft.CodeAnalysis.OptimizationLevel,
 Boolean,
 Boolean,
 System.String,
 System.String,
 System.Collections.Immutable.ImmutableArray`1<Byte>,
 System.Nullable`1<Boolean>,
 Microsoft.CodeAnalysis.Platform,
 Microsoft.CodeAnalysis.ReportDiagnostic,
 Int32,
 System.Collections.Generic.IEnumerable`1<
  System.Collections.Generic.KeyValuePair`2<
    System.String,
    Microsoft.CodeAnalysis.ReportDiagnostic>>,
 Boolean,
 Boolean,
 Microsoft.CodeAnalysis.XmlReferenceResolver,
 Microsoft.CodeAnalysis.SourceReferenceResolver,
 Microsoft.CodeAnalysis.MetadataReferenceResolver,
 Microsoft.CodeAnalysis.AssemblyIdentityComparer,
 Microsoft.CodeAnalysis.StrongNameProvider,
 Boolean,
 Microsoft.CodeAnalysis.MetadataImportOptions,
 Microsoft.CodeAnalysis.CSharp.NullableContextOptions)'.
  Stack Trace:
     at Test.Utilities.CodeMetrics.CodeMetricsTestBase..ctor()
   at Microsoft.CodeAnalysis.CodeMetrics.UnitTests.CodeMetricsComputationTests..
ctor()
----- Inner Stack Trace -----
   at Test.Utilities.CodeMetrics.CodeMetricsTestBase..cctor()

... it looks like the namespace for NullableContextOptions changed (there's no CSharp in it anymore). Trying to figure out how to move forward from here.

bartonjs · 2020-04-06T18:04:53Z

If at first you don't succeed... add reflection? Testing the workaround now.

mavasani · 2020-04-06T18:20:55Z

@bartonjs - great find! Please do file a bug on Roslyn repo for the breaking change in the constructor, which should get this fixed asap in Roslyn. Thanks!

…ackages

bartonjs · 2020-04-06T18:32:44Z

It's already known / marked as "can't fix without introducing yet another breaking change".

dotnet/roslyn#39524

codecov · 2020-04-06T18:55:21Z

Codecov Report

Merging #3464 into master will increase coverage by 0.01%.
The diff coverage is 97.32%.

@@            Coverage Diff             @@
##           master    #3464      +/-   ##
==========================================
+ Coverage   95.30%   95.31%   +0.01%     
==========================================
  Files        1027     1033       +6     
  Lines      233052   234250    +1198     
  Branches    15068    15141      +73     
==========================================
+ Hits       222111   223282    +1171     
- Misses       9278     9294      +16     
- Partials     1663     1674      +11

mavasani · 2020-04-06T19:09:24Z

src/Test.Utilities/CodeMetricsTestsBase.cs

    public abstract class CodeMetricsTestBase
    {
-        private static readonly CompilationOptions s_CSharpDefaultOptions = new Microsoft.CodeAnalysis.CSharp.CSharpCompilationOptions(OutputKind.DynamicallyLinkedLibrary);
+        private static readonly CompilationOptions s_CSharpDefaultOptions = BuildDefaultCSharpOptions();


It will also be fine to move Test.Utilities.csproj to a newer Microsoft.CodeAnalysis reference.

bartonjs added 7 commits March 20, 2020 16:24

First cut at detecting range indexer implicit conversion to span/memory

d2587b8

Add missing doc comment

d196d8a

Start adding a fixer

9100702

Merge remote-tracking branch 'dotnet/master' into range_indexer_to_span

dfe1146

React to merge errors

a596ae6

Generally change to markup syntax in tests

a1af944

For each different kind of error, the "Local" variant uses the long form, and verifies the message arguments.

Add more fixer tests

6f624e1

bartonjs requested a review from a team as a code owner April 1, 2020 21:39

mavasani reviewed Apr 2, 2020

View reviewed changes

bartonjs added 3 commits April 2, 2020 09:59

Add comments to the argument-verifying tests so they don't get transf…

8c21de0

…ormed to markup

Support detecting range indexers via OperationKind.None

1185297

Bump the tests to use the latest stable Roslyn (3.5.0)

87b763c

mavasani reviewed Apr 2, 2020

View reviewed changes

mavasani suggested changes Apr 2, 2020

View reviewed changes

mavasani reviewed Apr 2, 2020

View reviewed changes

eng/Versions.props Outdated Show resolved Hide resolved

bartonjs added 5 commits April 2, 2020 15:51

Upgrade roslyn for NetAnalyzers.UnitTests only

4dbda13

Move type names to WellKnownTypeNames

9180658

Remove compile-time reference to CodeAnalysis.CSharp from language se…

6c0d447

…mi-neutral code

Better tolerance of surprising states

b25d805

Only call RegisterCodeFix after it's known to succeed.

93cc17e

mavasani approved these changes Apr 2, 2020

View reviewed changes

Add fixer validation to all cases, use WithSpan in message-verifying …

048b6bb

…cases

Merge remote-tracking branch 'dotnet/master' into range_indexer_to_span

af761d8

Work around binary breaking change between 3.0.0 and 3.5.0 compiler p…

4beceb6

…ackages

mavasani reviewed Apr 6, 2020

View reviewed changes

bartonjs merged commit 6b16c9c into dotnet:master Apr 8, 2020

stephentoub mentioned this pull request Apr 17, 2020

Misleading return types for range indexing: sub-array vs span dotnet/docs#17912

Closed

bartonjs deleted the range_indexer_to_span branch April 28, 2020 16:14

Conversation

bartonjs commented Apr 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Detects

Does not detect

Fixes

Uh oh!

bartonjs commented Apr 1, 2020

Uh oh!

bartonjs commented Apr 1, 2020

Uh oh!

bartonjs commented Apr 1, 2020

Uh oh!

mavasani commented Apr 1, 2020

Uh oh!

mavasani commented Apr 1, 2020

Uh oh!

stephentoub commented Apr 1, 2020

Uh oh!

bartonjs commented Apr 2, 2020

Uh oh!

mavasani commented Apr 2, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mavasani Apr 2, 2020

Choose a reason for hiding this comment

Uh oh!

333fred Apr 3, 2020

Choose a reason for hiding this comment

Uh oh!

agocke Apr 6, 2020

Choose a reason for hiding this comment

Uh oh!

agocke Apr 6, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mavasani Apr 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bartonjs Apr 2, 2020

Choose a reason for hiding this comment

Uh oh!

mavasani left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bartonjs commented Apr 6, 2020

Uh oh!

bartonjs commented Apr 6, 2020

Uh oh!

mavasani commented Apr 6, 2020

Uh oh!

bartonjs commented Apr 6, 2020

Uh oh!

codecov bot commented Apr 6, 2020

Codecov Report

Uh oh!

mavasani Apr 6, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bartonjs commented Apr 1, 2020 •

edited

Loading

mavasani Apr 2, 2020 •

edited

Loading