Skip to content

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

Open
ada4a wants to merge 3 commits into
rust-lang:masterfrom
ada4a:with_search_pat_ast_expr
Open

feat(utils/check_proc_macro): impl WithSearchPat for ast::Expr#15972
ada4a wants to merge 3 commits into
rust-lang:masterfrom
ada4a:with_search_pat_ast_expr

Conversation

@ada4a
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

Comment thread 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
Copy Markdown
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("#")),

Comment thread 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
Copy Markdown
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("#")),

Comment thread 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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

This comment has been minimized.

Sign in to view
@ada4a
Copy link
Copy Markdown
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
@rustbot

This comment has been minimized.

Sign in to view
@ada4a ada4a force-pushed the with_search_pat_ast_expr branch from 2be9421 to e2ea611 Compare November 27, 2025 21:29
@rustbot

This comment has been minimized.

Sign in to view
@rustbot

This comment has been minimized.

Sign in to view
@ada4a ada4a force-pushed the with_search_pat_ast_expr branch from e2ea611 to fdfef98 Compare February 23, 2026 13:40
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Feb 23, 2026

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.

@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Apr 15, 2026

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

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