dart-lang / linter

Linter for Dart.
https://dart.dev/tools/linter-rules
BSD 3-Clause "New" or "Revised" License
627 stars 172 forks source link

Lint docs are used as source of truth-->make them more reliable #2103

Open davidmorgan opened 4 years ago

davidmorgan commented 4 years ago

It turns out that lint docs are a great way to settle an argument. On numerous occasions I've seen people linking to lint docs as if they are absolute truth; and I've done it myself.

Unfortunately, some lint docs are not correct, e.g.:

https://dart-lang.github.io/linter/lints/use_string_buffers.html claims performance difference; but when compiled to js, there isn't one \ https://dart-lang.github.io/linter/lints/prefer_final_locals.html makes the vague claim "allows the compiler to do optimizations"; was this ever true? Is it still true? \ https://dart-lang.github.io/linter/lints/avoid_slow_async_io.html makes performance claims that depend on the filesystem \ https://dart-lang.github.io/linter/lints/avoid_as.html makes a claim about removing the type check that is implementation dependent and almost certainly changed with Dart 2

I wonder if we could improve this. For example, we could require linking to an authoritative source for any claims that go beyond purely mechanical details of the lint.

For performance claims, I'd very much like to see links to benchmarks--and, more than that, to benchmarks that we actually run, so we notice when the claims in the lint are no longer true.

Thoughts? :)

Thanks!

bwilkerson commented 4 years ago

Yes, the docs for lints need to be updated. They haven't been maintained as the language and SDK have evolved. And the performance-related lints are especially prone to getting out of date because of changes to the execution technologies (and might apply in some cases but not others).

While I think it would be great to link to authoritative sources, there might not be any source for some things, even when they're true at the time of writing. I'd also want to have a build step that detects broken links.

In addition to the issues you raised above I'd like to see them conform to the style of documentation we've adopted for the rest of the diagnostics.

pq commented 4 years ago

I can't speak to the exact provenance of the docs generally though I'm sure they were best effort. I'm very much in favor of improving them where possible though.

EDIT: @bwilkerson beat me to what I was about to say! 😄

davidmorgan commented 4 years ago

Another approach would be to restrict the lint docs to purely mechanical matters, and leave the 'why' to e.g. Effective Dart, pedantic, ...

There are cases where 'pedantic' instead has a 'why not' for a lint. I guess we could arrange to link those too.

bwilkerson commented 4 years ago

... leave the 'why' to e.g. Effective Dart, pedantic, ...

I'd be fine with the first if it weren't for the fact that not all of the lint rules are based on advice from Effective Dart. I do think it would be useful to link those that are based on the style guide to the section where they're discussed, but all of the lint rules need to be documented.

The second seems wrong, though. The fact that we've chosen not to enable a lint internally doesn't mean that it doesn't have value to some other group of users. I think it makes sense to explain the value that each rule has. I have no problem referencing the reasons why we're not using it internally, but it seems to me that that should be more along the lines of a rebuttal (or additional considerations) rather than the primary documentation.

jamesderlin commented 4 years ago

As I mentioned in https://github.com/dart-lang/sdk/issues/36883, https://dart-lang.github.io/linter/lints/avoid_as.html also claims that assert can remove a type-check but that's not true at all.

lukepighetti commented 2 years ago

I believe avoid_slow_async_io may be misleading in the context of a Flutter app. I would expect the synchronous io operations to block UI painting causing jank, while the async calls would allow the UI to continue painting even if it is slower to perform the task.

related: https://github.com/dart-lang/sdk/issues/36269#issuecomment-897736922

pq commented 2 years ago

@goderbauer: do you have any thoughts on @lukepighetti's thinking?