Prefer Dictionary.Remove(key, out value) over Dictionary.this[key], followed by Dictionary.Remove(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.

Category: Performance

Estimates:

Analyzer: Medium
Fixer: Medium

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);
            }
        }

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

@buyaa-n

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

@steveberdy

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

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

Navigation Menu

Repository navigation

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

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

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

Description

Activity

jeffhandley commented on Mar 21, 2020

carlossanlop commented on Jan 8, 2021

buyaa-n commented on Jun 3, 2021

bartonjs commented on Jul 27, 2021

steveberdy commented on Oct 15, 2021

buyaa-n commented on Oct 18, 2021

steveberdy commented on Sep 14, 2022

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions