Linter: [meta] rationalize/align/document different lint rule sets

[pq]

Rationalization

The Dart ecosystem has a number of "default" rulesets.

• the pana package uses one that should mirror what is used by stagehand but could consider another (for example, pedantic?)
• package:pedantic describes google standard defaults, which are close but not quite to a subset of those in Effective Dart
• and what's the rationale for what's defined by stagehand anyway; wouldn't it be better if it just used one of the existing rulesets (https://github.com/dart-lang/stagehand/issues/593)?
• the flutter repo uses still another and the flutter analyze tool still one more
• (finally?!?) the dartanalyzer tool uses yet another set of rules (https://github.com/dart-lang/linter/issues/1363)

Needless to say, this is VERY confusing. Ideally we could converge on one set of defaults with variants well documented.

Alignment

Concretely, I'd propose we converge on package:pedantic where possible. Specifically, I'd suggest defaults should be:

• [x] stagehand : pedantic (https://github.com/dart-lang/stagehand/issues/593)
• [x] pana : pedantic (https://github.com/dart-lang/pana/issues/459)
• [x] dartanalyzer : none (#1376, https://github.com/dart-lang/sdk/issues/35708)
• [x] flutter
  
  
  
  • [x] repo unchanged
  
  
  • [x] ~user: superset of pedantic?~ (leave informal for now)
  
  

Documentation

Having aligned as best we can, we'll want to document. Some thoughts for where:

• [x] analyzer package README (shows in pub) PR
• [x] linter
  
  
  
  • [x] README
  
  
  • [x] ~generated rule docs~
  
  
• [x] dart-lang "Customize Static Analysis" doc

/cc @bwilkerson @davidmorgan @kevmoo @mit-mit @stereotype441 @devoncarew @kwalrath

[bwilkerson]

It makes sense to me to change stagehand to build a project that imports from pedantic.

It also makes sense to me that pana would be based on pedantic, but it might be the case that it makes sense for it to be a superset. I'm not familiar with the differences between the two rule sets.

My personal preference would be for dartanalyzer to not enable _any_ lints by default. Lints were always designed to be opt-in, but enabling some by default violates that contract.

I agree that we're not going to change Flutter's lists of rules.

[mit-mit]

It seems inconsistent to me to have stagehand and flutter produce new Dart packages that are not equally pedantic.

[pq]

For context, here's a breakdown of what rules are currently defined where.

| name | flutter user | flutter repo | pedantic | stagehand |
| :--- | :---: | :---: | :---: | :--- |
| always_declare_return_types | | ✅ | | |
| always_put_control_body_on_new_line | | ✅ | | |
| always_put_required_named_parameters_first | | | | |
| always_require_non_null_named_parameters | | ✅ | | |
| always_specify_types | | ✅ | | |
| annotate_overrides | | ✅ | | |
| avoid_annotating_with_dynamic | | | | |
| avoid_as | | ✅ | | |
| avoid_bool_literals_in_conditional_expressions | | | | |
| avoid_catches_without_on_clauses | | | | |
| avoid_catching_errors | | | | |
| avoid_classes_with_only_static_members | | ✅ | | |
| avoid_double_and_int_checks | | | | |
| avoid_empty_else | ✅ | ✅ | ✅ | |
| avoid_field_initializers_in_const_classes | | ✅ | | |
| avoid_function_literals_in_foreach_calls | | ✅ | | |
| avoid_implementing_value_types | | | | |
| avoid_init_to_null | ✅ | ✅ | ✅ | |
| avoid_js_rounded_ints | | | | |
| avoid_null_checks_in_equality_operators | | ✅ | | |
| avoid_positional_boolean_parameters | | | | |
| avoid_private_typedef_functions | | | | |
| avoid_relative_lib_imports | | ✅ | ✅ | |
| avoid_renaming_method_parameters | | ✅ | | |
| avoid_return_types_on_setters | ✅ | ✅ | ✅ | |
| avoid_returning_null | | | | |
| avoid_returning_null_for_future | | | | |
| avoid_returning_null_for_void | | ✅ | | |
| avoid_returning_this | | | | |
| avoid_setters_without_getters | | | | |
| avoid_shadowing_type_parameters | | | | |
| avoid_single_cascade_in_expression_statements | | | | |
| avoid_slow_async_io | | ✅ | | |
| avoid_types_as_parameter_names | | ✅ | ✅ | |
| avoid_types_on_closure_parameters | | | | |
| avoid_unused_constructor_parameters | | ✅ | | |
| avoid_void_async | | ✅ | | |
| await_only_futures | ✅ | ✅ | | |
| camel_case_types | ✅ | ✅ | | |
| cancel_subscriptions | ✅ | ✅ | | ✅ |
| cascade_invocations | | | | |
| close_sinks | ✅ | | | |
| comment_references | | | | |
| constant_identifier_names | | | | |
| control_flow_in_finally | ✅ | ✅ | | |
| curly_braces_in_flow_control_structures | | | | |
| directives_ordering | | ✅ | | |
| empty_catches | | ✅ | | |
| empty_constructor_bodies | ✅ | ✅ | | |
| empty_statements | ✅ | ✅ | | |
| file_names | | | | |
| flutter_style_todos | | ✅ | | |
| hash_and_equals | ✅ | ✅ | | ✅ |
| implementation_imports | ✅ | ✅ | | |
| invariant_booleans | | | | |
| iterable_contains_unrelated_type | | ✅ | | ✅ |
| join_return_with_assignment | | | | |
| library_names | ✅ | ✅ | | |
| library_prefixes | | ✅ | | |
| lines_longer_than_80_chars | | | | |
| list_remove_unrelated_type | | ✅ | | ✅ |
| literal_only_boolean_expressions | | | | |
| no_adjacent_strings_in_list | | ✅ | | |
| no_duplicate_case_values | | ✅ | ✅ | |
| non_constant_identifier_names | ✅ | ✅ | | |
| null_closures | | | ✅ | |
| omit_local_variable_types | | | | |
| one_member_abstracts | | | | |
| only_throw_errors | | | | |
| overridden_fields | | ✅ | | |
| package_api_docs | ✅ | ✅ | | |
| package_names | ✅ | ✅ | | |
| package_prefixed_library_names | ✅ | ✅ | | |
| parameter_assignments | | | | |
| prefer_adjacent_string_concatenation | | ✅ | | |
| prefer_asserts_in_initializer_lists | | ✅ | | |
| prefer_bool_in_asserts | | | | |
| prefer_collection_literals | | ✅ | | |
| prefer_conditional_assignment | | ✅ | | |
| prefer_const_constructors | | ✅ | | |
| prefer_const_constructors_in_immutables | | ✅ | | |
| prefer_const_declarations | | ✅ | | |
| prefer_const_literals_to_create_immutables | | ✅ | | |
| prefer_constructors_over_static_methods | | | | |
| prefer_contains | | ✅ | ✅ | |
| prefer_equal_for_default_values | | ✅ | ✅ | |
| prefer_expression_function_bodies | | | | |
| prefer_final_fields | | ✅ | | |
| prefer_final_in_for_each | | | | |
| prefer_final_locals | | ✅ | | |
| prefer_foreach | | ✅ | | |
| prefer_function_declarations_over_variables | | | | |
| prefer_generic_function_type_aliases | | ✅ | | |
| prefer_initializing_formals | | ✅ | | |
| prefer_int_literals | | | | |
| prefer_interpolation_to_compose_strings | | | | |
| prefer_is_empty | | ✅ | ✅ | |
| prefer_is_not_empty | ✅ | ✅ | ✅ | |
| prefer_iterable_whereType | | ✅ | | |
| prefer_mixin | | | | |
| prefer_single_quotes | | ✅ | | |
| prefer_typing_uninitialized_variables | | ✅ | | |
| prefer_void_to_null | | ✅ | | |
| public_member_api_docs | | | | |
| recursive_getters | | ✅ | ✅ | |
| slash_for_doc_comments | ✅ | ✅ | | |
| sort_constructors_first | | ✅ | | |
| sort_pub_dependencies | | ✅ | | |
| sort_unnamed_constructors_first | | ✅ | | |
| super_goes_last | ✅ | ✅ | | |
| test_types_in_equals | ✅ | ✅ | | ✅ |
| throw_in_finally | ✅ | ✅ | | |
| type_annotate_public_apis | | | | |
| type_init_formals | ✅ | ✅ | | |
| unawaited_futures | | | ✅ | |
| unnecessary_await_in_return | | | | |
| unnecessary_brace_in_string_interps | ✅ | ✅ | | |
| unnecessary_const | | ✅ | | |
| unnecessary_getters_setters | ✅ | ✅ | | |
| unnecessary_lambdas | | | | |
| unnecessary_new | | ✅ | | |
| unnecessary_null_aware_assignments | | ✅ | | |
| unnecessary_null_in_if_null_operators | | ✅ | | |
| unnecessary_overrides | | ✅ | | |
| unnecessary_parenthesis | | ✅ | | |
| unnecessary_statements | ✅ | ✅ | | |
| unnecessary_this | | ✅ | | |
| unrelated_type_equality_checks | ✅ | ✅ | ✅ | ✅ |
| use_function_type_syntax_for_parameters | | | | |
| use_rethrow_when_possible | | ✅ | ✅ | |
| use_setters_to_change_properties | | | | |
| use_string_buffers | | | | |
| use_to_and_as_if_applicable | | | | |
| valid_regexps | ✅ | ✅ | ✅ | ✅ |
| void_checks | | | | |

[bwilkerson]

Very nice way of comparing, thanks. I kind of wish it included 'pana', though.

So, 'pedantic' defines 16 rules and 'flutter user' defines 28, but only 10 of those are in the intersection. Similarly, stagehand defines 7 rules and only 2 are in the intersection with 'pedantic'.

The rule sets are farther apart than I had guessed.

[mit-mit]

pana uses stagehand for Dart code, and analysis_options_user.yaml for Flutter code.

[kwalrath]

I'm fine with using pedantic for stagehand templates.

[pq]

I'm fine with using pedantic for stagehand templates.

💎

Assuming we can find consensus, https://github.com/dart-lang/stagehand/pull/594 updates stagehand.

If that sticks, we can update pana to follow suit.

[kwalrath]

I don't see package_names under https://github.com/dart-lang/linter/tree/master/lib/src/rules.

[pq]

package_names lives in https://github.com/dart-lang/linter/tree/master/lib/src/rules/pub. (Or were you asking something else?)

While we're at it, you helped me notice a bug in my table generator... Notice the sorting?

package_names

The package_names lint is in a file called PubPackageNames and it looks like I sorted on filename and not rule name.

Thanks for the catch!

(EDIT: order fixed 👍)

[mit-mit]

I don't think we should update Stagehand before we resolve the larger issue: Customers increasingly use Dart across multiple platforms and frameworks, and will no doubt be very confused if they try to move a few lines of code from one context to another and suddenly see different static errors.

I suggest we collaborate with the Flutter team on getting to a single set of default rules for Dart libraries and apps, regardless of framework or platform.

[pq]

@mit-mit: here's a more targeted view for the purposes of that conversation. (cc @Hixie FYI.)

| name | pedantic | stagehand | flutter user |
| :--- | :---: | :---: | :---: |
| avoid_empty_else | ✅ | | ✅ |
| avoid_init_to_null | ✅ | | ✅ |
| avoid_relative_lib_imports | ✅ | | |
| avoid_return_types_on_setters | ✅ | | ✅ |
| avoid_types_as_parameter_names | ✅ | | |
| await_only_futures | | | ✅ |
| camel_case_types | | | ✅ |
| cancel_subscriptions | | ✅ | ✅ |
| close_sinks | | | ✅ |
| control_flow_in_finally | | | ✅ |
| empty_constructor_bodies | | | ✅ |
| empty_statements | | | ✅ |
| hash_and_equals | | ✅ | ✅ |
| implementation_imports | | | ✅ |
| iterable_contains_unrelated_type | | ✅ | |
| library_names | | | ✅ |
| list_remove_unrelated_type | | ✅ | |
| no_duplicate_case_values | ✅ | | |
| non_constant_identifier_names | | | ✅ |
| null_closures | ✅ | | |
| package_api_docs | | | ✅ |
| package_names | | | ✅ |
| package_prefixed_library_names | | | ✅ |
| prefer_contains | ✅ | | |
| prefer_equal_for_default_values | ✅ | | |
| prefer_is_empty | ✅ | | |
| prefer_is_not_empty | ✅ | | ✅ |
| recursive_getters | ✅ | | |
| slash_for_doc_comments | | | ✅ |
| super_goes_last | | | ✅ |
| test_types_in_equals | | ✅ | ✅ |
| throw_in_finally | | | ✅ |
| type_init_formals | | | ✅ |
| unawaited_futures | ✅ | | |
| unnecessary_brace_in_string_interps | | | ✅ |
| unnecessary_getters_setters | | | ✅ |
| unnecessary_statements | | | ✅ |
| unrelated_type_equality_checks | ✅ | ✅ | ✅ |
| use_rethrow_when_possible | ✅ | | |
| valid_regexps | ✅ | ✅ | ✅ |

_(95 lints w/o rulesets not shown)_

[pq]

FYI @a14n

[a14n]

I suggest we collaborate with the Flutter team on getting to a single set of default rules for Dart libraries and apps, regardless of framework or platform.

Pedantic enables null_closures but this couldn't be used in flutter. Some widgets like RaisedButton use null as onPressed callback to disable the button.

[davidmorgan]

null_closures only applies to specific known methods.

http://dart-lang.github.io/linter/lints/null_closures.html

The pedantic ruleset is the one used internally at google--including for flutter apps--so we have good evidence that it works :)

[a14n]

Thanks @davidmorgan ! Sorry for the noise.

[bwilkerson]

I suggest we collaborate with the Flutter team on getting to a single set of default rules for Dart libraries and apps, regardless of framework or platform.

@mit-mit I'm concerned by this comment, but perhaps I'm misunderstanding what you're saying.

While I completely agree that there are some conditions that are not included in the specification that should be reported to all users (which for historic reasons we call "hints"), I believe that there is room for allowing users to choose to have some other conditions reported based on their personal preference (which we, and the larger industry, call "lints").

But this makes it sound like you're proposing that there should be a single list of lints that should be recommended to all users. If we accept the preference that it's OK to disagree about which lints to enable, then I don't see why it would be bad for us to have multiple example rule sets tailored to some common sets of preferences.

We don't need to internally enforce all of the rules that Flutter recommends, nor do we need to limit Flutter users to only those rules we use internally.

Customers ... will no doubt be very confused if they try to move a few lines of code from one context to another and suddenly see different static errors.

(Minor nit: they aren't static errors, they're lints, which typically _are_ represented differently to the user.)

Nits aside, I don't think that's true at all. If I copy some code from an open source package and paste it into mine, and I have enabled a lint to help enforce a preferred style that the other package didn't enforce, and if I then get lints suggesting that I change the code, I think I'll understand exactly why I got that lint: because the original authors didn't enforce the same style that I do.

[devoncarew]

on getting to a single set of default rules for Dart libraries and apps, regardless of framework or platform

Agreement here may be challenging; I think just reducing the number of disparate rules sets would be a worthwhile improvement.

[pq]

Agreement here may be challenging; I think just reducing the number of disparate rules sets would be a worthwhile improvement.

I think a good start would be agreeing on pedantic as a shared kernel¹ and allowing for Flutter-specific projects to add more stringent rules on top. If we did that, the Flutter user rules, could be updated to look something like:

# defines base lint rules
include: package:pedantic/analysis_options.yaml

analyzer:
  errors:
    # treat missing required parameters as a warning (not a hint)
    missing_required_param: warning

# additional Flutter-recommended rules
linter:
  rules:
    - await_only_futures
    - camel_case_types
    etc...

¹ Since package:pedantic rules are enforced for internal projects they seems like a reasonable baseline IMO.

📟 paging @Hixie for a gut check...

[Hixie]

I expect we (Flutter) will want to control our rule set explicitly (we toggle rules on and off multiple times a month), and it's really valuable for us to have the complete list on our analysis options with explicit comments next to each one that we don't turn on explaining why, so it's not clear to me that this would be solving a problem that we have.

As a user (and not with my Flutter hat on), I'd love to have a way to turn on all possible lints, and require that I opt-out of the ones I don't want. This is because I frequently find that some super-useful lint has existed for months without my knowing.

[pq]

Thanks @Hixie. Super helpful.

I expect we (Flutter) will want to control our rule set explicitly (we toggle rules on and off multiple times a month), and it's really valuable for us to have the complete list on our analysis options with explicit comments next to each one that we don't turn on explaining why, so it's not clear to me that this would be solving a problem that we have.

I should be clear that the problem this general issue is trying to address is less about framework _authors_ and more about users who are understandably bewildered. I think the alignment around pedantic should help a lot with that confusion. It would be ideal I think if the Flutter user rules were a more explicit superset of package:pedantic (e.g., starting with an include and adding). Would you be amenable to updating the "user" rule template that way or should we just leave it well enough alone?

Your ask for a better awareness around new lints deserves it's own topic so I started one in https://github.com/dart-lang/linter/issues/1390 👍.

Cheers all for the thoughtful feedback!

[pq]

I added a few ideas for documentation in the main issue description but ideas are most welcome.

@kwalrath: any thoughts here?

Is there a single doc (existing, repurposed or new) that the various tools could point back to?

[kwalrath]

I'm in the midst of updating Customize Static Analysis to talk about includes and pedantic. It wouldn't have a complete list of lints, though.

What would you like to have on the page you point to?

[pq]

That's awesome @kwalrath!

What would you like to have on the page you point to?

Maybe just what you're doing already? Besides the mechanics of including options, something that establishes that there is a kernel set of rules that is shared by a number of our tools would be great.

Thanks!

[Hixie]

I don't have a good sense of what our users want in this space. @InMatrix might.

[pq]

Thanks for the thoughtful back and forth all. With @kwalrath's docs and the most recent updates to pana I think we can mark this is done.

Cheers! 🍻