Skip to content

Make many MacResult methods unreachable. #142061

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nnethercote
Copy link
Contributor

  • Every method in the MacResult trait can be unreachable except make_stmts.

  • Lots of MacEager methods can removed, so they fall back to the default unreachable methods. Because of this, the impl_items/trait_items/foreign_items fields are never used and can also be removed.

r? @petrochenkov

- Every method in the `MacResult` trait can be unreachable *except*
  `make_stmts`.

- Lots of `MacEager` methods can removed, so they fall back to the
  default unreachable methods. Because of this, the
  `impl_items`/`trait_items`/`foreign_items` fields are never used and
  can also be removed.
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 5, 2025
@nnethercote
Copy link
Contributor Author

@petrochenkov: see what you think of this. I don't fully understand the details of the affected code. I was trying to work out how some of these methods are called, and so I inserted a bunch of unreachable!() calls and discovered that they aren't. Maybe they are genuinely unreachable, maybe there is some missing test coverage, I'm not sure. I think if a method truly is unreachable it's good to have unreachable!() in it, it makes the code more understandable.

@@ -501,9 +501,6 @@ make_MacEager! {
expr: P<ast::Expr>,
pat: P<ast::Pat>,
items: SmallVec<[P<ast::Item>; 1]>,
impl_items: SmallVec<[P<ast::AssocItem>; 1]>,
trait_items: SmallVec<[P<ast::AssocItem>; 1]>,
foreign_items: SmallVec<[P<ast::ForeignItem>; 1]>,
stmts: SmallVec<[ast::Stmt; 1]>,
ty: P<ast::Ty>,
Copy link
Contributor

Choose a reason for hiding this comment

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

MacEager can certainly be trimmed further, only MacEager::{expr, items, ty} are actually used in built-in macros, the other fields are never set to anything other than None and their uses can be replaced with None.


fn make_variants(self: Box<DummyResult>) -> Option<SmallVec<[ast::Variant; 1]>> {
Some(SmallVec::new())
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We need

  • a test using #[rustfmt::skip] in all possible attribute positions,
  • a test using log_syntax!() in all possible fn-like macro positions,
  • and a test using include!() or concat_idents!() in all possible fn-like macro positions.

If all of those do not crash, then we can keep the unreachable!s.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants