Skip to content

feat(utils/check_proc_macro): impl WithSearchPat for ast::Expr #15972

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
Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

feat(utils/check_proc_macro): impl WithSearchPat for ast::Expr #15972

wants to merge 3 commits into from

Conversation

@ada4a
Copy link
Contributor

@ada4a ada4a commented Oct 28, 2025

Extracted from #15939, which ended up not needing it.

changelog: none

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 28, 2025
@rustbot
Copy link
Collaborator

rustbot commented Oct 28, 2025

r? @Jarcho

rustbot has assigned @Jarcho.
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

samueltardieu
samueltardieu reviewed Oct 29, 2025
View reviewed changes
Copy link
Member

@samueltardieu samueltardieu left a comment •
edited by rustbot
Loading

Choose a reason for hiding this comment

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

It looks neither used (thus exercised through UI tests) nor unit-tested. Isn't that a bit hazardous to include this?

View changes since this review

clippy_utils/src/check_proc_macro.rs Outdated
LitKind::Byte => (Pat::Str("b'"), Pat::Str("'")),
LitKind::ByteStr => (Pat::Str("b\""), Pat::Str("\"")),
LitKind::ByteStrRaw(0) => (Pat::Str("br\""), Pat::Str("\"")),
LitKind::ByteStrRaw(_) => (Pat::Str("br#\""), Pat::Str("#")),
Copy link
Member

@samueltardieu samueltardieu Oct 29, 2025

Choose a reason for hiding this comment

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

Suggested change
LitKind::ByteStrRaw(_) => (Pat::Str("br#\""), Pat::Str("#")),
LitKind::ByteStrRaw(_) => (Pat::Str("br#"), Pat::Str("#")),

clippy_utils/src/check_proc_macro.rs Outdated
LitKind::ByteStrRaw(_) => (Pat::Str("br#\""), Pat::Str("#")),
LitKind::CStr => (Pat::Str("c\""), Pat::Str("\"")),
LitKind::CStrRaw(0) => (Pat::Str("cr\""), Pat::Str("\"")),
LitKind::CStrRaw(_) => (Pat::Str("cr#\""), Pat::Str("#\"")),
Copy link
Member

@samueltardieu samueltardieu Oct 29, 2025

Choose a reason for hiding this comment

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

Suggested change
LitKind::CStrRaw(_) => (Pat::Str("cr#\""), Pat::Str("#\"")),
LitKind::CStrRaw(_) => (Pat::Str("cr#"), Pat::Str("#")),

clippy_utils/src/check_proc_macro.rs
ExprKind::Unary(UnOp::Not, e) => (Pat::Str("!"), inner(e, outer_span).1),
ExprKind::Unary(UnOp::Neg, e) => (Pat::Str("-"), inner(e, outer_span).1),
ExprKind::Lit(lit) => token_lit_search_pat(lit),
ExprKind::Paren(e) => inner(e, outer_span),
Copy link
Member

@samueltardieu samueltardieu Oct 29, 2025

Choose a reason for hiding this comment

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

Let's imagine we have ([1, 2]). Wouldn't ExprKind::Paren(ExprKind::Array(…)) return ("[", "]")? Would that match?

Copy link
Contributor Author

@ada4a ada4a Oct 29, 2025

Choose a reason for hiding this comment

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

I think it would, because span_matches_pat trims parens before checking. The other places where this happens have the following comment:

// Parenthesis are trimmed from the text before the search patterns are matched.
// See: `span_matches_pat`

I probably should add it here as well

@ada4a
Copy link
Contributor Author

ada4a commented Oct 29, 2025

Isn't that a bit hazardous to include this?

Yeah you're probably right... but I put so much time into this, and it'd be a shame to let all that go to waste :(

I would want to at least give an eventual implementer some hint about this PR existing, so that they know they don't need to start from scratch -- my idea would be to have a stub ast_expr_search_pat with either the code from this PR but commented out, or a panic + a link to this PR. WDYT?

@ada4a ada4a force-pushed the with_search_pat_ast_expr branch from 9d8a0f2 to 395cfe0 Compare October 29, 2025 10:29
@samueltardieu
Copy link
Member

samueltardieu commented Oct 29, 2025

Isn't that a bit hazardous to include this?

Yeah you're probably right... but I put so much time into this, and it'd be a shame to let all that go to waste :(

I would want to at least give an eventual implementer some hint about this PR existing, so that they know they don't need to start from scratch -- my idea would be to have a stub ast_expr_search_pat with either the code from this PR but commented out, or a panic + a link to this PR. WDYT?

@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 29, 2025
@samueltardieu samueltardieu reopened this Oct 29, 2025
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 29, 2025
@samueltardieu
Copy link
Member

samueltardieu commented Oct 29, 2025

(sorry, didn't intend to close this)

Yes of course, this must not be lost. Isn't there a place where this could be used (and tested)?

@ada4a
Copy link
Contributor Author

ada4a commented Oct 29, 2025

(sorry, didn't intend to close this)

Ah, good 😅

Isn't there a place where this could be used?

Now that you said it, yes, this could probably be added to a lot of early-pass lints as a !is_from_proc_macro check. I'll try to do that for some lints, and add corresponding tests.

@ada4a ada4a force-pushed the with_search_pat_ast_expr branch from 395cfe0 to 54a84d5 Compare November 7, 2025 20:16
@rustbot
Copy link
Collaborator

rustbot commented Nov 7, 2025

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@ada4a
Copy link
Contributor Author

ada4a commented Nov 7, 2025

So it turns out that the intersection of "early-pass lints" and "lints that check for any kind of expansion" was quite small, and so I only added the check in a couple lints.. hopefully that will be a good start

@ada4a ada4a force-pushed the with_search_pat_ast_expr branch from 54a84d5 to 2be9421 Compare November 7, 2025 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@samueltardieu samueltardieu samueltardieu left review comments

Assignees

@Jarcho Jarcho

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.

4 participants

@ada4a @rustbot @samueltardieu @Jarcho