☂️ [wildcards] new and updated diagnostics / lints

[pq]

Accommodating wildcards implies changes to a number of existing diagnostics and lints. In a number of places we're already supporting a kind of simulation of wildcards (ignoring unused catch parameters for example and ignoring unused identifiers with just underscores and special casing underscores in naming lints) and it's tempting to see this as an opportunity to fix the approximations now that we have actual wildcards and moreover nudge people towards using them appropriately. Unfortunately, this is complicated by interrelationships between the existing lints and diagnostics.

This issue will attempt to tease out these relationships, explore diagnostics we want to report and give us a place to discuss a plan forward.

Lints (current)

• non_constant_identifier_names (core lint)

  • allows parameters with just underscores
  • allows variables and members named _ (but not __... – except constructors where __+ is ok)

• no_leading_underscores_for_local_identifiers (recommended lint)

  • allows variables with just underscores

• no_wildcard_variable_uses (core lint)

  • disallows the use of any identifier that is just underscores

• empty_catches (core lint)

  • allows a catch with an empty body if the parameter is named _
  • requires a body or a comment for all other identifiers

Annotations (current)

• @useResult
  • assignment to any identifier is considered a legitimate “use”

Question: should wildcards be special-cased?

Diagnostics (current)

TODO

Wildcard Uses (future)

With wildcards we can improve some current Dart idioms.

For example:

Unused catch clause parameters.

An idiom for catching and ignoring all thrown exceptions is to write this:

try {

} catch(e) {

}

Since the identifier cannot be omitted we do not report it as unused. (Nor did we choose to require it to be an underscore.)

With wildcards, we might consider flagging all bound parameters (e.g., non-wildcards) and encourage the use of a wildcard. With such a fix, the above code would become:

try {

} catch(_) {

}

Proposal: new lint: use_catch_parameters

See also #5571 which proposes that such cases be treated as warnings (UNUSED_LOCAL_VARIABLE or a new UNUSED_CATCH_PARAMETER diagnostic).

Reasons for a lint over a warning:

1. feels like a style concern which is more the province of lints
2. a warning could be very disruptive (Google3 migration could be onerous)

Unnecessary underscores.

With _ binding, developers have adopted an idiom of using multiple underscores for parameters that they intend to be unused.

For example:

f(int _, int __) { }

With wildcards, we might consider flagging unnecessary (multiple) underscores and encourage the use of a proper wildcard. With such a fix, the above code would become:

f(int _, int _) { }

Proposal: new lint: unnecessary_underscores

Unused parameters.

Unused parameters are not currently reported (except by avoid_unused_constructor_parameters for constructors). In some cases the unused parameters might signal a programming error.

int f(int x, int y, int z) => x + y;

With wildcards, we might consider flagging unused parameters and encourage the use a wildcard variable (or removal of the parameter). With such a fix, the above code would become:

int f(int x, int y, int _) => x + y;

or:

int f(int x, int y) => x + y;

Complications:

• its common practice for parameters in overriding methods to match the name in the overridden method; would we skip those cases?
• for similar reasons, we would not want to recommend an unused parameter in an overridden method be converted to a wildcard but here it's trickier since we can't know if there are any overrides
• and what about augmentations? 😬

Proposal: do nothing.

(It'd be great to get more feedback here.)

EDIT see related discussion and proposal in #59475 (similar conclusion)

Implementation

New lints

I've proposed 2 new lints to ease adoption and Google3 migration. (Alternatively, we might consider a super use_wildcard_variables lint that enforces both proposals but this could limit adoption. Open question!)

If we introduce new lints, existing ones can remain largely unchanged.

Existing lints

• non_constant_identifier_names : unchanged
• no_leading_underscores_for_local_identifiers: unchanged
• no_wildcard_variable_uses: update to be a no-op with wildcards enabled
• empty_catches:
  • do we want to start reporting on non-wildcard parameters?
  • maybe only when use_catch_parameters is enabled?

---

While this is still a bit rough and not complete, feedback is welcome!

/cc @bwilkerson @kallentu

[kallentu]

I'm in favor of a new use_wildcard_variables. I think it's straight-forward and will make the most sense to our users. It also makes our future wildcard migrations a lot easier.

Hopefully that lint won't overlap with no_leading_underscores_for_local_identifiers or non_constant_identifier_names though.

[pq]

I'm in favor of a new use_wildcard_variables.

For enforcing unused catch clause parameters and unnecessary underscores?

It'd be good to get a sense for whether a bundled lint could (reasonably or even aspirationally) land in Google3.

Hopefully that lint won't overlap with no_leading_underscores_for_local_identifiers or non_constant_identifier_names though.

I agree. Whatever we do shouldn't. Ideally the existing naming lints can avoid wildcard territory entirely.

[kallentu]

For enforcing unused catch clause parameters and unnecessary underscores?

Ah sorry, I meant actually "I'm in favour of a new unnecessary_underscores". For that specific error case, a new lint would be great.

From first glance, I wouldn't actually want to bundle the use_catch_parameters lint with the unnecessary_underscores

[bwilkerson]

As for the proposed new lints (use_catch_parameters, and unnecessary_underscores), we should first find out whether we'd be likely to add either to the recommended rule set and whether we'd be likely to enable it internally. In other words, treat them like any other new lint proposal.

<rambling>
As for existing lints, I'm still not certain. On the one hand, it makes sense to me to partition the lints in such a way that users won't get more than one diagnostic reported for a single problem. On the other hand, lints can be enabled independently, and it seems wrong to not report a real problem because a different lint, that might or might not be enabled, would report the same problem. I don't see any way to eliminate the problem, so I guess the question is: is it better to have false negatives (as a result of an enabled rule failing to report a problem because an not-enabled rule would have reported it) or to have multiple diagnostics for a single problem. Neither is great, but I'm kind of leaning toward tolerating multiple diagnostics.

For example, it seems reasonable for non_constant_identifier_names to flag any name that doesn't match the camel-case style (where I'm counting a single identifier to not be a name, and hence not subject to the check). It also seems reasonable for unnecessary_underscores to flag cases where a name consists of only underscores. I suspect that the number of times that there are local variables named with only underscores is small enough that very few users would actually see two diagnostics, while the cost of not reporting poor name choices if only one of the two lints is enabled might be higher.

Of course, the math might come out differently if one or both of the proposed lints is accepted to be in the recommended set, as that strongly increases the probability that all the bases will be covered without the need to risk multiple diagnostics.
</rambling>

I think figuring out whether these lints would be approved is the next step.

[pq]

Thanks Brian!

I agree it would be great to get some feedback from package:lints folks.

Putting it to: @lrhn @goderbauer @natebosch @jakemac53 @davidmorgan @devoncarew @mit-mit ,

Do you all have a sense for how you'd like to see wildcard use supported/promoted in the core/recommended/flutter rulesets?

[jakemac53]

use_catch_parameters, unused_parameters, unnecessary_underscores

I do think we should push users to use wildcards for all of these, and I think they are all close enough that I would create a single "use_wildcard_variables" which covers all three, if we can feasibly roll that out.

If we need separate ones to simplify the roll out process we could also do that, but I don't see why any user would want to enable a subset of these as opposed to all 3. They are essentially all covering the same use case.

It seems like these should all have a trivial dart fix though - with the exception of cases where these current _ variables are actually used.

unused_parameters

I would not ever trigger this lint for top level or member declarations, whether they are overriding or not. It should only apply to local functions and closures imo. There are too many false positives for members here imo to lint on it.