Prefer Dictionary<K, V>.TryAdd(key) over guarded Add(key)

[CollinAlpert]

This PR adds an Analyzer to detect usages of dictionary.Add() guarded by a dictionary.ContainsKey() check. It also adds a CodeFix which offers to refactor this construct into a single dictionary.TryAdd() call.

Fixes dotnet/runtime#33799, #6589

[CollinAlpert]

Ping for review :)

[Youssef1313]

Why not use suspected.Equals(comparator, SymbolEqualityComparer.Default)?

[CollinAlpert]

Because symbolically, System.Collections.Generic.Dictionary<TKey, TValue>.ContainsKey(TKey) and System.Collections.Generic.IDictionary<TKey, TValue>.ContainsKey(TKey) are not equal.

[Youssef1313]

@CollinAlpert There is an IsImplementationOfInterfaceMember extension method that I think could work for this scenario.

[CollinAlpert]

Yeah, I tried substituting the body of the method with return suspected.OriginalDefinition.IsImplementationOfInterfaceMember(comparator);, but that also doesn't work.

[CollinAlpert]

Thank you @CollinAlpert, I did tested with runtime and somehow the fixer is not producing suggestions in VS. Have you tried the use the fixer in VS? Could you take a look into this issue?

That's weird. I double checked everything, and the IDs registered are all correct and passed to the appropriate properties. Unfortunately I don't use VS, so I can't check there. I consolidated the fixers now, maybe that will help?

[buyaa-n]

Thanks for the updates @CollinAlpert, now the fixer is working in VS, so I continued with analyzing all diagnostics and found these false positives:

1. The analyzers flagging an instance of ImmutableDictionary<TKey, TValue>.Builder and suggesting the code fix when it doesn't have a TryAdd(TKey, TValue) method, this case should not be flagged
2. The analyzer flagging and suggesting a fixer when the value is only created within the conditional, this should exclude this scenatio. Example code snippet:

        XmlQualifiedName notationName = GetNameQualified(false);
        SchemaNotation? notation = null;
        if (!_schemaInfo.Notations.ContainsKey(notationName.Name))
        {
            _undeclaredNotations?.Remove(notationName.Name);
            notation = new SchemaNotation(notationName);
            _schemaInfo.Notations.Add(notation.Name.Name, notation);
        }

SchemaNotation notation only created with this is not exist in the dictionary

Alos found one false negative case, it is not flagging the below case when it should:

    public void Test(Dictionary<XmlQualifiedName, SchemaElementDecl> _elementDecls )
    {
        foreach (KeyValuePair<XmlQualifiedName, SchemaElementDecl> entry in sinfo._elementDecls)
        {
            if (!_elementDecls.ContainsKey(entry.Key))
            {
                _elementDecls.Add(entry.Key, entry.Value);
            }
        }
    }

Please fix these issues, thank you!

[CollinAlpert]

@buyaa-n Thanks for the hints. I have addressed them and added tests.

Regarding the false negative, I don't think it is one. Behind a property reference (in this case entry.Value) could be a computation which the developer only wants to trigger when the key is not present in the dictionary. Currently the analyzer only flags literals, local references, field references and constants.

[buyaa-n]

@buyaa-n Thanks for the hints. I have addressed them and added tests.

Thank you!

Regarding the false negative, I don't think it is one. Behind a property reference (in this case entry.Value) could be a computation which the developer only wants to trigger when the key is not present in the dictionary. Currently the analyzer only flags literals, local references, field references and constants.

Well, the KeyValuePair<TKey,TValue >.Value just returns an internal field which set within constructor.

        public KeyValuePair(TKey key, TValue value)
        {
            this.key = key;
            this.value = value;
        }
        public TKey Key => key;
        public TValue Value => value;

Anyway, I guess you meant about any property not specific to KeyValuePair, so this scenario is not a blocker for the PR. We can discuss this scenario with a separate issue.

[CollinAlpert]

Yes, I meant properties in general could have expensive computations in their getter, not specifically KeyValuePair.

[buyaa-n]

Left some NIT, overall LGTM, thank you!

It would be great if you could add a test case for #6589