Roslyn: Fade out unused parameters

[Neme12]

I think it would be a good addition to dead code analysis to fade out unused parameters on private methods, as well as parameters that are never read (similar to the current diagnostics for fields & properties).

Tagging @mavasani

[Neme12]

Actually this could even be extended to all internal methods (that are not an override, implementation, abstract or virtual) since the analysis would still only have to cover the method body.

[Therzok]

What about this?

https://github.com/dotnet/roslyn-analyzers/blob/master/src/Microsoft.CodeQuality.Analyzers/Core/Maintainability/ReviewUnusedParameters.cs

[mavasani]

I am actually prototyping this now 😊. I am going to start with fading dead assignments to locals and parameters based on values assigned that are never read on any control flow path (based on dataflow analysis). This can be extended to find parameters whose initial value is never used by assuming they are assigned at start of code block execution. I hope to have a PR within a week or two.

[mavasani]

I am thinking of an editorconfig option that allows users to configure how to deal with unused values: explicitly assign to a discard OR explicitly assign to a local variable that is never read on any control path, with the former being the default setting. This also enables fading expression statements that drop a computed value, so the user intend is more clear (yes I want to ignore the expression value on purpose but still need to run the computation OR oops this is a dead computation that should be removed).

[Neme12]

@Therzok Thanks, I didn't know about that one (but it's much less usable since this particular analyzer doesn't work with the VSIX and requires adding a NuGet package to the project - I'd like this as a general code quality analyzer I can see on all code I work on, regardless of whether I control it... for example in Roslyn :smile:).

[Neme12]

I am thinking of an editorconfig option that allows users to configure how to deal with unused values: explicitly assign to a discard OR explicitly assign to a local variable that is never read on any control path

That sounds interesting, althought I'm not sure if such a code style is necessary, we should probably try to nudge users to standardize on one thing (probably a discard) and only add other options if there's a request for them. But of course that's a question for the design review.

[CyrusNajmabadi]

Note: this exists in TypeScript, but is overly aggressive. It tells me about unread parameters taht i can't do anything about (i.e. because my method is an implementation of some required interface method). As with other 'unnecessary' features, can we only do this for code we're certain the user can safely modify. Thanks!

[Neme12]

@CyrusNajmabadi Of course that's a concern, that's why I said:

Actually this could even be extended to all internal methods (that are not an override, implementation, abstract or virtual)

there may very well be other cases I'm not aware of (and we should find them). I just didn't mention these in the original post with private methods because none of them apply to private methods.

[CyrusNajmabadi]

I wasn't trying to imply you didn't say it :) I was just throwing my hat into the convo to emphasize I thought this was a very important part to get right, since i've experienced first-hand how it can be annoying otherwise :)

[Neme12]

Another idea: fading out unused type parameters would be nice too

[mavasani]

Ported fading out unused type parameters would be nice too to https://github.com/dotnet/roslyn/issues/31399.

The original feature requested was implemented with https://github.com/dotnet/roslyn/pull/31391

[Neme12]

Thanks!