-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
☂️ [wildcards] new and updated diagnostics / lints #56595
Comments
I'm in favor of a new Hopefully that lint won't overlap with |
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.
I agree. Whatever we do shouldn't. Ideally the existing naming lints can avoid wildcard territory entirely. |
Ah sorry, I meant actually "I'm in favour of a new From first glance, I wouldn't actually want to bundle the |
As for the proposed new lints ( <rambling> For example, it seems reasonable for 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. I think figuring out whether these lints would be approved is the next step. |
Thanks Brian! I agree it would be great to get some feedback from 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? |
use_catch_parameters, unused_parameters, unnecessary_underscoresI 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 unused_parametersI 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. |
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
(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 |
@kallentu said:
Can you elaborate on why? My gut reaction is aligned with Jake - I think we should bundle these. |
@lrhn said
But earlier in #55723 (comment) said
I think a middleground would avoid making a style decision, while still guiding against unnecessary syntax. IMO we should allow both |
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 |
…mes` TL;DR this returns the lint to a pre-wildcard feature laissez faire attitude towards underscores. There's ongoing conversation about how we want to nudge folks to avoid needless use of underscores on #56595 but it's not likely to happen in this lint (and almost certainly not before the feature is enabled). Change-Id: I9aff2bc225616336655cddbc2635b427265497bf Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/381903 Reviewed-by: Brian Wilkerson <[email protected]> Commit-Queue: Phil Quitslund <[email protected]>
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. |
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 |
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. |
It's been a long time since this issue was updated. Is there still work to be done here? |
I think we still want to keep this open pending a strategy for nudging folks to use/discover wildcards, but |
After talking to @pq and @bwilkerson, we've decided we're doing to do the following:
|
[implementation note 🧐 ]: ("fix"es in 2 and 3 will technically be "assists" since they won't (yet) have associated diagnostics.) |
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)_
(but not__
... – except constructors where__
+ is ok)no_leading_underscores_for_local_identifiers
(recommended lint)no_wildcard_variable_uses
(core lint)empty_catches
(core lint)_
Annotations (current)
@useResult
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:
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:
Proposal: new lint:
use_catch_parameters
See also #5571 which proposes that such cases be treated as warnings (
UNUSED_LOCAL_VARIABLE
or a newUNUSED_CATCH_PARAMETER
diagnostic).Reasons for a lint over a warning:
Unnecessary underscores.
With
_
binding, developers have adopted an idiom of using multiple underscores for parameters that they intend to be unused.For example:
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:
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.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:
or:
Complications:
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
: unchangedno_leading_underscores_for_local_identifiers
: unchangedno_wildcard_variable_uses
: update to be a no-op with wildcards enabledempty_catches
:use_catch_parameters
is enabled?While this is still a bit rough and not complete, feedback is welcome!
/cc @bwilkerson @kallentu
The text was updated successfully, but these errors were encountered: