search

dart-lang / linter

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

[Wildcard Variables] support for `non_constant_identifier_names` #5048

Closed pq closed 1 week ago

pq commented 1 month ago

We're currently special-casing __, ___, etc. in a number of places that we should update with the availability of wildcards.

Specifically we should now flag multiple underscores in:

Exception. We also special case constructor declarations but I think that should not change and we should continue to all A.__(), A.___(), etc. (See discussion in https://github.com/dart-lang/linter/issues/1854.)

/cc @kallentu @lrhn @bwilkerson

lrhn commented 1 month ago

SGTM. Treat __ as a private name in every way, which means warning in places where we warn about private names. Generally just don't special-case __ etc. in any way. They were special-cased as a way to have more than one way to say "ignore this value", and now you can use _ more than once.

That wouldn't applyt to named constructor names, which can be private, so A.__() should be safe.

A.____() is a sure sign of an insane mind, on the other hand.

pq commented 1 month ago

Fixed w/ https://github.com/dart-lang/sdk/commit/cc63001515e81faf1355214e738ef7b8d071fd82

kallentu commented 3 weeks ago

While making a flag flip CL, I'm coming across this lint a lot. This lint doesn't seem like the right wording + fix. I'm pretty sure all these instances are formal parameters using _, __, for which the fix is to turn them both into wildcards (once wildcards are enabled).

   info - lib/src/engine/initialization.dart:142:62 - The variable name '__' isn't a lowerCamelCase identifier. Try changing the name to follow the lowerCamelCase style. - non_constant_identifier_names
   info - test/engine/composition_test.dart:49:74 - The variable name '__' isn't a lowerCamelCase identifier. Try changing the name to follow the lowerCamelCase style. - non_constant_identifier_names
   info - test/engine/semantics/text_field_test.dart:170:23 - The variable name '__' isn't a lowerCamelCase identifier. Try changing the name to follow the lowerCamelCase style. - non_constant_identifier_names
   info - test/engine/semantics/text_field_test.dart:243:23 - The variable name '__' isn't a lowerCamelCase identifier. Try changing the name to follow the lowerCamelCase style. - non_constant_identifier_names
   info - test/engine/semantics/text_field_test.dart:274:23 - The variable name '__' isn't a lowerCamelCase identifier. Try changing the name to follow the lowerCamelCase style. - non_constant_identifier_names
   info - test/engine/semantics/text_field_test.dart:319:23 - The variable name '__' isn't a lowerCamelCase identifier. Try changing the name to follow the lowerCamelCase style. - non_constant_identifier_names
   info - test/engine/semantics/text_field_test.dart:343:23 - The variable name '__' isn't a lowerCamelCase identifier. Try changing the name to follow the lowerCamelCase style. - non_constant_identifier_names
   info - test/engine/semantics/text_field_test.dart:377:23 - The variable name '__' isn't a lowerCamelCase identifier. Try changing the name to follow the lowerCamelCase style. - non_constant_identifier_names
   info - test/engine/semantics/text_field_test.dart:419:23 - The variable name '__' isn't a lowerCamelCase identifier. Try changing the name to follow the lowerCamelCase style. - non_constant_identifier_names
   info - test/engine/semantics/text_field_test.dart:439:23 - The variable name '__' isn't a lowerCamelCase identifier. Try changing the name to follow the lowerCamelCase style. - non_constant_identifier_names
   info - test/engine/semantics/text_field_test.dart:457:23 - The variable name '__' isn't a lowerCamelCase identifier. Try changing the name to follow the lowerCamelCase style. - non_constant_identifier_names
   info - test/engine/semantics/text_field_test.dart:478:23 - The variable name '__' isn't a lowerCamelCase identifier. Try changing the name to follow the lowerCamelCase style. - non_constant_identifier_names
   info - test/engine/semantics/text_field_test.dart:544:23 - The variable name '__' isn't a lowerCamelCase identifier. Try changing the name to follow the lowerCamelCase style. - non_constant_identifier_names
   info - test/engine/window_test.dart:265:41 - The variable name '__' isn't a lowerCamelCase identifier. Try changing the name to follow the lowerCamelCase style. - non_constant_identifier_names
   info - test/engine/window_test.dart:265:71 - The variable name '___' isn't a lowerCamelCase identifier. Try changing the name to follow the lowerCamelCase style. - non_constant_identifier_names
kallentu commented 3 weeks ago

And then following up on the above, this is also blocking the flag flip roll out, because we're in a catch 22 with flipping the flag and flutter/engine linting on the flag flip pre-submits.

Knowing that this lint message isn't exactly what we wanted, could we possibly make a new lint with an diagnostic message like "Don't use multiple underscores, use one underscore to get a wildcard variable" ? We revert this change, so maybe don't lint on __ as well as _ (the original behaviour), and then just have a lint that directly tells users to use _ over __.

So 1. revert this lint. 1a. We can then go through with the flag flip

  1. Make a new lint with messaging that directs users to use _ when they try and use __, with perhaps a quickfix.
  2. Do the migration using that new lint.

Thoughts?

pq commented 3 weeks ago

Right yeah, this is what we agreed on at the time but we absolutely can do better.

I see a few choices. The best seem to be:

  1. introduce a new lint where we can do whatever we want, or
  2. continue to use this lint and add a new lint code that has a more targeted message

The costs of adding a new lint make me inclined towards the second option.

As for the fix, we currently have a RENAME_TO_CAMEL_CASE fix associated which has some smarts to convert to camel case. I'd be up for adding logic to suggest a wildcard where multiple underscores trigger the lint (and the wild card feature is enabled).

Thoughts?

Input welcome on error messaging too. (That's often the toughest part!)

/fyi @bwilkerson

pq commented 3 weeks ago

So 1. revert this lint.

Do you mean just making it less opinionated about __s?

kallentu commented 3 weeks ago

Do you mean just making it less opinionated about __s?

Yes, sorry. I meant making it less opinionated about __s temporarily.

pq commented 3 weeks ago

Current plan:

  1. update the lint to not fire on __, etc.
  2. land the bit flip
  3. update the lint to fire again where we want it
  4. fix breakages in flutter and google3

(3 and 4 can happen in parallel.)

bwilkerson commented 3 weeks ago

... this is also blocking the flag flip roll out ...

I don't know what our release process is suppose to look like, but it's my understanding that once we flip the flag (to make the feature enabled by default) there's no going back, and the feature will ship in the next dev and stable builds.

I'd like to propose that if existing lints are currently broken, or if there are lints we want to have available when the feature ships, then we aren't ready to flip the flag.

In addition, the analyzer and analysis server work doesn't appear to be complete, based on the tracking issues for them. It feels like we're being a bit hasty to be flipping the flag at this point.

And yes, we should absolutely have a fix in place to convert multi-underscore names to a single underscore where doing so is allowed, and that fix needs to be usable by dart fix. And it should also be done before we flip the flag.

pq commented 3 weeks ago

Great points Brian! We should definitely discuss.

One quick thought.

I'd like to propose that if existing lints are currently broken

I agree. In this case, I wouldn't say non_constant_identifier_names is strictly broken, it's jut not as opinionated as it could be. The trick is to make it that opinionated would break rolls and because the fix requires the feature to be enabled, we can't land fixes proactively. If it were easier to update google3 and flutter atomically with an SDK commit this wouldn't be an issue but that's a long-standing issue.

pq commented 2 weeks ago

For the underscore case, we're currently producing a message that reads like:

The variable name '__' isn't a lowerCamelCase identifier. Try changing the name to follow the lowerCamelCase style.

Would something like this be too verbose?

The variable name '__' isn't a wildcard or lowerCamelCase identifier. If its meant to be a wildcard, try changing the name to '_' or if not, follow the lowerCamelCase style.

Note that the lint currently does not try and establish if __ is used or not. We might also consider adding that (body.isPotentiallyMutatedInScope?) and then we could better target the message:

The variable name '__' isn't a wildcard. Try changing the name to '_'.

(If unused.)

And maybe just fall back to the existing message if it is:

The variable name '__' isn't a lowerCamelCase identifier. Try changing the name to follow the lowerCamelCase style.

@bwilkerson, @kallentu?

pq commented 2 weeks ago

Another wrinkle: there's potential overlap/interference w/ no_leading_underscores_for_local_identifiers which is also opinionated about _s.

The more I think about it, maybe nudging folks to wildcards is better done there?

See: https://github.com/dart-lang/linter/issues/5040

bwilkerson commented 2 weeks ago

The non_constant_identifier_names lint is in core while no_leading_underscores_for_local_identifiers is in recommended, so theoretically the first would be enabled more often than the second, though the difference is probably fairly small.

But yes, I think that being a local variable is a better signal than being a constant. I would think we'd want to flag non-constant local variables in the same way, but we wouldn't want to flag non-local constants. I agree that no_leading_underscores_for_local_identifiers is a better place for this check.

kallentu commented 2 weeks ago

A few arguments for adding a new lint instead of using either of those two:

pq commented 2 weeks ago

Yes, thanks, Kallen! #5077 proposes a new lint. Feedback welcome!

pq commented 1 week ago

As of https://github.com/dart-lang/sdk/commit/b2bdedfd4f4c58569be0bcf2bfb907ab0099a989, non_constant_identifer_names is back to being un-opinionated about underscores with the rationale being we want to do any (future) nudging from different lints/analyses.