☂️ [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 dart-lang/linter#4972 (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.

[lrhn]

I don't know what to call it, but I'd want a "unnecessary wildcard" warning for any local variable declaration, type parameter variable declaration or function type parameter declaration named _ that isn't:

• An (obviously positional) parameter name
• A type parameter name
• The error of a catch clause followed by a non-_ stack trace variable.

(Pattern _s are not variable declarations at all, they're a non-binding pattern.)

That means the following are Bad:

var _;
... } catch (_) { ...     // use `on Object` instead.
... } catch (_, _) { ...
... } catch (e, _) { ...
int Function(int _) f;  // Just remove the `_`.

Don't use a variable declaration named _ where no variable would work instead.

[natebosch]

@kallentu said:

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

Can you elaborate on why? My gut reaction is aligned with Jake - I think we should bundle these.

[natebosch]

@lrhn said

That means the following are Bad:

... } catch (_) { ...     // use `on Object` instead.
... } catch (_, _) { ...
... } catch (e, _) { ...

But earlier in #55723 (comment) said

I wouldn't say anything about } catch (_) {, } catch (_, _) { or even } catch (e, _) {. They're fine as they are. Slightly longer than necessary for the last two, but not something worth bothering the author about unless they have a specific "As short as possible" lint.

I think a middleground would avoid making a style decision, while still guiding against unnecessary syntax. IMO we should allow both } catch (_) { and } on Object {, but lint on } catch(e, _) { and } catch(_, _) {.

[lrhn]

Ack. These examples would be a lint that you opt in to, the "as short as possible"/"no unnecessary _" lint.

But it is different from var _ = …, which is a lint I want in core, whereas catch (_, _) would probably not even be in recommended.

[kallentu]

@kallentu said:

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

Can you elaborate on why? My gut reaction is aligned with Jake - I think we should bundle these.

I think the main thing I was thinking about was what Jake mentioned with top level or member declarations. I had in mind that we would lint on those.

If we did, I can see a world where perhaps someone wouldn't want to use that lint, but would be okay with the other two lints on catch clauses and unnecessary underscores. In that case, I was thinking perhaps split them up?

If we didn't, like Jake suggested, I'm sure that would make the migration easier and we keep them all bundled if the rollout works out. It's a little bit of a shame because I come across unused parameters in members a lot and see plenty of CLs go by to clean up method signatures.

If we do bundle them, I would like to make sure the documentation for the wildcards lint bundle includes all these examples and is pretty thorough. A nice thing about very granular lints/warnings is that their documentation is straightforward and to the point.

[natebosch]

That means the following are Bad:

var _;

I can't recall where (#27401 touches on this but I think there was more conversation about this), but at some point we talked about var _ = someFuture(); as an alternative to unawaited(someFuture());. IIRC we specifically called out how unusable _ variables could make that pattern more useful since it wouldn't get the unused variable diagnostic. I think we even discussed allowing multiple uses of that pattern without an error for redefining the variable. I'm not sure if any of that should still be considered.

[pq]

at some point we talked about var _ = someFuture(); as an alternative to unawaited(someFuture());

Yeah, I recall this conversation too.

Note that today (pre-wildcard feature), underscore identifiers are treated as an idiomatic way to silence use result warnings when you don't want to get an unused element warning (but think you know what you're doing in ignoring the result):

@useResult
int f() => 42;

void g() {
  var _ = f(); // NO WARNING
}

As it stands, the behavior will be same with real wildcards.