Skip to content

Don't guard Dictionary<K, V>.Remove(key) by ContainsKey(key)#4836

Merged
buyaa-n merged 47 commits intodotnet:mainfrom
chucker:features/33797-do-not-guard-dictionary-remove-by-dictionary-containskey
Apr 29, 2022
Merged

Don't guard Dictionary<K, V>.Remove(key) by ContainsKey(key)#4836
buyaa-n merged 47 commits intodotnet:mainfrom
chucker:features/33797-do-not-guard-dictionary-remove-by-dictionary-containskey

Conversation

@chucker
Copy link

@chucker chucker commented Feb 11, 2021

This implements dotnet/runtime#33797, including a small number of tests for both C# and VB.

By design, the diagnostic will not be reported if the condition contains multiple statements, as suggested by @carlossanlop. That means it doesn't yet include @terrajobst's suggested further improvement of hoisting the Remove call into the condition, and leaving the remaining statements inside the block.

@chucker chucker requested a review from a team as a code owner February 11, 2021 23:09
@dnfadmin
Copy link

dnfadmin commented Feb 11, 2021 •
edited
Loading

CLA assistant check
All CLA requirements met.

@Youssef1313
Copy link
Member

Youssef1313 commented Feb 12, 2021

You'll need to rebase on .NET 6 release branch.

@mavasani Should this go to preview 1 or preview 2 branches?

TIP: I'd suggest hard-resetting the last commit (13c184b) before rebasing. This should decrease conflicts, then re-run pack target after rebasing.

mavasani
mavasani previously requested changes Feb 16, 2021
View reviewed changes
Copy link

@mavasani mavasani left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New .NET6.0 analyzers must target release/6.0.1xx-preview2 branch, please retarget.

@chucker chucker force-pushed the features/33797-do-not-guard-dictionary-remove-by-dictionary-containskey branch from 13c184b to 0a3bdb3 Compare February 16, 2021 16:33
@chucker chucker changed the base branch from master to release/6.0.1xx-preview2 February 16, 2021 16:34
Evangelink
Evangelink reviewed Feb 17, 2021
View reviewed changes
Evangelink
Evangelink reviewed Feb 18, 2021
View reviewed changes
@Evangelink
Copy link
Member

Evangelink commented Feb 18, 2021

Thanks @chucker. You will need to apply the code-fix to add the new rule entry.

@chucker
Copy link
Author

chucker commented Feb 18, 2021

Thanks @chucker. You will need to apply the code-fix to add the new rule entry.

Not sure how I managed to write a code fix and not consider the possibility that adding this rule is as simple as invoking a code fix. :-)

@chucker chucker requested a review from mavasani February 18, 2021 22:12
@chucker chucker force-pushed the features/33797-do-not-guard-dictionary-remove-by-dictionary-containskey branch from 5a836cb to 8155018 Compare February 18, 2021 22:20
@Evangelink Evangelink self-requested a review February 18, 2021 22:22
@buyaa-n
Copy link

buyaa-n commented Mar 3, 2022

New .NET6.0 analyzers must target release/6.0.1xx-preview2 branch, please retarget.

@mavasani this PR is ready for review, need your approval as change requested

@mavasani mavasani dismissed their stale review April 1, 2022 17:01

Now targeting the correct branch

@buyaa-n buyaa-n requested a review from mavasani April 27, 2022 18:05
@mavasani
Copy link

mavasani commented Apr 28, 2022

@buyaa-n Seems like there are real test failures.

@buyaa-n
Copy link

buyaa-n commented Apr 28, 2022

@buyaa-n Seems like there are real test failures.

Failed on .net472 builds, looks Remove overload with out parameter is not exist in .Net framework, I will make that overload optional

@buyaa-n
Copy link

buyaa-n commented Apr 29, 2022

Thanks @chucker for your contribution, the PR is finally ready for a merge.

Looks @carlossanlop requested a change long time ago to unify this PR with @CollinAlpert's PR, to ensure all 3 closely-related analyzers are handled together. But somehow @CollinAlpert were not able to push his changes to this PR, I had offline conversation with @carlossanlop about this issue and we decided to merge this PR first and let @CollinAlpert work on main branch instead. Seems Carlos forgot to remove the change request, as he is out in rest of the week I am dismissing it to unblock the PR

@buyaa-n buyaa-n dismissed carlossanlop’s stale review April 29, 2022 06:27

We decided to merge this PR first

@buyaa-n buyaa-n merged commit 757243c into dotnet:main Apr 29, 2022
@github-actions github-actions bot added this to the vNext milestone Apr 29, 2022
@CollinAlpert
Copy link

CollinAlpert commented Apr 30, 2022

Thanks @buyaa-n. So I will just create a new branch off of main and work on there?

@buyaa-n
Copy link

buyaa-n commented May 1, 2022

Thanks @buyaa-n. So I will just create a new branch off of main and work on there?

Exactly

@CollinAlpert
Copy link

CollinAlpert commented May 2, 2022

I just merged main into my existing PR and made the necessary changes. It is ready to be reviewed.

@Youssef1313 Youssef1313 mentioned this pull request May 3, 2022
Closed
10 tasks
@jmarolf jmarolf modified the milestones: vNext, .NET 7.x Nov 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Evangelink Evangelink Evangelink approved these changes

@carlossanlop carlossanlop Awaiting requested review from carlossanlop

+3 more reviewers

@Youssef1313 Youssef1313 Youssef1313 left review comments

@buyaa-n buyaa-n buyaa-n approved these changes

@mavasani mavasani mavasani approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@genlu genlu

@carlossanlop carlossanlop

@buyaa-n buyaa-n

Labels

None yet

Projects

None yet

Milestone

.NET vNext

Development

Successfully merging this pull request may close these issues.