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

☂️ [wildcards] new and updated diagnostics / lints #56595

Open
Tracked by #55680 ...
pq opened this issue Aug 28, 2024 · 17 comments
Open
Tracked by #55680 ...

☂️ [wildcards] new and updated diagnostics / lints #56595

pq opened this issue Aug 28, 2024 · 17 comments
Assignees
Labels
analyzer-warning Issues with the analyzer's Warning codes area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. feature-wildcard-variables Implementation of the wildcard variables feature P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug

Comments

@pq
Copy link
Member

pq commented Aug 28, 2024

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 #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 : 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

@pq pq added P1 A high priority bug; for example, a single project is unusable or has many test failures area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. analyzer-warning Issues with the analyzer's Warning codes feature-wildcard-variables Implementation of the wildcard variables feature labels Aug 28, 2024
@pq pq self-assigned this Aug 28, 2024
@kallentu
Copy link
Member

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

pq commented Aug 28, 2024

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

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

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

pq commented Aug 30, 2024

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

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

lrhn commented Sep 4, 2024

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

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

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

lrhn commented Sep 5, 2024

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.

copybara-service bot pushed a commit that referenced this issue Sep 5, 2024
…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]>
@kallentu
Copy link
Member

kallentu commented Sep 5, 2024

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

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

pq commented Sep 5, 2024

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.

@srawlins srawlins added the type-enhancement A request for a change that isn't a bug label Sep 6, 2024
@bwilkerson
Copy link
Member

It's been a long time since this issue was updated. Is there still work to be done here?

@pq pq added P2 A bug or feature request we're likely to work on and removed P1 A high priority bug; for example, a single project is unusable or has many test failures labels Nov 11, 2024
@pq
Copy link
Member Author

pq commented Nov 11, 2024

I think we still want to keep this open pending a strategy for nudging folks to use/discover wildcards, but P1 is not the right priority so I've de-escalated.

@kallentu
Copy link
Member

After talking to @pq and @bwilkerson, we've decided we're doing to do the following:

  • Evaluate catch parameter diagnostics and make sure they don't conflict with the wildcard feature.
  • Add a fix option to turn unused catch(e) -> catch(_) as an option for our users.
  • Create a new lint named unnecessary_underscores.
  • Add a fix option to turn unnecessary local function parameters to _.

@pq
Copy link
Member Author

pq commented Nov 21, 2024

[implementation note 🧐 ]: ("fix"es in 2 and 3 will technically be "assists" since they won't (yet) have associated diagnostics.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analyzer-warning Issues with the analyzer's Warning codes area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. feature-wildcard-variables Implementation of the wildcard variables feature P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

7 participants