[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 (consider updating template to use package:pedantic lints dart-archive/stagehand#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 (What are the default lints enforced? #57872)

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:

• stagehand : pedantic (consider updating template to use package:pedantic lints dart-archive/stagehand#593)
• pana : pedantic (Consider using pedantic as a baseline for linting? pana#459)
• dartanalyzer : none (NPE given invalid import #1376, Remove lint rule registry default list #35708)
• flutter
  • repo unchanged
  • user: superset of pedantic? (leave informal for now)

Documentation

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

• analyzer package README (shows in pub) PR
• linter
  • README
  • generated rule docs
• 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, dart-archive/stagehand#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?

public_member_api_docs
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 #57885 👍.

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! 🍻