Add an analyzer to require optional parameters be specified for internal invocations

[evildour]

Analyzer package

Microsoft.CodeQuality.Analyzers

Analyzer

InternalOptionalParametersRequired

Repro steps

Consider the following code:

class Foo
{
    public void DoSomething(string value, Options options = null)
    {
        DoSomething(new[] { value });
    }

    public void DoSomething(IEnumerable<string> values, Options options = null)
    {
        if (options == null)
        {
            options = new Options();
        }

        // ...
    }
}

class Options
{
    // ...
}

Obviously, the first overload of DoSomething should have passed options to the 2nd overload when it called it. To catch this kind of issue, there should be an analyzer available which will warn when invocations to other members defined within the same assembly omit optional parameters.

Note: I would be willing to implement this.

[sharwell]

My general preference is to not allow parameters to be omitted when best practice is to include them. For example, I oppose allowing a CancellationToken to be omitted from an asynchronous method (through overloads or optional parameters) in all cases, all the time. For the example above, the solution is removing both = null values.

I would prefer to see one of the following more specific rules instead:

1. Allow this rule to be more specific to a particular type. For example, New rule proposal: Provide CancellationToken in calls DotNetAnalyzers/AsyncUsageAnalyzers#36 concerns the creation of a rule like the one proposed here, except limited to CancellationToken.
2. Restrict this rule to cases where an instance of the parameter type is available in the current scope. For example, New rule proposal: Propagate cancellation tokens DotNetAnalyzers/AsyncUsageAnalyzers#37 concerns the creation of a similar rule.

[evildour]

Thanks @sharwell. For public library APIs, I think keeping the parameters optional makes for the best user experience. And while I think a rule specific to CancellationTokens would be good, it wouldn't make sense to have individual rules for each kind of options/setting object, or any other parameter that should be passed along.

I really like your suggestion to restrict the rule to where something is available in the current scope. I've implemented this restriction a version of this rule I've added to the project I'm working on. The code can be found here. I've also added a further restriction that for string and primitive-type parameters, the warning should only be shown if the thing that's in scope has the same name (compared case-insensitively). The rule would be too noisy if an optional string was omitted, say, and the rule flags it because there is some other unrelated string in scope.

I would be willing to port that rule over to this repo and add an associated VB analyzer.

[Evangelink]

@evildour Sorry for the late ping, would you still be willing to port the rule here?

[mavasani]

I think we should write a very simple analyzer Avoid optional parameters, with an implementation very similar to existing rules CA1021: Avoid out parameters and CA1045: Do not pass types by reference, but the added rule should default to api_surface = private, internal, i.e. the rule discourages use of optional parameters for internal and private methods and the option means users can make a stronger enforcements on public API surface if they intend to.

[mavasani]

Also, let us avoid hardcoding any special types such as CancellationToken or such - we want to absolutely avoid opinionated suggestions by hand picking certain types. Instead, we should provide generic API design improvement suggestions in CA rules.

[mavasani]

A code fix (with FixAll) here would be super handy - it would just do a Find all references for the containing method/property symbol, and replace the call sites that use the implicit optional argument with use of an explicit default argument that was used in the original optional parameter definition.

class C
{
  internal void M(object o = null) // changed to 'M(object o)' by the code fix
  {
  }

  internal void M1()
  {
    M(); // changed to 'M(o: null);' by the code fix
    M(new object()); // not changed by code fix
  }
}