[Wildcard Variables] support for `non_constant_identifier_names`
pq opened this issue · 16 comments
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:
- catch clauses and
- formal params
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 #1854.)
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.
Fixed w/ dart-lang/sdk@cc63001
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
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
2. Make a new lint with messaging that directs users to use _ when they try and use __, with perhaps a quickfix.
3. Do the migration using that new lint.
Thoughts?
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:
- introduce a new lint where we can do whatever we want, or
- 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
So 1. revert this lint.
Do you mean just making it less opinionated about __s?
Do you mean just making it less opinionated about
__s?
Yes, sorry. I meant making it less opinionated about __s temporarily.
Current plan:
- update the lint to not fire on
__, etc. - land the bit flip
- update the lint to fire again where we want it
- fix breakages in flutter and google3
(3 and 4 can happen in parallel.)
... 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.
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.
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.
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: #5040
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.
A few arguments for adding a new lint instead of using either of those two:
- Neither of those lints are currently in g3 right now, meaning that the migration for these breaks once we turn on the wildcards flag is going to be a lot more annoying with a bunch of false positives, and we wouldn't even be able to turn the lint on to enforce the behaviour we want with multiple underscores. cc. @davidmorgan @stereotype441
- (Phil's thoughts that I'm paraphrasing) The error message for these two lints aren't exactly the right UX if we want to introduce someone to using wildcards. If we had a lint that was specifically for turning multiple underscores into wildcards, we could link them to the docs and provide a less confusing error code.