chore: Review and Update Analysis Rules

[tomarra]

Is your feature request related to a problem? Please describe.
We have a couple of tickets around adding new analysis rules in. We need to actually inventory the current rule set, when it was last reviewed, and what is new since then to determine what we want to actually add in.

Describe the solution you'd like

• We have a list of all the available analysis rules (Either in the readme or some other documentation file) and a checkmark or "X" to denote if they are enabled or disabled.
• Disabled rules should have some kind of description as to why. Given historical context on some of them this may be tough so need to do our best on this.
• Rules that have been introduced since the last update are reviewed and the team determines if they should be enabled or not.
• The rules that are deemed as accepted are added into the tool

Describe alternatives you've considered
None

Additional context
Other tickets asking for new rules

• Exit code on lint failures dart-lang/sdk#57217
• Error group. dart-lang/linter#90
• Speed up tests. dart-lang/sdk#57208

Linter rules Documentation - https://dart.dev/tools/linter-rules
Also seems like the "added in" documentation may be moved into each lint documentation page itself rather then an overview page.

[tomarra]

Looks like we need to be evaluating the rules that were added from Aug 30th 2023 and forward. This lines up with the v5.1.0 release.

[alestiago]

The following are all the rules that are disabled by default by Very Good Analysis, according to the ticket we should try and provide a reason for each one of them:

Rule	Reason
always_put_control_body_on_new_line	Can conflict with the Dart formatter
always_specify_types	Incompatible with omit_local_variable_types
annotate_redeclares	Experimental
avoid_annotating_with_dynamic	Not specified
avoid_catches_without_on_clauses	Has unresolved false positives
avoid_classes_with_only_static_members	Not specified
avoid_implementing_value_types	Not specified
avoid_types_on_closure_parameters	Not specified
close_sinks	Has unresolved false positives
deprecated_member_use_from_same_package	Not specified
diagnostic_describe_all_properties	Not specified
discarded_futures	Has unresolved false positives
do_not_use_environment	Not specified
matching_super_parameters	Not specified
missing_code_block_language_in_doc_comment	Not specified
no_literal_bool_comparisons	Not specified
no_self_assignments	Not specified
no_wildcard_variable_uses	Not specified
prefer_double_quotes	Incompatible with prefer_single_quotes
prefer_expression_function_bodies	Not specified
prefer_final_parameters	Not specified
prefer_foreach	Not specified
prefer_mixin	Not specified
prefer_relative_imports	Incompatible with always_use_package_imports
type_literal_in_constant_pattern	Not specified
unnecessary_final	Not specified
unnecessary_library_name	Not specified
unnecessary_null_aware_operator_on_extension_on_nullable	Not specified
unreachable_from_main	Not specified
unsafe_html	Not specified
use_decorated_box	Has unresolved malfunctions

[alestiago]

These are my views about the new rules introduced in Dart 3.4:

• annotate_redeclares: It is currently marked as experimental, I think we should revisit it once it is no longer experimental.
• missing_code_block_language_in_doc_comment: I think we should add it in our next release.
  • It will enable proper syntax highlighting of Markdown code blocks, dart doc strongly recommends code blocks to specify the language used after the initial code fence.
  • Adding "none" as a language for those code blocks that don't have a defined language will suppress the warning.
• unnecessary_library_name: I think we should add it in our next release.
  • Catches when a library does not need a library declaration, but one can be added to attach library documentation and library metadata to. A declaration of library; is sufficient for those uses.
  • Has a quick fix
  • Might need to consider removing library_names and package_prefixed_library_names.

---

Regarding #90, I do think we should include no_wildcard_variable_uses as soon as possible. It is not only marked as recommended but there is a warning specifying that the current usage will be breaking:

Wildcard parameters and local variables (e.g. underscore-only names like _, __, ___, etc.) will become non-binding in a future version of the Dart language. Any existing code that uses wildcard parameters or variables will break. In anticipation of this change, and to make adoption easier, this lint disallows wildcard and variable parameter uses.

---

It was also suggested to add no_self_assignments, it was added in Dart 3.1.0, I haven't found issues related to it reporting false positives. I think it will also be a great addition.

[jsgalarraga]

Here's my opinion on some rules that we can add:

• avoid_catches_without_on_clauses: As per the documentation of the rule itself: "Using catch clauses without on clauses make your code prone to encountering unexpected errors that won't be thrown (and thus will go unnoticed)." Therefore adding this rule will help our error handling be more meaningful.
• no_literal_bool_comparisons: As per Effective Dart, don't use true or false in equality operations. Has a quick fix.

I also agree with the reasons provided by @alestiago to add missing_code_block_language_in_doc_comment, no_wildcard_variable_uses and no_self_assignments.

---

To try and help fill the table about the reasons why are not using some rules, I can add the following:

• always_put_control_body_on_new_line: Can conflict with the Dart formatter.
• always_specify_types: Incompatible with omit_local_variable_types
• avoid_classes_with_only_static_members: As per Effective Dart, there are natural use cases such as grouping constants in a class.
• do_not_use_environment: We intentionally use environment variables to retrieve keys and other sensitive data.

[alestiago]

Here's my opinion on some rules that we can add.

@jsgalarraga regarding the two suggested rules you would like to get included, have you found any issues about them not performing as expected? I think they are useful, but they don't seem to be tagged as "recommened", "style:fluter" or "style:core", do you know why?

To try and help fill the table about the reasons why are not using some rules, I can add the following:

I've added some of the given reasons to the table, thank you so much!

do_not_use_environment: We intentionally use environment variables to retrieve keys and other sensitive data.

Regarding do_not_use_environment I've tried gathering more information before we have a solid reason. So, the rule doesn't catch Platform.environment but does catch, for example, String.fromEnvironment. Abstaining (current) from including it means there is no clear preference to use Platform.environment over .fromEnvironment. Whereas enabling it would state a clear preference for Platform.environment.

However, I'm currently unsure about the scenarios where String.fromEnvironment is preferred over Platform.environment and viceversa. There are issues from the Dart & Flutter team where themselves find the distinction between them confusing:

• Consider adding unnecessary_library_name to recommended dart-lang/core#823
• Dart.next: <Primitive>.fromEnvironment is a total misnomer dart-lang/sdk#27585

There is also a blog "Configuring apps with compilation environment declarations" about such environment variables, but it mainly explains .fromEnvironment with no reference to Platform.environment.

If we look at the original proposal of the lint rule, dart-lang/sdk#58165 a Dart team member (rakudrama) commented:

I have to say I don't really understand the motivation. The environment is baked into the app by the compiler. You can't do anything in the app to change the result, so it is more lake a configuration rather than (mutable) state.

One big advantage of, e.g. const bool.fromEnvironment is that since it is baked into the app by the compiler, the app can be optimized against the configuration, for example, tree-shaking all the dependent code.

This is not true of an alternative like a command-line option, which must not be optimized away in case the app is passed a command line with the option. Avoiding the 'environment' will always result in larger compiled apps.

To which I tend to incline towards due to the tree-shaking benefit.

In occasions, Platform.environment is preferred over .fromEnvironment, as in Dart Frog.

Knowing the above, regarding "We intentionally use environment variables to retrieve keys and other sensitive data." do you do so with Platform.environment or String.fromEnvironment and if so, why one and not the other?

P.S. I'm still evaluating the avoid_classes_with_only_static_members reason.

---

After reading through dart-lang/core#823 I'm a bit skeptical on adding unnecessary_library_name, it seems that according to goderbauer comment:

It is not clear to me why dartdoc behaves differently based on the presence of a library names.

It is unclear to me the where the current progress is on solving such issue. We could go forward with it and simply recommend users to rename their files or, if previously pointing to a path that no longer exists, updating such path. At the end of the day, the rule does align with Effective Dart. On the other hand, it might be worth waiting a bit too see where dart-lang/core#823 ends up.

[jsgalarraga]

@jsgalarraga regarding the two suggested rules you would like to get included, have you found any issues about them not performing as expected? I think they are useful, but they don't seem to be tagged as "recommened", "style:fluter" or "style:core", do you know why?

I've found this issue regarding the avoid_catches_without_on_clauses which had some false positives and apparently is now getting fixed in dart 3.5.0. I assume this is the reason why is not tagged as "recommended", but I agree with the reasons to add it mentioned here. Maybe we can track this and wait until the rule is more robust.

Regarding no_literal_bool_comparisons, haven't found any issues and no idea why is not "recommended" since Effective Dart has that use case outlined 🤔 .

About do_not_use_environment, I didn't know that the rule allowed the Platform.environment, which is confusing to me. I honestly don't understand the rule motivation, since I don't agree with this statement on the rule:

creates hidden global state and makes applications hard to understand and maintain.

Therefore I agree with the comment of the dart team member you shared.
In any case, I would not add this one to very good analysis.

Regarding unnecessary_library_name, 100% agree with you, with that issue tracking if it's going to be added to "recommended", I would follow closely and update very good analysis when that issue is solved.

[lishaduck]

close_sinks is because of dart-lang/sdk#57882 (felangel/bloc#1233 (comment)).

[marwfair]

Regarding do_not_use_environment, I find it confusing. One reason to prefer String.fromEnvironment over Platform.environment is that Platform.environment depends on the dart:io package, which doesn't work on Flutter web.

The reason to choose one over the other is whether you are passing the environment variable as a --dart-define, which is set at compile time, or reading the variable from the operating system environment variables during runtime.

For example, if I run this command to run my app:
MY_PLATFORM_VAR=platform_var flutter run --dart-define MY_STRING_VARIABLE=string_var

  print(Platform.environment['MY_PLATFORM_VAR']);   // platform_var
  print(const String.fromEnvironment('MY_STRING_VARIABLE'));   // string_var

I think that both of these serve a purpose and have use cases.

[alestiago]

Thanks @jsgalarraga and @lishaduck I've updated the table and have a matching Pull Request #102 that updates the README table. I've added both of you as co-authors of the reasons you've found so that you are counted as contributors.

[alestiago]

Closing since v6.0.0 has been released. As per usual we're always open to reviewing some rules, if so, don't hesitate on telling us by opening an issue.