avoid_unused_constructor_parameters false positives

[davidmorgan]

An unused constructor parameter is occasionally needed to force something to be instantiated, for example when using dependency injection.

We could use an ignore comment for these cases, but it would be nice to have a prettier / more descriptive way to do it.

Some other languages, e.g. python, allow prefixing the variable name with _ to mark it as something that is intentionally unused.

What do you think, please? I'd be happy to add this if it sounds good.

Thanks!

[bwilkerson]

I think that underscore already has a meaning in Dart and that it might be confusing to overload it (even though marking a parameter as private is generally a bad practice unless it's a field formal parameter).

I think it might be better to introduce an annotation that would indicate the reason why it's ok, such as @neededForInjection.

[pq]

Some other languages, e.g. python, allow prefixing the variable name with _ to mark it as something that is intentionally unused.

I'm not sure we're explicitly supporting this idiom (though I thought for sure we were). We do use it a bunch of places internally however.

Anyway, it's a fairly common pattern (I can add the lua linter to the list) so I don't object to making it first class in analysis -- though if we do we ought to consider other ramifications.

Having said that, an annotation has other advantages -- documentation in particular.

Can we have both? :)

[pq]

Aside: should we consider a broader rule that adds this check to functions/methods generally?

[bwilkerson]

I'm not sure we're explicitly supporting this idiom (though I thought for sure we were). We do use it a bunch of places internally however.

I'm aware of the convention of using identifier names consisting of only underscores to mean "ignored" (typically in closures), but I wasn't aware we were using other private identifiers that way.

Can we have both?

It's generally better to not have multiple ways of accomplishing the same goal unless there are reasons why it's required, so I'd rather not.

Aside: should we consider a broader rule that adds this check to functions/methods generally?

I think we actually did consider this originally. It doesn't work for methods because they can be overridden and a parameter that isn't used in the base implementation might well be used by an overridding method. (And, of course, we couldn't lint abstract methods even though none of the parameters are refrenced.) We could have a similar lint for functions (top level or local) because they, like constructors, can't be overridden.

[creisman]

Sorry, my original suggestion wasn't very clear. My intent was to recommend that unused variables have names consisting entirely of underscores, not prefixed with it. As already pointed out, this pattern is used pretty commonly for closures (search Dart code for "()"). When there's multiple unused variables, you just use multiple underscores ("(, __)").

The other option, which I think Java uses, is to allow variables prefixed with "unused" to be ignored as well. This is useful when you still want to indicate what the variable represents, you just don't care about the value (unusedIndex, unusedValue, unusedStream, etc).

[pq]

@davidmorgan: is this still a desired improvement?

[davidmorgan]

Yes, I think having _ as a variable name turn off the check would be a nice path, and there's a decent chance we'd enforce it.