IOperation callbacks for compiler generated code

[mavasani]

Currently, we generate IOperation nodes even for compiler generated code. For example, compiler generates an ArrayCreation, which implements IArrayCreationExpression, for param array parameters for which no explicit argument was provided. Analyzers which have registered for an IArrayCreationExpression will be called back for this compiler generated. One of the analyzers in roslyn-analyzers repo: CA1825 (AvoidZeroLengthArrayAllocations) ends up complaining about zero length allocations for this compiler generated code: see here. Such diagnostics are not helpful for the end users, and may or may not be useful for the compiler authors.

Open questions:

1. Should we avoid analyzer callbacks for compiler generated code? We do explicitly avoid symbol callbacks for implicitly declared symbols, so I presume the answer is yes.
2. Should we expose a property IOperation.IsCompilerGenerated? The analyzer driver can skip calling back the analyzers for compiler generated nodes.
3. If answer to (2) is yes, then should this property be public?

[mavasani]

Tagging few folks who might have an opinion @JohnHamby @srivatsn @gafter @jaredpar @genlu

[mavasani]

Skipped test showing this issue: https://github.com/dotnet/roslyn-analyzers/pull/904/files#diff-70761c3b23525ba1ad10f9da24e4dc9eR224

[mavasani]

We will worked around this issue in the analyzer for now: dotnet/roslyn-analyzers#905

Once this issue is fixed, we can revert the workaround in the analyzer.

[genlu]

I'm on the fence about callback on compiler generated node. On one hand, I can see why people might not want to deal with it in their analyzer as it's not something explicitly expressed in the code being analyzed, but on the other hand, compiler generated or not, there's no different semantically.

For IOperation.IsCompilerGenerated, i think we should expose it as public, especially if we decided to keep things as is, i.e. invoking callbacks on compiler generated nodes.

[mavasani]

I feel default should not call back for compiler generated code. We can add a new field to GeneratedCodeAnalysisFlags to allow analyzers to explicitly request callback for compiler generated code.

[genlu]

GeneratedCodeAnalysisFlags sounds like a fine idea :) I assume it will apply to symbol analysis as well?

[mavasani]

Currently the driver doesn't callback the analyzers for implicitly declared symbols, but with the new flag, we should use that to turn on callbacks even for implicitly declared symbols.

[srivatsn]

Why do we need a new field to GenerateCodeAnalysisFlags though? Shouldn't the definition of generated code just expand to include these operations as well?

[mavasani]

That depends on the semantics we want to tie up with what classifies as "generated code". If we believe any analyzer that wants to analyze code generated by an external tool (e.g. .designer.generated.cs files) should also explicitly get callbacks for compiler generated code (the former actually having source inputs to the compiler, while the latter having no source input) - then the single flag will suffice.

I feel these are 2 completely distinct analysis scenarios and may deserve separate flags - analyzing some tool generated code passed into the compiler as source input (analyzers to find issues for generated code generators) versus analyzing compiler generated code (analyzers to find issues for compiler code generators)

[srivatsn]

To me, as an user, the reason I don't want to see issues in generated code is because I can't fix them directly (unless I had access to the generator somehow). From that perspective whether it was generated by a different tool or the compiler doesn't matter.

[mavasani]

The end user never sees or configures GenerateCodeAnalysisFlags though - it is only the analyzer author who uses it to configure the category of callbacks that are desired for its actions.

[mavasani]

I agree in the sense that whenever we add compiler support for the end user to be able to specify whether or not they intend to see generated code diagnostics, we should consider generated from tool or compiler to be same. However, I think they are different from the viewpoint of an analyzer author, as they generate callbacks for code from source files input to compiler versus IOperations that have no corresponding source.

[msJohnHamby]

Stateful analyzers require that actions run on generated code. For example, if an analyzer is trying to determine whether the accessibility of a property could be narrowed, failing to analyze generated code could result in recommending that a property could be made private when it actually needs to be, say, internal.

If we're going to have a switch related to analyzing generated code, how about having the switch control display of diagnostics rather than whether or not actions run?

[CyrusNajmabadi]

I disagree with the statement: "Such diagnostics are not helpful for the end users"

If i was writing an analyzer looking for unnecessary allocations, i would want to know about this so i could do something appropriate.

Now, that said, having information on the operation to know if it's compiler generated or not seems very useful.

[bkoelman]

As an analyzer author, I agree with @mavasani. Depending on what an analyzer does, some analyzers should and others should not get callbacks for generated operations. Having an API to control this (apart from whether analysis runs on generated-code files) would be helpful, as well as publicly exposing whether the operation is compiler-generated or exists in source.

For example, I'm working on an analyzer that counts the number of statements in a method body. Now I see it failing on lambda expressions. For some reason, the compiler translates the expression into a bound block, which is then counted as a statement. But the user never typed a statement, so the operation should not be included in the count.

[CyrusNajmabadi]

A variant of this topic came up while discussing things with @gafter and @mattwar . The issue, IMO, is that IOperations sit on top of the BoundNodes, and the compilers do some amount of BoundNode-lowering when interpreting code. IMO, this is a mistake. We should have an initial set of un-lowered BoundNodes that correspond much more closely (or identically) to the syntax that the user provided.

We could then optionally consider ways to expose lowering passes, and the corresponding IOperations available then. For example, some people might want to operate over the bound nodes after things like lambdas/async/iterators were rewritten.

Right now, we're in a strange hybrid state. Much of teh tree is lowered, but not all of it is. That makes the API difficult to consume as you're never quite sure what you're going to get.

As we proceed with IOperatoin moving forward, i would highly recommend as a first step that we actually move to un-lowering everything, and then consider future work where lowered passes get exposed as well.

[msJohnHamby]

As a starting point, certainly the less lowering involved, the better. The initial IOperation experiment--and unfortunately the state of things is still experimental--deliberately avoiding changing more than strictly necessary in the workings of the compilers.

[paul1956]

This issue impacts new VB Templates in Microsoft Visual Studio Community 2019 Preview
Version 16.8.0 Preview 2.1. VB analyzer template needs code cleanup (polish) Issue in dotnet
/roslyn-sdk#598