dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
19.07k stars 4.04k forks source link

[New API] stackalloc in IOperation #66833

Open Vannevelj opened 1 year ago

Vannevelj commented 1 year ago

Background and Motivation

I have a collection of Roslyn analyzers that were previously using the syntax and semantic level to register actions. After some guidance from a Roslyn team member, I've nearly finished the process of changing them all to IOperation instead. One of my holdouts is UnboundedStackallocAnalyzer which still registers on SyntaxKind.StackAllocArrayCreationExpression.

I would like to convert this to an IOperation-based analyzer instead so I can improve uniformity in my codebase, as well as making use of the much more ergonomic APIs exposed through IOperation compared to SyntaxKind.

Proposed API

namespace Microsoft.CodeAnalysis.Operations
{
     public interface IArrayCreationOperation
     {
+        public bool IsStackAlloc { get; }
     }

Usage Examples

See https://github.com/Vannevelj/SharpSource/blob/648fc377db5747e1517f162884b095d07cebdf2e/SharpSource/SharpSource/Diagnostics/UnboundedStackallocAnalyzer.cs#L30 for the original code that would be converted into IOperation.

Alternative Designs

IStackAllocOperation

We can create a new operation kind:

namespace Microsoft.CodeAnalysis.Operations
{
+    public interface IStackAllocOperation
+    {
+        public IOperation Operation { get; }
+    }

I decided against this because I imagine in most cases we'll want to treat stackalloc arrays the same as regular ones, for the purpose of static analysis. After all, if we wanted to treat them differently then we'd have had to jump through hoops until now and a feature request would've come about earlier. By introducing a property on IArrayCreationOperation we avoid fragmentation.

Stackalloc vs StackAlloc

The keyword is stackalloc but the syntax node is StackAllocArrayCreationExpression so I figured StackAlloc is the capitalization to use. Alternative would be IsStackalloc or IStackallocOperation.

Risks

It is my understanding that stackalloc can only be used in combination with arrays today. If that is incorrect or if that is likely to change in the future, an IStackAllocOperation might be more prudent after all. However if we don't expect its use cases to expand significantly, then we can always add an IsStackAlloc property onto those operations in the future.

jasonmalinowski commented 1 year ago

@333fred Where do you want to route this? Not sure which area path this should live in.

Youssef1313 commented 1 year ago

See also https://github.com/dotnet/roslyn/issues/20123

333fred commented 1 year ago

I'll let @AlekseyTs chime in as well, but I don't believe that we should be reusing IArrayCreationOperation here. I don't really buy the argument that stackalloc will want to be treated like array creation most of the time. stackalloc has a lot of additional restrictions and semantics that array creations don't have, and imo most analyses and refactorings do not apply to both.

AlekseyTs commented 1 year ago

I am also leaning towards special IOperation node for stackalloc expressions

B1Z0N commented 2 months ago

Hello. +1 request here, and willing to implement.

@mavasani can't we move it to approved yet?

333fred commented 2 months ago

@B1Z0N the original poster didn't update their post to address our feedback to have a dedicated node be the regular proposal. If you want to open a new proposal addressing this feedback, we can move forward.

333fred commented 2 months ago

It will also need to be more complete, and show how to handle initializers and dimensions. I get that that's why the original request reused ArrayCreation, but they're not really related.