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

CSharpUseRangeOperatorDiagnosticAnalyzer should ignore calls to single-int Slice/Substring overloads #53295

Closed
naine opened this issue May 10, 2021 · 3 comments
Labels
Area-Analyzers untriaged Issues and PRs which have not yet been triaged by a lead

Comments

@naine
Copy link

naine commented May 10, 2021

Version Used: 3.9.0-6.21160.10 (Visual Studio 16.9.4)

There are a number of scenarios detected by this analyzer where preferring the range operator makes sense:

x.Slice(s, x.Length - 1 - e)    // Analyzer suggests: x[s..^e]
x.Slice(s, e - s)               // Analyzer suggests: x[s..e]

The analyzer seems to recognize that there are cases where using the range operator would be suboptimal, and does not raise diagnostics even when enabled:

x.Slice(s, l)   // Not flagged by analyzer, presumably to avoid unnecessary subtractions

Another case where using the range operator is suboptimal is when the slice goes to the end of the original sequence, and there is an overload accepting a single int:

x.Slice(n)      // Analyzer suggests: x[n..]

Using a range expression in the way suggested would be fine if the compiler would recognise that this can use the simpler overload, and therefore generate identical code, but this isn't the case (and would appear to not be permitted by the spec):

// static Span<byte> Foo(Span<byte> x, int n) => x.Slice(n);
ldarga.s x
ldarg.1
call instance valuetype [System.Runtime]System.Span`1<!0> valuetype [System.Runtime]System.Span`1<uint8>::Slice(int32)
ret
// static Span<byte> Foo(Span<byte> x, int n) => x[n..];
ldarg.0
stloc.0
ldloca.s 0
call instance int32 valuetype [System.Runtime]System.Span`1<uint8>::get_Length()
ldarg.1
stloc.1
ldloc.1
sub
stloc.2
ldloca.s 0
ldloc.1
ldloc.2
call instance valuetype [System.Runtime]System.Span`1<!0> valuetype [System.Runtime]System.Span`1<uint8>::Slice(int32, int32)
ret
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Analyzers untriaged Issues and PRs which have not yet been triaged by a lead labels May 10, 2021
@CyrusNajmabadi
Copy link
Member

This is by design. The survey of the ecosystem indicated the semantics should be identical in an these cases. So this is an acceptable idiom to move the code to.

Now, the question is this idiom is optimized as much as it should be is another issue altogether. But anyone writing x[n..] should expect that that should be reasonable and properly optimized by the stack.

@naine
Copy link
Author

naine commented May 11, 2021

Now, the question is this idiom is optimized as much as it should be is another issue altogether.

In my benchmarks on .NET 5 the range idiom is about 40% slower than calling Slice(int).

I agree with your other points though so I'll close this issue.

@naine naine closed this as completed May 11, 2021
@naine
Copy link
Author

naine commented May 11, 2021

Also duplicate of #47629

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Analyzers untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

No branches or pull requests

2 participants