Skip to content

Store DefId instead of EiiDecl in EiiImplResolution::Known#158235

Open
cezarbbb wants to merge 1 commit into
rust-lang:mainfrom
cezarbbb:eii-metadata-opt
Open

Store DefId instead of EiiDecl in EiiImplResolution::Known#158235
cezarbbb wants to merge 1 commit into
rust-lang:mainfrom
cezarbbb:eii-metadata-opt

Conversation

@cezarbbb

@cezarbbb cezarbbb commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Fixes the FIXME(eii) about removing the extern target from the encoded decl.

Known(EiiDecl) serialized the same EiiDecl N+1 times per entry (once as the declaration, once per Known impl). Changing to Known(DefId) eliminates that redundancy — the EiiDecl is recoverable via tcx.externally_implementable_items() or a local lookup map, consistent with how Rust metadata uses DefId as a reference elsewhere.

I believe there are benefits in both performance and size(full EiiDecl to single DefId, and O(N) to O(1) lookup).

Also fixes a soundness gap(I think?) in check_attr.rs where the Known branch skipped impl_unsafe checking, and replaces an .expect() in codegen_attrs.rs with a let Some pattern.

tracking issues: #125418

r? @jdonszelmann

@rustbot

rustbot commented Jun 22, 2026

Copy link
Copy Markdown
Collaborator

Some changes occurred in compiler/rustc_passes/src/check_attr.rs

cc @jdonszelmann, @JonathanBrouwer

Some changes occurred in compiler/rustc_hir/src/attrs

cc @jdonszelmann, @JonathanBrouwer

@rustbot rustbot added A-attributes Area: Attributes (`#[…]`, `#![…]`) 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 22, 2026
@rustbot

rustbot commented Jun 22, 2026

Copy link
Copy Markdown
Collaborator

jdonszelmann is currently at their maximum review capacity.
They may take a while to respond.

Comment thread compiler/rustc_metadata/src/eii.rs Outdated
Comment thread compiler/rustc_codegen_ssa/src/codegen_attrs.rs Outdated
@rust-bors

This comment has been minimized.

@cezarbbb cezarbbb force-pushed the eii-metadata-opt branch from 499975b to c65973d Compare July 6, 2026 02:50
@rustbot

rustbot commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

This PR was rebased onto a different main 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.

) {
EiiImplResolution::Known(decl)
&& let Some(foreign_item_did) =
self.lower_path_simple_eii(*node_id, &target.foreign_item)

@bjorn3 bjorn3 Jul 8, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

According to the doc comment of the known_eii_macro_resolution field on EiiImpl, it is supposed to bypass name resolution to prevent bugs like #149981. This PR effectively changes it back to resolving paths (target.foreign_item is a path). If bypassing name resolution is no longer necessary, maybe known_eii_macro_resolution could be removed entirely? Or did I misunderstand the exact problem this is supposed to workaround?

Edit: Maybe what actually matters is getting the path to the foreign item rather than to the macro here:

View changes since the review

@bjorn3

bjorn3 commented Jul 8, 2026

Copy link
Copy Markdown
Member

You should be able to simplify this a bit further:

Author: bjorn3 <17426603+bjorn3@users.noreply.github.com>
Date:   Wed Jul 8 11:29:54 2026 +0200

    Replace EiiDecl in known_eii_macro_resolution with Path
    
    Only the Path is actually looked at now.

diff --git a/compiler/rustc_ast/src/ast.rs b/compiler/rustc_ast/src/ast.rs
index bd5ce5d18b8..9b881688a6b 100644
--- a/compiler/rustc_ast/src/ast.rs
+++ b/compiler/rustc_ast/src/ast.rs
@@ -3924,7 +3924,7 @@ pub struct EiiImpl {
     ///
     /// This field is that shortcut: we prefill the extern target to skip a name resolution step,
     /// making sure it never fails. It'd be awful UX if we fail name resolution in code invisible to the user.
-    pub known_eii_macro_resolution: Option<EiiDecl>,
+    pub known_eii_macro_resolution: Option<Path>,
     pub impl_safety: Safety,
     pub span: Span,
     pub inner_span: Span,
diff --git a/compiler/rustc_ast_lowering/src/item.rs b/compiler/rustc_ast_lowering/src/item.rs
index 8b129576308..8d4a6f10476 100644
--- a/compiler/rustc_ast_lowering/src/item.rs
+++ b/compiler/rustc_ast_lowering/src/item.rs
@@ -141,8 +141,7 @@ fn lower_eii_impl(
         }: &EiiImpl,
     ) -> hir::attrs::EiiImpl {
         let resolution = if let Some(target) = known_eii_macro_resolution
-            && let Some(foreign_item_did) =
-                self.lower_path_simple_eii(*node_id, &target.foreign_item)
+            && let Some(foreign_item_did) = self.lower_path_simple_eii(*node_id, target)
         {
             EiiImplResolution::Known(foreign_item_did)
         } else if let Some(macro_did) = self.lower_path_simple_eii(*node_id, eii_macro_path) {
diff --git a/compiler/rustc_builtin_macros/src/eii.rs b/compiler/rustc_builtin_macros/src/eii.rs
index 68d5278c85e..66879d5daf7 100644
--- a/compiler/rustc_builtin_macros/src/eii.rs
+++ b/compiler/rustc_builtin_macros/src/eii.rs
@@ -302,15 +302,12 @@ fn generate_default_impl(
         },
         span: eii_attr_span,
         is_default: true,
-        known_eii_macro_resolution: Some(ast::EiiDecl {
-            foreign_item: ecx.path(
-                foreign_item_name.span,
-                // prefix self to explicitly escape the const block generated below
-                // NOTE: this is why EIIs can't be used on statements
-                vec![Ident::from_str_and_span("self", foreign_item_name.span), foreign_item_name],
-            ),
-            impl_unsafe,
-        }),
+        known_eii_macro_resolution: Some(ecx.path(
+            foreign_item_name.span,
+            // prefix self to explicitly escape the const block generated below
+            // NOTE: this is why EIIs can't be used on statements
+            vec![Ident::from_str_and_span("self", foreign_item_name.span), foreign_item_name],
+        )),
     };
 
     let mut item_kind = item_kind.clone();
diff --git a/compiler/rustc_resolve/src/late.rs b/compiler/rustc_resolve/src/late.rs
index c94f2f3ffb8..6f206aadf0a 100644
--- a/compiler/rustc_resolve/src/late.rs
+++ b/compiler/rustc_resolve/src/late.rs
@@ -5609,12 +5609,7 @@ fn resolve_eii(&mut self, eii_impls: &[EiiImpl]) {
             // See docs on the `known_eii_macro_resolution` field:
             // if we already know the resolution statically, don't bother resolving it.
             if let Some(target) = known_eii_macro_resolution {
-                self.smart_resolve_path(
-                    *node_id,
-                    &None,
-                    &target.foreign_item,
-                    PathSource::ExternItemImpl,
-                );
+                self.smart_resolve_path(*node_id, &None, target, PathSource::ExternItemImpl);
             } else {
                 self.smart_resolve_path(*node_id, &None, &eii_macro_path, PathSource::Macro);
             }

@bjorn3 bjorn3 added the F-extern_item_impls `#![feature(extern_item_impls)]` label Jul 8, 2026
@bjorn3

bjorn3 commented Jul 8, 2026

Copy link
Copy Markdown
Member

r? bjorn3

@rustbot rustbot assigned bjorn3 and unassigned jdonszelmann Jul 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-attributes Area: Attributes (`#[…]`, `#![…]`) F-extern_item_impls `#![feature(extern_item_impls)]` 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants