False positive with named arguments (AV1555)

[dennisdoomen]

Analyzer

Example: AV1555 : Parameter 'configureTelemetryConfiguration' in the call to 'ApplicationInsightsLoggingBuilderExtensions.AddApplicationInsights(ILoggingBuilder, Action, Action)' is invoked with a named argument

Repro steps

The following code triggers AV1555, even though it's invoking a Microsoft NuGet package

builder.Services.AddLogging(loggingBuilder =>
{
    if (!string.IsNullOrWhiteSpace(connectionString))
    {
        // Enable logging to the Application Insights service.
        loggingBuilder.AddApplicationInsights(
            configureTelemetryConfiguration: config => config.ConnectionString = connectionString,
            configureApplicationInsightsLoggerOptions: options => { }
        );
    }
});

Expected behavior

Should not complain about it

Actual behavior

Complains about it ;-)

[bkoelman]

Why do you consider this a false positive? As far as I'm aware, the analyzer matches the rule definition.

[dennisdoomen]

I was expecting this to only trigger for my own code. In this case, I'm using a method in a package I don't control. AV1555 states this as an exception.

[bkoelman]

Are you referring to:

Exception: The only exception where named arguments improve readability is when calling a method of some code base you don’t control that has a bool parameter

Because that applies to booleans only.

[dennisdoomen]

Because that applies to booleans only.

Crap. You're right (as usual). Well, I'm going to change the guidelines. I think it can really help when working with external libraries.

[bkoelman]

I'm looking forward to what you have in mind. Putting the lambda parameter names in code clarifies what the callback is supposed to accomplish, which is hard to guess from your example when not inside an IDE. So I see how this improves readability. It makes sense to allow that.

Does this apply specifically to parameters that return a Func or Task (so basically, that take a lambda)? I'm asking because I'm trying to imagine how the analyzer is supposed to work. It could detect "Microsoft" packages by scanning for the namespace and/or assembly name to be System.* or Microsoft.*. However, that would exclude third-party libraries where the same readability improvement is applicable. For example:

    builder.Services.AddDbContext<AppDbContext>(optionsAction: options => // allow
        options.UseNpgsql(npgsqlOptionsAction: npgsqlOptions => npgsqlOptions.EnableRetryOnFailure())); // warn

On the other hand, detecting "my own code" is a bit problematic as well. An analyzer can ask the compiler if source code is available for the method being called, but that only works when everything is built from source (so in a single solution). As soon as the team starts splitting things up (frontend/backend solutions, or build their own NuGets), it won't trigger the warning anymore because it now has become "external" code, as far as the analyzer is aware.

[bkoelman]

@dennisdoomen I'd like to close this issue, because the conversation has stalled and this isn't the best place to discuss. If you still want to pursue this, can you please open an issue at https://github.com/dennisdoomen/CSharpGuidelines, pointing to this conversation?