what lints to enable in the linter codebase

[a14n]

There are the following 45 lints that are not enabled in analysis_options.yaml.

#    - always_declare_return_types
#    - always_put_control_body_on_new_line
#    - always_specify_types
#    - avoid_as
#    - avoid_bool_literals_in_conditional_expressions # not yet tested
#    - avoid_classes_with_only_static_members
#    - avoid_double_and_int_checks # not yet tested
#    - avoid_field_initializers_in_const_classes # not yet tested
#    - avoid_function_literals_in_foreach_calls
#    - avoid_js_rounded_ints # not yet tested
#    - avoid_private_typedef_functions # not yet tested
#    - avoid_relative_lib_imports # not yet tested
#    - avoid_renaming_method_parameters # not yet tested
#    - avoid_single_cascade_in_expression_statements # not yet tested
#    - avoid_types_as_parameter_names # not yet tested
#    - avoid_types_on_closure_parameters
#    - avoid_unused_constructor_parameters # not yet tested
#    - cascade_invocations
#    - close_sinks # https://github.com/dart-lang/linter/issues/268
#    - constant_identifier_names
#    - curly_braces_in_flow_control_structures # not yet tested
#    - file_names # not yet tested
#    - lines_longer_than_80_chars # not yet tested
#    - non_constant_identifier_names
#    - null_closures # not yet tested
#    - omit_local_variable_types
#    - one_member_abstracts
#    - prefer_bool_in_asserts # not yet tested
#    - prefer_const_constructors
#    - prefer_const_declarations # not yet tested
#    - prefer_const_literals_to_create_immutables # not yet tested
#    - prefer_final_locals
#    - prefer_function_declarations_over_variables
#    - prefer_generic_function_type_aliases # not yet tested
#    - prefer_interpolation_to_compose_strings
#    - prefer_iterable_whereType # not yet tested
#    - prefer_typing_uninitialized_variables # not yet tested
#    - public_member_api_docs
#    - sort_constructors_first
#    - sort_unnamed_constructors_first
#    - super_goes_last
#    - type_annotate_public_apis
#    - unnecessary_parenthesis # not yet tested
#    - unnecessary_statements # not yet tested
#    - void_checks # not yet tested

Some are not compatible between each other. But a lot of them could be enable to improve the code (and dogfooding our lints).

Those that are commented out should have an explanation to know why.

[bwilkerson]

Thanks for opening this issue! I think it's a very good topic for discussion, and this is a good place to discuss it. What follows is my personal opinion, and not necessarily that of the Dart team (or anyone else). It's intended to promote discussion, not to shut it down, so I hope it comes across correctly.

Those that are commented out should have an explanation to know why.

I actually think that's backward. I do not believe that any project should enable a lint because there isn't a stated reason not to. I think, rather, that a project should only enable those lints for which there is a stated reason for it to be enabled.

And I don't believe that the linter package should be required to dogfood lints. It should be managed like any other package and only enable those rules that bring value to the developers. (Where "value" and "developers" are intentionally poorly defined terms.)

I do not object to enabling additional lints, but I think we need to carefully consider the value that each one brings and only enable the ones that unequivocally add value. (And I think we should periodically re-evaluate previously enabled lints to see whether they still meet that criteria.)

[pq]

Echoing @bwilkerson I think we should be thoughtful about what value is being added by any proposed new analyses, being mindful of the vagueness of "value" too.

Moving forward, I'd suggest maybe we take on proposals one at a time and weigh merits and implications in a dedicated tracking issue. (I'm also of the opinion that we might consider disabling some too and in that case we might follow the same process.)

[matanlurey]

Big 👍to @bwilkerson's answer.

The reason there is a linter is to lint additional static analysis issues that aren't part of the language, meaning that any enabled lint on any project should add value (and be worth the overhead of the lint - including analysis/perf time, complying with the lint, possible false positives, etc).

@pq:

Moving forward, I'd suggest maybe we take on proposals one at a time and weigh merits and implications in a dedicated tracking issue. (I'm also of the opinion that we might consider disabling some too and in that case we might follow the same process.)

One of the reasons I hope to see a "Dart Team Official" style guide lint set (/cc @davidmorgan) is to also squash some of these external discussions into a single discussion on what we want for our own dart-lang/* repositories. Ideally most or all of our external packages would have the same set of lints, so that you don't have to constantly change styles (i.e. prefer_final) when you are working in different packages.

(I realize there are some packages where having slightly different configuration might be valuable, but those should be the exceptions not the rules, ideally)

[a14n]

Those that are commented out should have an explanation to know why.

I actually think that's backward. I do not believe that any project should enable a lint because there isn't a stated reason not to. I think, rather, that a project should only enable those lints for which there is a stated reason for it to be enabled.

I meant that there should be an explanation to know why it is commented out. In the current analysis_options.yaml there are lines commented out but you don't know if it's because of an issue in the lint or if it haven't been yet evaluated or if we simply don't want it (a simple # we don't want it ... is fine).

And I don't believe that the linter package should be required to dogfood lints. It should be managed like any other package and only enable those rules that bring value to the developers. (Where "value" and "developers" are intentionally poorly defined terms.)

I do not object to enabling additional lints, but I think we need to carefully consider the value that each one brings and only enable the ones that unequivocally add value. (And I think we should periodically re-evaluate previously enabled lints to see whether they still meet that criteria.)

👍

One of the reasons I hope to see a "Dart Team Official" style guide lint set

👍 the lints enforcing Dart Style Guide should all be enabled.

[bwilkerson]

the lints enforcing Dart Style Guide should all be enabled.

Maybe the lints enforcing required guidelines (though even there it isn't clear that all of them add value), but not all of the lints enforcing optional guidelines unless we all agree that there's sufficient value to warrant the overhead.

[pq]

not all of the lints enforcing optional guidelines unless we all agree that there's sufficient value to warrant the overhead.

Emphatic +1.

Just because there are knobs doesn't mean we should turn them all up to 11.

🤘

[kasperpeulen]

From a “outside of google” developer perspective, that is trying to get productive in Dart, the linter is useful, but it is also 100 extra decision a developer has to make.

It would save a lot of peoples time, if the Dart team had some guidelines, in the sense of, “You should definitely enable the following lints”, and “You may also want to enable the following lints”.

[pq]

@kasperpeulen : you make a very good point and we've discussed it a lot. (See #57364 for example.) . Needless to say, I agree!

[davidmorgan]

I think the major reason this discussion keeps cropping up is that we don't actually have an agreed-upon set of lints internally at google. We're gradually building such a set--it takes discussion and experimentation for each one.

I'm not sure how useful a work in progress is to the open source world. Internally, the people adding a lint are responsible for fixing it everywhere--but externally, each individual project owner will have to make fixes whenever we push a change.

[zoechi]

This would work fine if the rulesets would be versioned, then it could be managed by developers if they want to apply the latest changes.

[davidmorgan]

True.

A secondary issue is that the set that makes sense for external projects may be different. For example, we decided not to enforce empty_statements because we already enforce use of dartfmt, so empty statements already stand out. I guess we can publish the reasoning, too, and people can go from there.

FWIW here's the current list of enforced lints

- avoid_empty_else
- avoid_return_types_on_setters
- avoid_types_as_parameter_names
- control_flow_in_finally
- no_duplicate_case_values
- prefer_contains
- prefer_equal_for_default_values
- throw_in_finally
- unrelated_type_equality_checks
- use_rethrow_when_possible
- valid_regexps

--yes, it's pretty short--work in progress!

[zoechi]

... or instead of versioning probably better #57730

[parlough]

There’s been a lot of varied discussion in this issue, and most has been resolved or is being discussed elsewhere.

• Lint set used by the linter package itself (define a set of analysis options shared by analyzer packages #53165)
• Recommended lint sets for users (https://pub.dev/packages/lints)
• Lint set for the Dart team and its packages (https://github.com/dart-lang/ecosystem/tree/main/pkgs/dart_flutter_team_lints)

Thanks all!