S3241 FP: Extension methods following fluent pattern should not be flagged

[SonnyRR]

Description

Rule S3241 ("Change return type to 'void'; not a single caller uses the returned value") produces a false positive for private extension methods that return their this parameter type, following the standard Fluent/convention API builder pattern.

Reproducer

public static class ServiceCollectionExtensions
{
    private static IServiceCollection AddFoo(this IServiceCollection services)
    {
        return services;
    }
}

Expected

No warning- the method follows the well-known fluent builder pattern (returning the this type).

Actual

warning S3241: Change return type to 'void'; not a single caller uses the returned value.

Analysis

The rule should suppress this diagnostic when:

1. The method is an extension method (has this parameter), AND
2. The return type matches the this parameter type

These methods follow the same convention as all ASP.NET Core IServiceCollection / IApplicationBuilder extensions. Changing to void would break the fluent chaining pattern and prevent callers from composing calls inline. The fact that no current callers chain off the return value is by design, the return type exists for consistency, future use, and to follow the ecosystem convention.

Product and Version

10.27.0.140913

[martin-strecker-sonarsource]

While I understand the desire to handle unused warnings for mutable fluent builders (like IServiceCollection extensions), I still question the core premise of this request regarding private methods.

I assume you use AddFoo this way within a larger, public configuration method:

public static IServiceCollection AddMyMassiveFeature(this IServiceCollection services) 
{
    services.AddFoo();
    services.AddOtherStuff();
    return services;
}

// S3241 flags this because the return value is ignored
private static IServiceCollection AddFoo(this IServiceCollection services)
{
    services.AddSingleton<IFoo, Foo>();
    return services; 
}

If this is the case, S3241 is doing exactly what it was designed to do: pointing out that the return type is dead code. There are two simple refactorings to resolve this warning without needing an analyzer exception:

Refactoring 1: Actually use the fluent chain
If you want the helper to remain fluent, chain it. Once the return value is used by the caller, the warning instantly disappears.

public static IServiceCollection AddMyMassiveFeature(this IServiceCollection services) 
{
    return services
        .AddFoo()
        .AddOtherStuff(); 
}

Refactoring 2: Change the helper to void
If you prefer the block-style layout and don't care about chaining, just change the private helper to void. Since it is a private method, you aren't breaking any public API contracts by changing the signature.

private static void AddFoo(this IServiceCollection services) 
{
    services.AddSingleton<IFoo, Foo>();
}

The Implementation Flaw

Even if we assume there is a valid use case for suppressing this, regarding the proposed fix in #9814: checking if the return type matches the this parameter type is not strict enough. This approach conflates Mutable Builders (which return their own mutated instance) with Immutable Transformers (which return a new instance of the same type).

If the analyzer only checks types, it will incorrectly classify standard pure functions as fluent. For example:

// Returns a completely new string. Ignoring this return value is a bug.
public static string AppendFoo(this string s) => s + "foo";

// Returns a new record instance. Ignoring this return value is a bug.
public static MyRecord WithName(this MyRecord r, string name) => r with { Name = name };

If warnings are suppressed based purely on the type signature, the analyzer risks hiding genuine bugs where developers forget to assign the result of an immutable transformation.

A More Robust Approach
The analyzer should verify the method body to ensure the returned symbol is actually the this parameter symbol. Here is a brief conceptual sketch of a symbol-based check using Roslyn for block bodies:

bool IsFluentBuilderMethod(IMethodSymbol methodSymbol, MethodDeclarationSyntax methodSyntax, SemanticModel semanticModel)
{
    // Ensure it is actually an extension method with the "this" modifier
    if (!methodSymbol.IsExtensionMethod || methodSymbol.Parameters.Length == 0 || methodSymbol.ReturnsVoid)
        return false;

    var thisParameter = methodSymbol.Parameters[0];
    
    // Find all standard return statements in the method body
    var returnStatements = methodSyntax.Body?.DescendantNodes().OfType<ReturnStatementSyntax>();
    
    if (returnStatements == null || !returnStatements.Any())
        return false; // Note: Production code would also need to handle ArrowExpressionClauseSyntax (=>)

    foreach (var returnStmt in returnStatements)
    {
        if (returnStmt.Expression == null) return false;
        
        var returnedSymbol = semanticModel.GetSymbolInfo(returnStmt.Expression).Symbol;
        if (!SymbolEqualityComparer.Default.Equals(returnedSymbol, thisParameter))
        {
            return false; // Returns something other than the 'this' parameter instance
        }
    }
    return true;
}

This strict symbol check safely isolates true fluent builders without causing false negatives for immutable types.

Summary for moving forward:

• Provide an alternative use-case justification: The helper method scenario discussed above is cleanly resolved by simple refactorings. Is there another specific use case in your codebase where a private fluent builder is strictly necessary and cannot be refactored in this way?
• Strengthen the implementation: Ensure the analyzer strictly checks for the returned this parameter symbol, rather than just matching return types.
• Cover instance methods: Extend the implementation strategy to handle standard instance-based fluent builders (return this;) as well, to ensure the rule is fully consistent.

I appreciate the effort you've put into the initial implementation, but a robust solution will require significantly more complexity than the current draft. Let's align on the use case first. Once we agree on the direction and give the green light, let me know if you'd still like to continue driving that PR.

[SonnyRR]

Hi @martin-strecker-sonarsource, thanks for the detailed reply! I agree with most of what you said, but unless I'm misunderstanding something I still think there current behavior is incorrect. The proposed PR changes were quickly drafted via the help of Claude Code, since I was unfamiliar with the codebase & cannot afford at the moment to dive deep.

Take this scenario for example:

    public static IServiceCollection AddCoreServices(this IServiceCollection services, IConfiguration configuration)
    {
        ArgumentNullException.ThrowIfNull(services);
        ArgumentNullException.ThrowIfNull(configuration);

        // All of the chained ext. methods below are custom private ones, implemented in the same ServiceCollection document as this method.
        // 'AddCoreConfigurationOptions(IConfiguration c)' & 'AddQuartzServices()'
        // don't trigger the S3241 warning - the last custom method invocation does.
        services
            .AddCoreConfigurationOptions(configuration)
            .AddQuartzServices()
            .AddTriangulationPipeline(); // ⚠️ - S3241

        return services;
    }

    private static IServiceCollection AddTriangulationPipeline(this IServiceCollection services) // ⚠️ - S3241
    {
        ArgumentNullException.ThrowIfNull(services);

        services
            .AddScoped<T1, T2>(...)

        return services;
    }

    private static IServiceCollection AddQuartzServices(this IServiceCollection services)
    {
        ArgumentNullException.ThrowIfNull(services);

        services
            .AddScoped<T1, T2>(...)

        return services;
    }

Now, I can register these services in any order that I want, meaning that the same warning can be as easily raised for AddQuartzServices() if it was the last method invoked in the chain. Why would that be considered as a correct case to raise the S3241 warning? The services variable is immediately returned afterwards.

Why would the order of the invocation of these extension methods be a reason for change of the return type on the last method, that is being invoked in the chain, only to prevent this warning from raising? If I want to extend the chain with new extension methods, to configure additional services, I'll have to once again change the return type of that method from void to IServiceCollection. The other option will be to always call this method last in the fluent chain, which is an odd thing to be forced to do. It will be the only method that is outlier with that return type in this chain only not to raise the warning.