dart-lang / linter

Linter for Dart.
https://dart.dev/tools/linter-rules
BSD 3-Clause "New" or "Revised" License
628 stars 170 forks source link

[Wildcard Variables][lint] `no_wildcard_variable_uses` and the new wildcard variables feature. #4968

Open kallentu opened 4 months ago

kallentu commented 4 months ago

Changes to make

(Updated May 14th 2024)

  1. Change the documentation of the lint to something like: Wildcard parameters and local variables (whose name is '_' by definition) will become non-binding in a future version of the Dart language.
  2. This lint should WARN on _ (not __ etc) if it's being used.
  3. UNUSED_VARIABLE will have an additional fix to suggest converting to _.

As mentioned from https://github.com/dart-lang/language/pull/3785#discussion_r1593537182.

https://dart.dev/tools/linter-rules/no_wildcard_variable_uses no_wildcard_variable_uses reports all named underscores, which is more than what we want.

This lint will flag usages of names of the form __, _ and so on, in addition to . Does it give rise to unnecessary churn to flag all the names that have more than one underscore, given that they will keep their status as regular (non-wildcard) identifiers? For example, if someone really wants to use local variables with the name then they might switch to just because it's similar, and not overly long, and they might be unhappy if they still get the lint message.

In the context of implementing the new wildcard variables feature (spec), what changes, if any, do we want to make to the no_wildcard_variable_uses lint?

1. Migration/Breakage

One purpose of this existing lint is for an easier migration/breakage: which since it's enabled in the core lint set, should make the breakages across projects pretty small.

2. Wildcard variables feature

However, once the wildcard variables feature ships, we will most likely need to make a new lint and remove/deprecate(?)/change this one (because wildcards can be used :) ).

As @eernstg said, add a new lint that "targets the cases where _ resolves to a declaration which would be wildcarded when that feature is enabled."

cc. @pq

eernstg commented 4 months ago

One change that would make sense would be to change the documentation:

Wildcard parameters and local variables (e.g. underscore-only names like , , , etc.) will become non-binding in a future version of the Dart language

to something like

Wildcard parameters and local variables (whose name is _ by definition) will become non-binding in a future version of the Dart language.

... and then something about how names of the form __, ___, ____, etc. are treated, and why.

bwilkerson commented 4 months ago

I mentioned this possibility in the kick-off meeting, but I'll mention it again.

Unless I'm mistaken, the wildcard variables feature is a breaking change. I don't know whether the next stable release is expected to be the point at which we introduce this breaking change or whether we're going to hold off shipping the feature until we have a few more breaking changes to bundle with it, but if the plan is the latter then we might want to consider making the warning about _ be a warning rather than a lint. Doing so would make sure that every user sees the notice of the upcoming breaking change and can migrate in advance.

If that's the case, then the next question becomes whether we should deprecate the lint or whether we should leave it in place (but no longer warning about _) in order to allow for the possibility that multi-underscore names might be treated specially (whether as wildcards or otherwise) in an even more future release.

srawlins commented 4 months ago

+1 to making the warning about _ usage a warning (default enabled) rather than a lint for one stable release before the stable release where the feature is flipped to enabled by default.

lrhn commented 4 months ago

For the lint, I'd personally want to warn about every __, ___, etc. name. Everywhere, even where even _ is allowed.

I'll want the lint to say: Use _ instead of __ or __ (etc.) where _ is now non-binding. Where _ is not non-binding, I'd actually still say to never use __ or ___ as names. They're just not readable. They suggest that the (variable) name is never used, and they're still not very good at that.

It's a lint, it's allowed to be opinionated. If someone disagrees, they can // ignore it.

natebosch commented 4 months ago

If we change the unused_variable diagnostic to remove the existing exception for __ I think it would provide the desired protection for local variables when it's used in combination with no_variable_wildcard_uses. It wouldn't cover parameters named __ though, so adding that coverage here sounds good to me.

kallentu commented 4 months ago

Updated the original post with changes we'd like to see.

The combination of the following (resulting from our discussion) will help users move to the new feature a lot more easily.

  1. UNUSED_VARIABLE being triggered on __ and etc, additional quickfix to convert to _.
  2. no_wildcard_variable_uses warns on _

This issue will continue to track what's needed for no_wildcard_variable_uses.