Analyzer CA1802 configuration option require_modifiers behavior does not match documentation, and defaults do not match.

[jtmedley]

Diagnostic ID: CA1802: Use Literals Where Appropriate

I can't make heads or tails of what the required_modifiers analyzer option is actually supposed to be doing. It's documented twice with different possible values and different defaults. The actual implementation doesn't do what the documentation says it does. The testing for it doesn't test any default option value, and it doesn't test all of the option values in either of the conflicting documentation pages.

Here's what is actually implemented:

Each analyzed field's modifiers contain all of those specified in the required_modifiers option value list here:

roslyn-analyzers/src/Utilities/Compiler/Options/AnalyzerOptionsExtensions.cs

Lines 607 to 616 in f4c8475

	public static bool MatchesConfiguredModifiers(
	this AnalyzerOptions options,
	DiagnosticDescriptor rule,
	ISymbol symbol,
	Compilation compilation,
	SymbolModifiers defaultRequiredModifiers = SymbolModifiers.None)
	{
	var requiredModifiers = options.GetRequiredModifiersOption(rule, symbol, compilation, defaultRequiredModifiers);
	return symbol.GetSymbolModifiers().Contains(requiredModifiers);
	}

Note the default required_modifiers value of defaultRequiredModifiers = SymbolModifiers.None.

The analyzer calls that as one of a long series of checks, and passing any of them causes a diagnostic to not be raised:

roslyn-analyzers/src/NetAnalyzers/Core/Microsoft.CodeQuality.Analyzers/QualityGuidelines/UseLiteralsWhereAppropriate.cs

Lines 69 to 81 in f4c8475

	var lastField = fieldInitializer?.InitializedFields.LastOrDefault();
	var fieldInitializerValue = fieldInitializer?.Value;
	if (fieldInitializerValue == null ||
	lastField == null ||
	lastField.IsConst ||
	!lastField.IsReadOnly ||
	(lastField.Type?.Name == lastField.Name && fieldInitializerValue.DescendantsAndSelf().Any(d => d is IFieldReferenceOperation field && lastField.Type.Equals(field.Field.Type, SymbolEqualityComparer.Default))) ||
	!fieldInitializerValue.ConstantValue.HasValue ||
	!context.Options.MatchesConfiguredVisibility(DefaultRule, lastField, context.Compilation, defaultRequiredVisibility: SymbolVisibilityGroup.Internal | SymbolVisibilityGroup.Private) ||
	!context.Options.MatchesConfiguredModifiers(DefaultRule, lastField, context.Compilation, defaultRequiredModifiers: SymbolModifiers.Static))
	{
	return;
	}

Note the default required_modifiers value of defaultRequiredModifiers: SymbolModifiers.Static, which overrides the default value for the default value.

So a diagnostic is not raised, therefore a field is ignored, if the field initializer does not contain all of the modifiers in required_modifiers. Or if required_modifiers is not specified, the field is ignored if it does not have the static modifier. Or, regardless of any modifiers in required_modifiers, the field is ignored if it does not have the readonly modifier. Or it it does have the const modifier. Or four other conditionals in that block.

Already, we see that const and readonly are also in the list of possible modifiers that could be parsed from required_modifiers via the SymbolModifiers enum. So regardless of intent, if const is in the list of required modifiers, the analyzer ignores a field if its modifiers have const or do not have const, which is always true, so the rule is always ignored. This makes sense given that the fixer for this analyzer makes the field const.

Conversely, the analyzer will ignore a field that does not have the readonly modifier in all cases, whether readonly is in the list of required modifiers or not. The required_modifiers unit testing does not test either the const or the readonly modifiers. It only tests static and none.

Then there's the fixer:

roslyn-analyzers/src/NetAnalyzers/Core/Microsoft.CodeQuality.Analyzers/QualityGuidelines/UseLiteralsWhereAppropriate.Fixer.cs

Lines 57 to 87 in f4c8475

	foreach (SyntaxToken modifier in GetModifiers(fieldDeclaration))
	{
	if (IsStaticKeyword(modifier) || IsReadonlyKeyword(modifier))
	{
	// The associated analyzer ensures we'll only get in the fixer if both 'static' and 'readonly'
	// keywords are in the declaration. Because their order is not relevant, we detect if both
	// have been passed by inspecting whether leading and trailing trivia are non-empty.
	if (leadingTrivia.Count == 0 && trailingTrivia.Count == 0)
	{
	leadingTrivia = leadingTrivia.AddRange(modifier.LeadingTrivia);
	trailingTrivia = trailingTrivia.AddRange(modifier.TrailingTrivia);
	}
	else
	{
	// Copy the trivia in-between both keywords ('static' and 'readonly') into
	// the combined set of trailing trivia.
	trailingTrivia = trailingTrivia.AddRange(modifier.LeadingTrivia);
	trailingTrivia = trailingTrivia.AddRange(modifier.TrailingTrivia);
	// We have processed both the keywords 'static' and 'readonly', so we insert the 'const' keyword here.
	// In case any additional modifiers will follow, their relative position should not change.
	SyntaxToken constModifier =
	GetConstKeywordToken().WithLeadingTrivia(leadingTrivia).WithTrailingTrivia(trailingTrivia);
	newModifiers = newModifiers.Add(constModifier);
	}
	}
	else
	{
	newModifiers = newModifiers.Add(modifier);
	}
	}

So the intent from this comment seems to be that under the condition when the analyzer does not ignore a given field, it will offer a fixer only if the field has both static and readonly keywords. Except that static isn't even checked before this block.

So if the field has both static and readonly, it will offer a fixer that does not include either static or readonly and instead replaces it with const. And all other modifiers are passed through normally. But if only readonly is given, the const modifier is never created, and readonly is not passed through with other modifiers, so it is dropped entirely. The same goes for the only static case, but readonly is ensured while static is not.

The only way that the analyzer can ensure static is present in order to offer the fixer is if static is in the required_modifiers list. With no list, static is used as the default, but any list of SymbolModifiers modifiers is valid, with or without static.

So for any required_modifiers list without static, the fixer for a field with readonly and not static will offer a code fix that just removes readonly without ever adding const. That's for sure a bug. But it's one issue that is part of a larger problem.

Compare this to the documentation, which gives conflicting information in different places. First, there's the CA1802 page:

https://github.com/dotnet/docs/blob/6e0c29d100a208170e81aa459f86139a02e642e1/docs/fundamentals/code-analysis/quality-rules/ca1802.md?plain=1#L82-L91

### Required modifiers

You can configure this rule to override the required field modifiers.
By default, `static` and `readonly` are both required modifiers for fields that are analyzed.
You can override this to a comma separated listed of one or more modifier values from the below table:

| Option Value         | Summary                                                  |
| -------------------- | -------------------------------------------------------- |
| `none`               | No modifier requirement.                                 |
| `static` or `Shared` | Must be declared as 'static' ('Shared' in Visual Basic). |
| `const`              | Must be declared as 'const'.                             |
| `readonly`           | Must be declared as 'readonly'.                          |

This lists none, static, const, and readonly as options, when in reality all modifiers in SymbolModifiers are valid, with the exception of const which effectively disables the rule and readonly which is redundant.

It also says the default is static and readonly, but the actual default is just static.

Then there's the Code Quality Rule Options page:

https://github.com/dotnet/docs/blob/6e0c29d100a208170e81aa459f86139a02e642e1/docs/fundamentals/code-analysis/code-quality-rule-options.md?plain=1#L141-L158

### required_modifiers

| Description | Allowable values | Default value | Configurable rules |
| - | - | - | - |
| Specifies the required modifiers for APIs that should be analyzed | One or more values from the below allowed modifiers table

Separate multiple values with a comma (,) | Depends on each rule | [CA1802](quality-rules/ca1802.md) |

| Allowed Modifier | Summary |
| --- | --- |
| `none` | No modifier requirement |
| `static` or `Shared` | Must be declared as `static` (`Shared` in Visual Basic) |
| `const` | Must be declared as `const` |
| `readonly` | Must be declared as `readonly` |
| `abstract` | Must be declared as `abstract` |
| `virtual` | Must be declared as `virtual` |
| `override` | Must be declared as `override` |
| `sealed` | Must be declared as `sealed` |
| `extern` | Must be declared as `extern` |
| `async` | Must be declared as `async` |

This correctly lists all of the modifiers in SymbolModifiers as valid, but makes no mention of const disabling the rule and readonly being redundant. It does not indicate why any of the rest of the modifiers might be used in required_modifiers.

It also does not give a default value, instead saying that the default value depends on the rule, even though there is only one rule: CA1802.

Am I missing something here? It seems like this started with a request in #2772 to have the option to not always require static and readonly, and the fix for that in #2808 introduced required_modifiers which doesn't even succeed in doing that correctly and is now documented incorrectly twice in different ways on top of that.