PrintMembers method in record is unused

[NN---]

Version Used:
VS 16.9

🔗 Also filed as AB#1305928

Steps to Reproduce:

using System;
using System.Text;

sealed record C {
   public string a;
   private bool PrintMembers(StringBuilder builder)
   {
       builder.Append("a = " + a);
       return true;
   }
}

    
class Q {
    static void Main()
    {
         C c = new C();
        Console.Write(c.ToString());
    }
}

https://sharplab.io/#v2:EYLgtghgzgLgpgJwD4AEBMBGAsAKBQBgAIUMA6AFTgA8YBuXXKOCAGzgBNCE4BjAewScAwoQDeuQpJQBmYhiIR6OSYQAOCAJYA3CPELA+fFoQAKmgHYwAsnDDBEUABQBlGBYDmAIQCuGlu0R9X39EAEoJSXFlFRVgYICEUgBBVVU4c3ZHACIIQgBeQizCAGpCCFClGJUUAHZCN284SskAX1w2nAjJXHRCAEUxLrkANmIAFkIrCA1zR3DoyKGqkR58wnM4AHdCITnmqp2+cygjOFIAdU14Rx4KPlcPOYqhjpagA==

Expected Behavior:
No warning about unused PrintMembers since it is used implicitly by record ToString.

Actual Behavior:
Unused method PrintMembers .

[jaredpar]

Agree that we should not be emitting the suggestion / diagnostic around an unused member here as the member is used in ToString.

[jcouv]

The diagnostic is IDE0051. Moved to Area-IDE

[CyrusNajmabadi]

This is a little challenging as we have no way to get insight for the compiler generated methods (Afaict). Is tehre way, for example, to get the IOp tree for tehse synthesized methods?

[jaredpar]

Agree. Not sure how the IDE can do it as currently spec'd out unless we expose this info via the API somehow. That could be done but I'm a bit hesistant. Not strong hesistant but thoughtful hesistant.

Makes me wonder though. Is there a strong reason we required these to be private? Is one option allowing these members to be internal.

[CyrusNajmabadi]

One thing that would be helpful (and i think has come up a few times in the past) would be a way to say: given this ISymbol, provide the IOp trees for it. We could then walk the implicit, but eixsting, methods in the type to see what htey actually used.

--

Or, we go whole hog and this analysis moves entirely to compiler, as compiler has the entire insight of what is or isn't used :)

[jaredpar]

Or, we go whole hog and this analysis moves entirely to compiler, as compiler has the entire insight of what is or isn't used :)

Woah woah now ... let's not get crazy here ;)

One thing that would be helpful (and i think has come up a few times in the past) would be a way to say: given this ISymbol, provide the IOp trees for it.

Any place I can read up on that discussion?

[CyrusNajmabadi]

Any place I can read up on that discussion?

I'll see if i can find. But it may have already been solved in one case. I think this was in teh context of analyzers and we added RegisterOperationBlockAction for this purpose. So in this case, it's likely a question of if these synthesized symbols show up in those callbacks. If so, IDE could likely make this work based on that.

[sharwell]

The analyzer registers a callback for generated code. The IOperation tree is expected to contain an accurate representation of the contents of the ToString() method, which the analyzer then uses to recognize the call to PrintMembers. This appears to be a compiler bug with the analyzer already behaving correctly with respect to the information provided by the compiler.

[CyrusNajmabadi]

This appears to be a compiler bug with the analyzer already behaving correctly with respect to the information provided by the compiler.

One thing that might be necessary woudl be a way for an analyzer to opt into hearing about iops for synthesized (or IsImplicit) members.

Taht, or we could treat this just as a straight up bug that these members are not included in iop. I'm curious what @mavasani thinks.

[mavasani]

One thing that might be necessary woudl be a way for an analyzer to opt into hearing about iops for synthesized (or IsImplicit) members.

@CyrusNajmabadi That already exists - the analyzer can invoke AnalysisContext.ConfigureGeneratedCodeAnalysis(flags) and pass in the correct flags to indicate if it wants to get callbacks into generated code and/or report diagnostics in generated code. The analyzer in question here does register for callbacks into generated code as well:

roslyn/src/Analyzers/Core/Analyzers/RemoveUnusedMembers/AbstractRemoveUnusedMembersDiagnosticAnalyzer.cs

Line 45 in dc24c3d

GeneratedCodeAnalysisFlags.Analyze) // We want to analyze references in generated code, but not report unused members in generated code.

[mavasani]

Do we know at what state of the compilation pipeline do we add this synthesized ToString method to the record type? As long as the method symbol gets added in the binding stage and SymbolDeclaredCompilationEvent is enqueued to the compilation event queue for it, it should automatically flow to the analyzer driver and we should get all the required analyzer callbacks.

[mavasani]

Ah, so I did a little more digging and figured there are 2 separate concepts for generated code:

1. User or tooling written generated code (marked with GeneratedCodeAttribute or autogenerated comment header in files): We use the GeneratedCodeAnalysisFlags passed into AnalysisContext.ConfigureGeneratedCodeAnalysis() method to determine whether or not to make callbacks into such code for the analyzers
2. Compiler synthesized code as part of compilation: These symbols are marked IsImplicitlyDeclared and are explicitly skipped from analyzer execution at multiple places:

   1. roslyn/src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs

      Line 1119 in dc24c3d

      if (diagsWritten && !methodSymbol.IsImplicitlyDeclared && _compilation.EventQueue != null)

   2. roslyn/src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalysisScope.cs

      Lines 113 to 124 in dc24c3d

      	public static bool ShouldSkipSymbolAnalysis(SymbolDeclaredCompilationEvent symbolEvent)
      	{
      	// Skip symbol actions for implicitly declared symbols and non-source symbols.
      	return symbolEvent.Symbol.IsImplicitlyDeclared || symbolEvent.DeclaringSyntaxReferences.All(s => s.SyntaxTree == null);
      	}
      	public static bool ShouldSkipDeclarationAnalysis(ISymbol symbol)
      	{
      	// Skip syntax actions for implicitly declared symbols, except for implicitly declared global namespace symbols.
      	return symbol.IsImplicitlyDeclared &&
      	!((symbol.Kind == SymbolKind.Namespace && ((INamespaceSymbol)symbol).IsGlobalNamespace));
      	}

I believe it was a design decision made in very early stages of analyzer design that compiler synthesized code is likely never useful for analyzer authors as (a) it would never have syntax or place to report diagnostics and (b) would be pretty much self contained. This no longer seems to be the case for records as synthesized ToString method is invoking into non-synthesized PrintMembers method.

[CyrusNajmabadi]

Records really do blur the lines here. Those members are really a public part of the type, with relevant code in it. It's just as if they were generated into a different partial part. That really separates them out from things that are mere implemetnation details and whose semantics on their own really don't affect surround code.

I don't have a good answer here, but i think we should pull in the right set of people to discuss this.

[jaredpar]

I don't have a good answer here, but i think we should pull in the right set of people to discuss this.

Agree. Think likely @333fred, @jcouv and @chsienki should be involved compiler side.

[chsienki]

Presumably we know if PrintMembers is Synthesized or not? Given that, could we say that ToString() is no longer considered synthesized when PrintMembers isn't?

Effectively, non-synthesized is viral: if synthesized code calls into something non-synthesized then it also becomes non synthesized.

We could still mark it with the generated attribute so most analyzers would ignore it, but ones that are equipped to handle it would then see it in that case, or ignore it still when everything is synthesized?

[mavasani]

Agree with your suggestion @chsienki.

[sharwell]

@mavasani Implicit symbols are skipped from symbol and declaration callbacks, but we should still be getting operation callbacks for executable code contained in them.

[sharwell]

Effectively, non-synthesized is viral: if synthesized code calls into something non-synthesized then it also becomes non synthesized.

This is why the containing type and method for top-level statements is considered non-synthesized.

[mavasani]

@mavasani Implicit symbols are skipped from symbol and declaration callbacks, but we should still be getting operation callbacks for executable code contained in them.

No, we skip them completely - there are no callbacks whatsoever for implicit symbols and nodes/operations within it. That has been the design since v1 of the analyzer driver.

[sharwell]

That makes sense for cases where two representations for the same code exist (e.g. async methods lower to a state machine that doesn't use async/await), but I'm not sure it makes sense for case that have no higher-level representation.

[Youssef1313]

Compiler synthesized code as part of compilation: These symbols are marked IsImplicitlyDeclared and are explicitly skipped from analyzer execution at multiple places:

@mavasani fwiw, I don't think this is accurate. IsImplicitlyDeclared can be false for compiler-synthesized code. For example, <Main>$ has IsImplicityDeclared = false.

[sharwell]

IsImplicitlyDeclared can be false for compiler-synthesized code

This was done specifically to enable analyzer callbacks in top-level statements. The change was made under the definition that IsImplicitlyDeclared=true means all code in this scope is implicitly declared.

[jaredpar]

This seems to have gotten a bit stale. Should we maybe schedule 15 minutes to chat about this to see how we want to proceed here?

[Youssef1313]

The analyzer registers a callback for generated code. The IOperation tree is expected to contain an accurate representation of the contents of the ToString() method, which the analyzer then uses to recognize the call to PrintMembers. This appears to be a compiler bug with the analyzer already behaving correctly with respect to the information provided by the compiler.

@sharwell Wouldn't this still be problematic for:

public sealed record R
{
    public override string ToString() => string.Empty;
}

Here, ToString is overridden. The compiler still emits PrintMembers and it is indeed unused. But a diagnostic is really not actionable. Likely we don't want a diagnostic here?