Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix is_from_proc_macro patterns #11538

Merged
merged 2 commits into from Dec 11, 2023
Merged

Fix is_from_proc_macro patterns #11538

merged 2 commits into from Dec 11, 2023

Conversation

Jarcho
Copy link
Contributor

@Jarcho Jarcho commented Sep 19, 2023

fixes #11533

changelog: none

@rustbot
Copy link
Collaborator

rustbot commented Sep 19, 2023

r? @dswij

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 19, 2023
blyxyas added a commit to blyxyas/rust that referenced this pull request Sep 24, 2023
@blyxyas
Improving is_from_proc_macro
4a6e902 
Finally, after a few months I finally release this 0.9.9 version of the final 'is_from_proc_macro'.
The main changes happen on:
  - Moving 'check_from_proc_macro.rs' from Clippy to rustc_lint
  - Applying a Jarcho's (yet to be merged) fix to that file (See rust-lang/rust-clippy#11538)
  - Change every occurence of 'is_from_proc_macro' to 'cx.in_proc_macro'
  - Added a new macro to 'rustc_lint/late.rs', now it checks if we're in Clippy, if we are indeed in Clippy, check if we're in a proc macro.
  - Apply some parallization to the checking with already provided functions (nothing too fancy, just using a parallization function)
  - Fix a bug on 'min_ident_chars.rs'
  - A few more things I don't remember...

You can test this commit with 'RUST_BACKTRACE=full x test src/tools/clippy --keep-stage=2'
blyxyas added a commit to blyxyas/rust that referenced this pull request Sep 25, 2023
@blyxyas
Improving is_from_proc_macro
f6b47b5 
Finally, after a few months I finally release this 0.9.9 version of the final 'is_from_proc_macro'.
The main changes happen on:
  - Moving 'check_from_proc_macro.rs' from Clippy to rustc_lint
  - Applying a Jarcho's (yet to be merged) fix to that file (See rust-lang/rust-clippy#11538)
  - Change every occurence of 'is_from_proc_macro' to 'cx.in_proc_macro'
  - Added a new macro to 'rustc_lint/late.rs', now it checks if we're in Clippy, if we are indeed in Clippy, check if we're in a proc macro.
  - Apply some parallization to the checking with already provided functions (nothing too fancy, just using a parallization function)
  - Fix a bug on 'min_ident_chars.rs'
  - A few more things I don't remember...

You can test this commit with 'RUST_BACKTRACE=full x test src/tools/clippy --keep-stage=2'
@GuillaumeGomez
Copy link
Member

r? @Centri3

@rustbot rustbot assigned Centri3 and unassigned dswij Oct 6, 2023
@blyxyas blyxyas added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 7, 2023
@blyxyas
Copy link
Member

blyxyas commented Oct 14, 2023

Maybe some optimizing would be pretty cool, we have a 3-second slowdown just from this PR on benchmarks.

@Jarcho
Copy link
Contributor Author

Jarcho commented Oct 14, 2023

Those results are quite suspicious given that part of this involves changing is_from_proc_macro to run only on as casts and not on every expression

@Jarcho
Copy link
Contributor Author

Jarcho commented Nov 2, 2023

Got around to checking this. Testing was done on the current master and this pr rebased onto it. Timed command is cargo dev dogfood after touch clippy_lints/src/lib.rs. Running both ten times got this pr as slightly faster (~52 seconds vs. ~56 seconds).

@bors
Copy link
Collaborator

bors commented Nov 2, 2023

☔ The latest upstream changes (presumably #11747) made this pull request unmergeable. Please resolve the merge conflicts.

Centri3
Centri3 approved these changes Nov 3, 2023
View reviewed changes
Copy link
Member

@Centri3 Centri3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This overall looks good to me, I just want some clarification regarding parenthesis.

clippy_utils/src/check_proc_macro.rs Outdated
@@ -125,6 +121,7 @@ fn qpath_search_pat(path: &QPath<'_>) -> (Pat, Pat) {
fn expr_search_pat(tcx: TyCtxt<'_>, e: &Expr<'_>) -> (Pat, Pat) {
match e.kind {
ExprKind::ConstBlock(_) => (Pat::Str("const"), Pat::Str("}")),
// Parenthesis are skipped before the patterns are matched.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment doesn't make much sense to me. I recall expressions within ExprKind::Paren retain the parenthesis' span? Can you elaborate on this further? Is this special for Tup?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's referring to how the search patterns are matched to the source text. It's why the head and tail patterns for an empty tuple appear flipped.

Copy link
Member

@Centri3 Centri3 Nov 14, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I understand now. Can you update this comment then? Perhaps:

// Parentheses are skipped before the patterns are matched. With a non-empty tuple, the head/tail pattern will be the first/last expression - with an empty tuple, there are no expressions, so using the next/previous token will flip the parentheses

(If that's why anyway.)

This is ready after

@flip1995
Copy link
Member

flip1995 commented Nov 9, 2023

Today is backport day. This PR isn't merged yet. What's the motivation behind backporting this? If this is only for performance improvements, I'd rather not backport it.

@Jarcho
Copy link
Contributor Author

Jarcho commented Nov 9, 2023

The first commit has an ICE fix.

@flip1995
Copy link
Member

flip1995 commented Nov 9, 2023

As this is not merged yet, I'll include this backport in the next release cycle.

@bors
Copy link
Collaborator

bors commented Nov 10, 2023

☔ The latest upstream changes (presumably #11750) made this pull request unmergeable. Please resolve the merge conflicts.

@dswij
Copy link
Member

dswij commented Dec 11, 2023

@bors r+

@bors
Copy link
Collaborator

bors commented Dec 11, 2023

📌 Commit f3f2f17 has been approved by dswij

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Dec 11, 2023

⌛ Testing commit f3f2f17 with merge 3813a7b...

@bors
Copy link
Collaborator

bors commented Dec 11, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: dswij
Pushing 3813a7b to master...

@bors bors merged commit 3813a7b into rust-lang:master Dec 11, 2023
5 checks passed
@flip1995
Copy link
Member

The first commit is already contained in the Rust upstream beta branch.

@flip1995 flip1995 added beta-nominated Nominated for backporting to the compiler in the beta channel. and removed beta-nominated Nominated for backporting to the compiler in the beta channel. labels Dec 21, 2023
@flip1995
Copy link
Member

Nvm, beta was apparently branched 1 day earlier than usual. I'll try to figure out how to backport those PRs anyway.

@flip1995 flip1995 mentioned this pull request Dec 21, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 21, 2023
@bors
Auto merge of rust-lang#119188 - flip1995:clippy-beta-backport, r=pie…
8a2881e 
…troalbini

[beta] Clippy beta backport

PR towards stable, as beta was branched a day early and I missed the notification.

- rust-lang/rust-clippy#11538
- rust-lang/rust-clippy#11756
- rust-lang/rust-clippy#11760
- rust-lang/rust-clippy#11953

r? `@pietroalbini`
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 21, 2023
@bors
Auto merge of rust-lang#119188 - flip1995:clippy-beta-backport, r=pie…
82e1608 
…troalbini

[beta] Clippy beta backport

PR towards stable, as beta was branched a day early and I missed the notification.

- rust-lang/rust-clippy#11538
- rust-lang/rust-clippy#11756
- rust-lang/rust-clippy#11760
- rust-lang/rust-clippy#11953

r? `@pietroalbini`
@flip1995 flip1995 added beta-accepted Accepted for backporting to the compiler in the beta channel. and removed beta-nominated Nominated for backporting to the compiler in the beta channel. labels Jan 4, 2024
@xFrednet xFrednet removed the beta-accepted Accepted for backporting to the compiler in the beta channel. label Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Centri3 Centri3 Centri3 approved these changes

@dswij dswij dswij approved these changes

Assignees

@Centri3 Centri3

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Stack overflow in clippy_utils::check_proc_macro::ty_search_pat
9 participants
@Jarcho @rustbot @GuillaumeGomez @blyxyas @bors @flip1995 @dswij @Centri3 @xFrednet