Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
davidmorgan opened this issue May 26, 2020 · 7 comments
Open

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

davidmorgan opened this issue May 26, 2020 · 7 comments
Labels
area-devexp For issues related to the analysis server, IDE support, linter, `dart fix`, and diagnostic messages. customer-google3 devexp-linter Issues with the analyzer's support for the linter package P2 A bug or feature request we're likely to work on type-documentation A request to add or improve documentation type-enhancement A request for a change that isn't a bug

Comments

@davidmorgan
Copy link
Contributor

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
Copy link
Member

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
Copy link
Member

pq commented May 26, 2020

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
Copy link
Contributor Author

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
Copy link
Member

... 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
Copy link
Contributor

jamesderlin commented May 28, 2020

As I mentioned in #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
Copy link

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: #36269 (comment)

@pq
Copy link
Member

pq commented Aug 13, 2021

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

@srawlins srawlins added the P2 A bug or feature request we're likely to work on label Sep 22, 2022
@srawlins srawlins added the type-enhancement A request for a change that isn't a bug label Mar 29, 2024
@devoncarew devoncarew added devexp-linter Issues with the analyzer's support for the linter package legacy-area-analyzer Use area-devexp instead. labels Nov 18, 2024
@devoncarew devoncarew transferred this issue from dart-archive/linter Nov 18, 2024
@bwilkerson bwilkerson added area-devexp For issues related to the analysis server, IDE support, linter, `dart fix`, and diagnostic messages. and removed legacy-area-analyzer Use area-devexp instead. labels Feb 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-devexp For issues related to the analysis server, IDE support, linter, `dart fix`, and diagnostic messages. customer-google3 devexp-linter Issues with the analyzer's support for the linter package P2 A bug or feature request we're likely to work on type-documentation A request to add or improve documentation type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

7 participants