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.

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

fix [dbg_macro] FN when dbg is inside some complex macros #12151

Closed
wants to merge 3 commits into from

Conversation

J-ZhengLi
Copy link
Member

fixes: #12131

It appears that [root_macro_call_first_node] only detects println! in the following example:

println!("{:?}", dbg!(s));

changelog: fix [dbg_macro] FN when dbg is inside some complex macros

@rustbot
Copy link
Collaborator

rustbot commented Jan 15, 2024

r? @Jarcho

(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 Jan 15, 2024
@Jarcho
Copy link
Contributor

Jarcho commented Feb 7, 2024

Why are we even skipping nested calls in the first place? dbg!(foo(dbg!(bar))) should still lint multiple times like we do with every other lint.

@J-ZhengLi
Copy link
Member Author

J-ZhengLi commented Feb 7, 2024

Why are we even skipping nested calls in the first place? dbg!(foo(dbg!(bar))) should still lint multiple times like we do with every other lint.

Idk, I guess the initial purpose was to warn the existence of dbg! macro thus linting the first one should suffice?

I didn't put that 'span.contains' check at first, instead it was 'macro_span = prev_macro_span', then I saw a lot additional warnings popping up, especially on line 28 of the test file.

Should I change it back to lint every nested calls?

@Jarcho
Copy link
Contributor

Jarcho commented Feb 10, 2024

Looks like it was quirk that was kept when switching from an early pass to a late pass. check_mac only sees top level macro invocations and not anything from their expansion.

You can change it to lint all nested calls. You'll need to either split the test file in two since uitest only runs rustfix once, so nested macros won't pass testing. actual cargo fix will run multiple times to handle this.

Copy link
Contributor

@Jarcho Jarcho left a comment

Choose a reason for hiding this comment

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

It's might be worth keeping track of the most recently seen SyntaxContext as well. This will let you avoid walking the context chain on for the majority of expression.

clippy_lints/src/dbg_macro.rs Outdated Show resolved Hide resolved
clippy_lints/src/dbg_macro.rs Outdated Show resolved Hide resolved
clippy_lints/src/dbg_macro.rs Outdated Show resolved Hide resolved
@Jarcho
Copy link
Contributor

Jarcho commented Mar 6, 2024

All looks good. Thank you. @bors r+

@bors
Copy link
Contributor

bors commented Mar 6, 2024

📌 Commit aa06d41 has been approved by Jarcho

It is now in the queue for this repository.

bors added a commit that referenced this pull request Mar 6, 2024
fix [`dbg_macro`] FN when `dbg` is inside some complex macros

fixes: #12131

It appears that [`root_macro_call_first_node`] only detects `println!` in the following example:
```rust
println!("{:?}", dbg!(s));
```

---

changelog: fix [`dbg_macro`] FN when `dbg` is inside some complex macros
@bors
Copy link
Contributor

bors commented Mar 6, 2024

⌛ Testing commit aa06d41 with merge 6e6dfd2...

@bors
Copy link
Contributor

bors commented Mar 6, 2024

💔 Test failed - checks-action_test

@Jarcho
Copy link
Contributor

Jarcho commented Mar 6, 2024

You'll need to rebase to fix the test. How error patterns need to be placed when macros are used changed.

@J-ZhengLi J-ZhengLi force-pushed the issue12131 branch 3 times, most recently from d196298 to b101598 Compare March 6, 2024 15:09
@@ -49,6 +41,7 @@ fn issue9914() {
macro_rules! expand_to_dbg {
() => {
dbg!();
//~^ ERROR: the `dbg!` macro is intended as a debugging tool
Copy link
Member Author

Choose a reason for hiding this comment

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

had to put that ERROR comment inside this macro because I don't know where else to put it 😆

@Jarcho
Copy link
Contributor

Jarcho commented Mar 6, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Mar 6, 2024

📌 Commit b101598 has been approved by Jarcho

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Mar 6, 2024

⌛ Testing commit b101598 with merge 2d1dc10...

bors added a commit that referenced this pull request Mar 6, 2024
fix [`dbg_macro`] FN when `dbg` is inside some complex macros

fixes: #12131

It appears that [`root_macro_call_first_node`] only detects `println!` in the following example:
```rust
println!("{:?}", dbg!(s));
```

---

changelog: fix [`dbg_macro`] FN when `dbg` is inside some complex macros
@bors
Copy link
Contributor

bors commented Mar 6, 2024

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Jarcho
Pushing 2d1dc10 to master...

@bors
Copy link
Contributor

bors commented Mar 6, 2024

👀 Test was successful, but fast-forwarding failed: 422 1 review requesting changes by reviewers with write access.

some refractoring;
(rebase): replace `.hir().find_parent` with `.parent_hir_node`;
(rebase): move `ERROR` label into the `expand_to_dbg!` macro in test
@J-ZhengLi J-ZhengLi requested a review from Jarcho March 7, 2024 01:34
@bors
Copy link
Contributor

bors commented Mar 7, 2024

👀 Test was successful, but fast-forwarding failed: 422 1 review requesting changes by reviewers with write access.

@Jarcho
Copy link
Contributor

Jarcho commented Mar 7, 2024

@bors retry

@bors
Copy link
Contributor

bors commented Mar 7, 2024

⌛ Testing commit b101598 with merge 9991cb7...

bors added a commit that referenced this pull request Mar 7, 2024
fix [`dbg_macro`] FN when `dbg` is inside some complex macros

fixes: #12131

It appears that [`root_macro_call_first_node`] only detects `println!` in the following example:
```rust
println!("{:?}", dbg!(s));
```

---

changelog: fix [`dbg_macro`] FN when `dbg` is inside some complex macros
@bors
Copy link
Contributor

bors commented Mar 7, 2024

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Jarcho
Pushing 9991cb7 to master...

@bors
Copy link
Contributor

bors commented Mar 7, 2024

👀 Test was successful, but fast-forwarding failed: 422 1 review requesting changes by reviewers with write access.

@Jarcho
Copy link
Contributor

Jarcho commented Mar 7, 2024

Looks like rust-lang/homu#75

@bors retry

@bors
Copy link
Contributor

bors commented Mar 7, 2024

⌛ Testing commit b101598 with merge 7a72cdb...

bors added a commit that referenced this pull request Mar 7, 2024
fix [`dbg_macro`] FN when `dbg` is inside some complex macros

fixes: #12131

It appears that [`root_macro_call_first_node`] only detects `println!` in the following example:
```rust
println!("{:?}", dbg!(s));
```

---

changelog: fix [`dbg_macro`] FN when `dbg` is inside some complex macros
@bors
Copy link
Contributor

bors commented Mar 7, 2024

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Jarcho
Pushing 7a72cdb to master...

@bors
Copy link
Contributor

bors commented Mar 7, 2024

👀 Test was successful, but fast-forwarding failed: 422 1 review requesting changes by reviewers with write access.

@Jarcho
Copy link
Contributor

Jarcho commented Mar 12, 2024

Lets see if this works today. @bors retry

bors added a commit that referenced this pull request Mar 12, 2024
fix [`dbg_macro`] FN when `dbg` is inside some complex macros

fixes: #12131

It appears that [`root_macro_call_first_node`] only detects `println!` in the following example:
```rust
println!("{:?}", dbg!(s));
```

---

changelog: fix [`dbg_macro`] FN when `dbg` is inside some complex macros
@bors
Copy link
Contributor

bors commented Mar 12, 2024

⌛ Testing commit b101598 with merge 05570ad...

@bors
Copy link
Contributor

bors commented Mar 12, 2024

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Jarcho
Pushing 05570ad to master...

@bors
Copy link
Contributor

bors commented Mar 12, 2024

👀 Test was successful, but fast-forwarding failed: 422 1 review requesting changes by reviewers with write access.

@Jarcho Jarcho closed this Mar 12, 2024
@Jarcho Jarcho reopened this Mar 12, 2024
@Jarcho
Copy link
Contributor

Jarcho commented Mar 12, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Mar 12, 2024

💡 This pull request was already approved, no need to approve it again.

  • This pull request previously failed. You should add more commits to fix the bug, or use retry to trigger a build again.

@bors
Copy link
Contributor

bors commented Mar 12, 2024

📌 Commit b101598 has been approved by Jarcho

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Mar 12, 2024

⌛ Testing commit b101598 with merge 8bc40df...

bors added a commit that referenced this pull request Mar 12, 2024
fix [`dbg_macro`] FN when `dbg` is inside some complex macros

fixes: #12131

It appears that [`root_macro_call_first_node`] only detects `println!` in the following example:
```rust
println!("{:?}", dbg!(s));
```

---

changelog: fix [`dbg_macro`] FN when `dbg` is inside some complex macros
@bors
Copy link
Contributor

bors commented Mar 12, 2024

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Jarcho
Pushing 8bc40df to master...

@bors
Copy link
Contributor

bors commented Mar 12, 2024

👀 Test was successful, but fast-forwarding failed: 422 1 review requesting changes by reviewers with write access.

@J-ZhengLi
Copy link
Member Author

lol

@Jarcho
Copy link
Contributor

Jarcho commented Mar 13, 2024

@J-ZhengLi can you try reopening this as a new PR?

@J-ZhengLi
Copy link
Member Author

@J-ZhengLi can you try reopening this as a new PR?

sure, that make sense😂

@J-ZhengLi J-ZhengLi closed this Mar 13, 2024
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.

dbg_macro ignored in macro call sites
4 participants