Skip to content

Rust: skip unexpanded stuff in library emission #19585

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: main
Choose a base branch
from

Conversation

redsun82
Copy link
Contributor

This will skip all unexpanded entities in library extraction, where we only really care about expanded things. This means skipping:

  • the token tree of macro calls
  • the unexpanded AST of attribute macros

In the latter case, in order to replace the single Item with its expansion (which is a MacroItems entity), we wrap the MacroItems in a dummy MacroCall with null path.

This will skip all unexpanded entities in library extraction, where we
only really care about expanded things. This means skipping:

* the token tree of macro calls
* the unexpanded AST of attribute macros

In the latter case, in order to replace the single `Item` with its
expansion (which is a `MacroItems` entity), we wrap the `MacroItems` in
a dummy `MacroCall` with null path.
@github-actions github-actions bot added the Rust Pull requests that update Rust code label May 27, 2025
@redsun82 redsun82 marked this pull request as ready for review May 27, 2025 12:55
@Copilot Copilot AI review requested due to automatic review settings May 27, 2025 12:55
@redsun82 redsun82 requested a review from a team as a code owner May 27, 2025 12:55
@redsun82
Copy link
Contributor Author

redsun82 commented May 27, 2025

DCA shows a moderate speedup, and improvement in call graph resolution (with one exception being bevy going very slightly worse). I think this is worth taking in.

@redsun82 redsun82 requested a review from aibaars May 27, 2025 12:56
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR enhances library-mode emission by skipping unexpanded macro entities and only emitting fully expanded items.

  • Updated pre_emit! to use prepare_item_expansion for early returns of attribute macro expansions.
  • Added logic to skip nodes inside unexpanded macro call token trees during library extraction.
  • Introduced prepare_item_expansion and emit_attribute_macro_expansion to wrap attribute macro expansions in a dummy MacroCall and refactored emit_item_expansion.
Comments suppressed due to low confidence (1)

rust/extractor/src/translate/base.rs:692

  • New skip logic for nodes inside unexpanded macro call token trees lacks accompanying tests; consider adding unit tests to verify that these nodes are correctly ignored in library mode.
if syntax

) -> Option<Label<generated::Item>> {
if self.source_kind == SourceKind::Library {
// if the item expands via an attribute macro, we want to only emit the expansion
if let Some(expanded) = self.emit_attribute_macro_expansion(node) {
Copy link
Preview

Copilot AI May 27, 2025

Choose a reason for hiding this comment

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

emit_attribute_macro_expansion is invoked both in prepare_item_expansion and again in emit_item_expansion, which may lead to duplicate work or inconsistent emission. Consider caching the result or ensuring it's only called once.

Suggested change
if let Some(expanded) = self.emit_attribute_macro_expansion(node) {
let node_id = node.syntax().text_range().start().into(); // Use a unique identifier for the node
if let Some(expanded) = self.macro_expansion_cache.get(&node_id).cloned()
.or_else(|| {
let result = self.emit_attribute_macro_expansion(node);
if let Some(ref expansion) = result {
self.macro_expansion_cache.insert(node_id, expansion.clone());
}
result
})
{

Copilot uses AI. Check for mistakes.

// we wrap it in a dummy MacroCall to get a single Item label that can replace
// the original Item
let label = self.trap.emit(generated::MacroCall {
id: TrapId::Star,
Copy link
Preview

Copilot AI May 27, 2025

Choose a reason for hiding this comment

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

[nitpick] Using a magic identifier (TrapId::Star) for dummy MacroCall may be unclear—consider defining a named constant or adding a comment explaining its special purpose.

Suggested change
id: TrapId::Star,
id: DUMMY_MACRO_CALL_ID,

Copilot uses AI. Check for mistakes.

@@ -723,52 +733,75 @@ impl<'a> Translator<'a> {
}
}

Copy link
Preview

Copilot AI May 27, 2025

Choose a reason for hiding this comment

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

prepare_item_expansion is a new public method and would benefit from a doc comment explaining its behavior, parameters, and return value.

Suggested change
/// Prepares an item for macro expansion if applicable.
///
/// This method handles items that may expand via attribute macros. If the item is part of a
/// library source and expands through an attribute macro, it emits the expansion and wraps
/// it in a dummy `MacroCall` to produce a single `Item` label. Otherwise, it checks if the
/// item is an attribute macro call and adjusts the macro context depth accordingly.
///
/// # Parameters
/// - `node`: A reference to the AST node representing the item to be prepared for expansion.
///
/// # Returns
/// - `Some(Label<generated::Item>)` if the item is expanded and wrapped in a `MacroCall`.
/// - `None` if no expansion is performed or the item is not an attribute macro call.

Copilot uses AI. Check for mistakes.

@@ -687,6 +689,14 @@ impl<'a> Translator<'a> {
{
return true;
}
if syntax
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can simply drop any node of type TokenTree. That includes macro call arguments, as well as macro rule/definition bodies. Like done in #19581

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Rust Pull requests that update Rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants