Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
Tracked by #4969
kallentu opened this issue May 9, 2024 · 6 comments
Labels
new-language-feature P2 A bug or feature request we're likely to work on set-core Affects a rule in the core Dart rule set type-question A question about expected behavior or functionality

Comments

@kallentu
Copy link
Member

kallentu commented May 9, 2024

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 dart-lang/language#3785 (comment).

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

@kallentu kallentu added type-question A question about expected behavior or functionality set-core Affects a rule in the core Dart rule set labels May 9, 2024
@kallentu kallentu changed the title [Wildcard Variables] no_wildcard_variable_uses and the new wildcard variables feature. [Wildcard Variables][lint] no_wildcard_variable_uses and the new wildcard variables feature. May 9, 2024
@srawlins srawlins added the P2 A bug or feature request we're likely to work on label May 13, 2024
@eernstg
Copy link
Member

eernstg commented May 13, 2024

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
Copy link
Member

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
Copy link
Member

+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
Copy link
Member

lrhn commented May 13, 2024

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
Copy link
Member

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
Copy link
Member Author

kallentu commented May 14, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-language-feature P2 A bug or feature request we're likely to work on set-core Affects a rule in the core Dart rule set type-question A question about expected behavior or functionality
Projects
None yet
Development

No branches or pull requests

7 participants