Skip to content

Modify lint pass note for consistency with the book #12596

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

Merged
merged 1 commit into from
Apr 30, 2024

Conversation

ARandomDev99
Copy link
Contributor

This PR:

  • removes the note which appears when an early lint pass is created using cargo dev new_lint.
  • adds a note that encourages contributors to use an early pass unless type information is needed if a late lint pass is created using cargo dev new_lint.

Late pass remains the default value if no pass is specified as most lints use late pass.

Closes #12595

changelog: none

@rustbot
Copy link
Collaborator

rustbot commented Mar 30, 2024

r? @Alexendoo

rustbot has assigned @Alexendoo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Mar 30, 2024
@Centri3
Copy link
Member

Centri3 commented Mar 30, 2024

This has been discussed before in some meeting, in general early lints are discouraged as often the lint will need additional info not present in an early pass later on anyway and thus will need to be rewritten (for example, to use is_from_proc_macro, or get the parent of a node), which doesn't outweigh the slight performance increase (which I haven't tested, and would be skeptical of tbh)

The only upsides to an early pass (the mentioned "something specific from an early pass") are handling ranges (which aren't desugared yet) or telling whether a module is inlined (which also isn't desugared yet), which you can do both in the HIR anyway. Otherwise you lose access to every query.

So we should change the book's documentation instead :P

@Alexendoo
Copy link
Member

Agreed it's confusing, yeah focusing on LateLintPass is the goal (#9311)

@ARandomDev99
Copy link
Contributor Author

I'm thinking this:

In short, the LateLintPass has access to type information while the EarlyLintPass doesn't. If you don't need access to type information, use the EarlyLintPass. The EarlyLintPass is also faster. However, linting speed hasn't really been a concern with Clippy so far.

should be replaced with:

In short, the LateLintPass has access to type information. The EarlyLintPass is faster. Linting speed hasn't been a concern with Clippy so far. So you should consider using the LateLintPass unless you need something specific from the EarlyLintPass.

Does this look good?

@Jarcho
Copy link
Contributor

Jarcho commented Mar 31, 2024

Early lints aren't inherently faster. They tend to be faster because they generally do less work, but a late lint only doing syntax checking will be just as fast. What early pass lints do is run before type checking and HIR lowering. This means code like this:

fn main() {
    let y = 'a'..'z';
    let x: () = 0;
}

can display:

warning: almost complete ascii range
 --> src/main.rs:4:13
  |
4 |     let y = 'a'..'z';
  |             ^^^--^^^
  |                |
  |                help: use an inclusive range: `..=`
  |
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#almost_complete_range
  = note: `#[warn(clippy::almost_complete_range)]` on by default

error[E0308]: mismatched types
 --> src/main.rs:5:17
  |
5 |     let x: () = 0;
  |            --   ^ expected `()`, found integer
  |            |
  |            expected due to this

If almost_complete_range were a late lint only the type error would occur.

@ARandomDev99
Copy link
Contributor Author

ARandomDev99 commented Apr 1, 2024

In that case, let's omit any mention of performance:

In short, the EarlyLintPass runs before type checking and HIR lowering and the LateLintPass has access to type information. Consider using the LateLintPass unless you need something specific from the EarlyLintPass.

@ARandomDev99
Copy link
Contributor Author

@Alexendoo can you please review the changes? :)

@Alexendoo
Copy link
Member

Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Apr 30, 2024

📌 Commit e2861d5 has been approved by Alexendoo

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Apr 30, 2024

⌛ Testing commit e2861d5 with merge d8e76ec...

@bors
Copy link
Contributor

bors commented Apr 30, 2024

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Alexendoo
Pushing d8e76ec to master...

@bors bors merged commit d8e76ec into rust-lang:master Apr 30, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistency about which lint pass to use (cargo dev and Clippy Book)
6 participants