Prefer Dictionary.Remove(key, out value) over Dictionary.this[key], followed by Dictionary.Remove(key)

[terrajobst]

Dictionary.ContainsKey<K, V(key), followed by Dictionary<K, V.this[key], followed by Dictionary<K, V>.Remove(key) can be combined into a single Remove call, using the overload accepting out TValue value.

Category: Performance

[jeffhandley]

Estimates:

• Analyzer: Medium
• Fixer: Medium

[carlossanlop]

This is closely related to #33797

Suggested severity: Info

// Before
        public void RemoveValue(Dictionary<string, int> data, string key)
        {​​
            if (data.ContainsKey(key))
            {
                // Dictionary.ContainsKey<K, V(key),
                // followed by Dictionary<K, V.this[key],
                // followed by Dictionary<K, V>.Remove(key) can be combined into a single Remove call,
                // using the overload accepting out TValue value.
                Foo(data[key]);
                data.Remove(key);
            }
        }​​

// After
        public void RemoveValue(Dictionary<string, int> data, string key)
        {​​
            if (data.Remove(key, out int value))
            {
                Foo(value);
            }
        }​​

Applying the below example in the analyzer might be more complicated:

// Before
        public void RemoveValue(Dictionary<string, int> data, string key)
        {​​
            if (data.ContainsKey(key))
            {
                int x = data[key];
                data.Remove(key);
                Foo(x);
            }
        }​​

// After
        public void RemoveValue(Dictionary<string, int> data, string key)
        {​​
            if (data.Remove(key, out int value))
            {
                Foo(value);
            }
        }​​

We should also consider the case where the data is used multiple times, and when there's unrelated code in the way:

// Before
        public void RemoveValue(Dictionary<string, int> data, string key)
        {​​
            if (data.ContainsKey(key))
            {
                Foo(data[key]);
                UnrelatedMethodCall();
                Bar(data[key]);
                data.Remove(key);
            }
        }​​
// After
        public void RemoveValue(Dictionary<string, int> data, string key)
        {​​
            if (data.Remove(key, out int value))
            {
                Foo(value);
                UnrelatedMethodCall();
                Bar(value);
            }
        }​​

[bartonjs]

Video

We came up with a lot of cases where the fixer can cause surprising functional changes, so this really only applies when there's an indexer followed by a call to Remove with no other method calls in between (because any method call could have modified the dictionary). As a specific example, in #33800 (comment) if Foo throws then in the before case the dictionary is not modified, but in the after case it is.

The case of x = dict[key]; dict.Remove(key); isn't safe to replace, because the exception is lost.

Conclusion:

If we find a dictionary call to TryGetValue that is immediately followed by a call to Remove (and the dictionary and the key are both simple variable or parameter expressions) then suggest collapsing them to Remove(key, out T). ("immediate" definitely means that there are no method calls or delegate invocations between those two expressions)

• Category: Performance
• Severity: Information

[steveberdy]

@buyaa-n may I have this assigned to me? Thanks.

[buyaa-n]

Thank you @steveberdy, sorry for the delayed response, it is assigned to you now, JFYI there are several analyzer proposals for dictionary, their implementations preferred to be written in one place (with different diagnostic ID when needed). I see only one of them merged into repo please try to add your implementation to that

[steveberdy]

I'm sorry. I don't really have time to make this analyzer. I've unassigned myself.