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

Lint when using variable named _ #51221

Closed
munificent opened this issue Feb 1, 2023 · 19 comments
Closed

Lint when using variable named _ #51221

munificent opened this issue Feb 1, 2023 · 19 comments
Labels
analyzer-language-patterns Issues with analyzer's support for the patterns language feature analyzer-linter Issues with the analyzer's support for the linter package area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P2 A bug or feature request we're likely to work on

Comments

@munificent
Copy link
Member

munificent commented Feb 1, 2023

Inside a pattern, an identifier named _ is a non-binding wildcard. This means you can use it more than once without a collision error:

var [a, _, b, _] = [1, 2, 3, 4]; // OK.

With patterns, we are not changing the behavior of declarations named _ outside of patterns. So this still works:

var _ = 1;
print(_);

But we would eventually like to change the language so that (at least) parameters and local variables named _ become non-binding.

This change is breaking because currently those identifiers are binding and can be used. It's rare for users to refer to parameters and variables named _, but I do see some examples of it in the wild.

To ease that transition, we could add a lint that fires whenever you use a parameter or variable named _ (or any sequence of only underscores, __, ___, etc.) The lint would suggest that they pick another name for the variable if they're going to refer to it. The lint wouldn't warn if you declare a variable or parameter named _. That's fine. It should only be a violation to have an identifier expression or assignment target that uses one of these variables.

That should make it less breaking to make _ non-binding in a later Dart release.

cc @dart-lang/language-team

@munificent munificent added area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. analyzer-linter Issues with the analyzer's support for the linter package labels Feb 1, 2023
@bwilkerson
Copy link
Member

Does it need to report every use of a variable, or only the first use? I'm thinking the latter would be a better UX, but there might be reasons for the former that I'm not thinking about.

@leafpetersen
Copy link
Member

Worth calling out that this lint should not fire on uses of members (or at least constructors) named _, since ClassName._ is a common idiom for a private constructor.

@jakemac53
Copy link
Contributor

jakemac53 commented Feb 1, 2023

Worth calling out that this lint should not fire on uses of members (or at least constructors) named _, since ClassName._ is a common idiom for a private constructor.

I was just about to comment that this should just apply to all declarations, but this is a good point :(. Feels weird to special case constructors too.

@munificent
Copy link
Member Author

Yeah, I was deliberate in my wording but I should have emphasized the "parameters and local variables" bit.

@jakemac53
Copy link
Contributor

jakemac53 commented Feb 1, 2023

I do think that a constructor named _ is really the only declaration that makes sense to ever have that name though 🤷‍♂️

@natebosch
Copy link
Member

The constructor _ is also not referenced without the class name. It feels consistent to error on referencing a bare _, but to not error on ClassName._. I don't think it matters if that leaves a loophole for a static method or variable named _.

@eernstg
Copy link
Member

eernstg commented Feb 2, 2023

The language specification has used the terminology "a constructor named C.name" for several years. With that terminology there is no overlap at all for a constructor named C._ and other declarations named _.

(As an aside, this terminology ensures that no constructors are anonymous/nameless (but a constructor can have the same name as the enclosing class), and it is consistent with the actual syntax that denotes a constructor in an instance creation (and constructor tear-offs are not that different). So I'd recommend that we just use this terminology. ;-)

I believe this would make it possible to use the following definition of the lint:

Emit a message for each identifier expression which resolves to a declaration named _.

This will not flag instance creations like C._(...) or constructor tear-offs like C._, and it will not flag this._ or someReceiver._. Those constructs would be benign anyway, in the sense that they can't possibly be wildcards.

Should we also emit 'deprecated choice of name' diagnostics for every declaration named _? It's all in the same library, so it shouldn't be difficult to rename such declarations. If we don't do this then it would be possible to keep declarations named _ in place indefinitely, as long as they are accessed using this._ in all those cases where it would otherwise cause a lint. That might make an eventual language change where declarations just can't have the name _ less smooth.

@lrhn
Copy link
Member

lrhn commented Feb 2, 2023

Warning about every declaration named _ would warn about (_) => 42, which is precisely where we'd want to allow the _.

There are places where we'd want to allow _ named declarations, but probably only local variables:

  • Parameters, (_) => 42
  • Catch variables, catch (_, s)
  • For-in variables, for (var _ in iterable) count++;

That's places where a name is required, but we may not care about the value.
In these cases we should warn if you actually refer to the _ variable later.

A local variable declaration of var _; is not necessary. If you refer to it, give it a better name. If not, there is no need to have the variable to begin with.

Non-local variable or function declarations named _ should probably be given a better name.

If we push this as lints, and people follow it, there will never be a _ which resolves in the lexical scope. There is either no binding for it at all, or it's a local binding that you are not referencing.

@eernstg
Copy link
Member

eernstg commented Feb 2, 2023

There are places where we'd want to allow _ named declarations

Very good, then we have a good estimate of where to warn: Everywhere else. ;-)

@lrhn
Copy link
Member

lrhn commented Feb 2, 2023

There was a comment elsewhere that some people are doing:

@metadata
var _ = expression;

to attach metadata to an expression, because we only allow metadata on declarations.
They might want that to keep working. As long as it's only a lint, they can ignore it.
Or we can just allow _-named local variables everywhere, and just lint on references to those variables.
That'll still ensure that making the variable non-binding won't cause a change.

@jakemac53
Copy link
Contributor

Warning about every declaration named _ would warn about (_) => 42, which is precisely where we'd want to allow the _.

If we define that those don't actually introduce a local variable into the scope, then we just have some special type of declaration for those, which is the only thing allowed to have the name _?

@jakemac53
Copy link
Contributor

Regarding metadata given this is always a local (and trivial) change I would just not allow it, force those to have a real name.

@leafpetersen
Copy link
Member

Warning about every declaration named _ would warn about (_) => 42, which is precisely where we'd want to allow the _.

This lint request explicitly says "The lint wouldn't warn if you declare a variable or parameter named _." This is about uses, not declarations.

@srawlins srawlins added the P2 A bug or feature request we're likely to work on label Feb 6, 2023
@pq pq added the analyzer-language-patterns Issues with analyzer's support for the patterns language feature label Apr 17, 2023
@pq
Copy link
Member

pq commented Apr 17, 2023

@munificent: assuming the wildcard proposal still has traction, do we want to push ahead on this?

@munificent
Copy link
Member Author

We didn't get the wildcard proposal done in time for 3.0, but I think there is still generally a desire to move in that direction, yes. I think the lint is definitely valuable.

@pq
Copy link
Member

pq commented May 26, 2023

Lint proposal cooking: #59157

@pq
Copy link
Member

pq commented Jul 19, 2023

The no_wildcard_variable_uses lint has landed and it'd be good to get feedback and perhaps add it to the core lints (dart-lang/lints#139)?

@leafpetersen
Copy link
Member

Yes, we should definitely aim to get this into the core lints IMO, at least once we've shaken it out. cc @devoncarew @natebosch @mit-mit @lrhn

@parlough
Copy link
Member

I'm going to close this as completed as no_wildcard_variable_uses now exists in Dart 3.1 and later. dart-lang/lints#165 also added it to the core set of lints, set to be included in the package's next major release.

Thanks everyone! If you'd like to see anything further done to the lint, please open a new issue. As for its inclusion in the core lint set, please open a new issue at https://github.com/dart-lang/lints/issues to suggest a new major release if it's necessary for that to occur earlier than the normally scheduled release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analyzer-language-patterns Issues with analyzer's support for the patterns language feature analyzer-linter Issues with the analyzer's support for the linter package area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P2 A bug or feature request we're likely to work on
Projects
None yet
Development

No branches or pull requests

10 participants