Remove casts to IDisposable where unneeded

[jamesqo]

Subissue of PR #2318, doesn't necessarily increase perf but it should for readability.

[jamesqo]

Sorry, my editor has a tendency to trim whitespace sometimes. For the second commit, go to line 1362.

[khellang]

Thank goodness for GitHub's well-hidden ?w=1 switch 😉

[sharwell]

@James-Ko It would be a good idea to either disable that feature in your editor, or really get used to only staging the lines in Git which you actually intended to change. With this pull request, Visual Studio will now say (in CodeLens) the last reason ConcurrentDictionary<TKey, TValue>.CopyToPairs changed was "Remove unneeded cast to ConcurrentDictionary". I believe it did the same for 11-12 other methods.

[stephentoub]

I agree with @sharwell. Please only submit changes relevant to the issue at hand. The real meat of this PR looks reasonable and like something we should take, but I'll wait to review until the whitespace changes are undone. Thanks, @James-Ko.

[jamesqo]

Alright, the whitespace problems should be fixed now. Git undid some of the indentation changes as well, though, so I had to address that too.

[stephentoub]

This comment is unnecessary. It can be deleted entirely rather than modified.

[nguerrera]

I wonder why the author of the original code was worried about the case where a subclass had re-implemented IDisposable. I much prefer what you have, but this is technically an externally observable change in that pathological case and there's a cryptic comment showing intention for the old behavior. cc @davkean

I wouldn't bother commenting the plain Dispose at all.

[stephentoub]

@nguerrera, @davkean, @weshaggard, so is this not a change we should take? I'm not sure why we'd be using this pattern to pick up the custom IDisposable implementation here but not elsewhere.

[davkean]

I don't know the history behind this particular code, but think about the time it was written:

TextReader only had Close and explicitly implemented Dispose. Close was virtual and there was a virtual Dispose(bool). Perhaps they were attempting to skip some overriden Close behavior?

[jamesqo]

@nguerrera I don't see any effect this would have, actually. Neither TextWriter or TextReader explicitly impl IDisposable, and Dispose is resolved during compilation to pick up on those implementations of IDisposable.

[davkean]

@James-Ko At the time the code was written, TextWriter and TextReader explicitly implemented IDisposable. Theoretically, also I can have my own TextReader/TextWriter implement IDisposable again, and completely skip the base's implementation. Such a class would experience a behavior change in above. However, the likelihood of such a thing is probably pretty low and such an implementation would have different broken behavior with other things that called Dispose directly.

ie I wouldn't worry about it.

[stephentoub]

Thanks, David. Agreed.

[sharwell]

I'm not sure why we'd be using this pattern to pick up the custom IDisposable implementation here but not elsewhere.

Technically this is the behavior of a using statement so it gets used frequently.

[stephentoub]

@sharwell, good point.

[weshaggard]

I'm also fine with this change.

[stephentoub]

Thanks for undoing the whitespace changes. I left a couple of additional comments to be addressed, but otherwise LGTM. Please remove those comments and then squash the commits. Thanks, @James-Ko.

[ghost]

In addition to what @sharwell suggested, you may want to install EditorConfig plugin/extension for your editor (available for almost all major editors).
This repository has .editorconfig file at root. With that in place and EditorConfig plugin installed, it automagically overrides your editor settings for that particular instance of your editor, in which the project is loaded and prevents misformatting.

[jamesqo]

Got it. Also thanks @jasonwilliams200OK for the suggestion.

[stephentoub]

@James-Ko, there are still a few places where code is being modified with whitespace-only changes. As it's just a few, I'll go ahead and merge this, but in future PRs please avoid that.

Thanks for the contribution!