Skip to content

Reject linked dylib EII default overrides#156370

Open
qaijuang wants to merge 4 commits into
rust-lang:mainfrom
qaijuang:eii-dylib-default-override
Open

Reject linked dylib EII default overrides#156370
qaijuang wants to merge 4 commits into
rust-lang:mainfrom
qaijuang:eii-dylib-default-override

Conversation

@qaijuang
Copy link
Copy Markdown
Contributor

@qaijuang qaijuang commented May 9, 2026

This PR rejects explicit EII implementations that would override a default implementation already selected through a linked dylib.

The check is intentionally split:

  • rustc_passes keeps the early, format-independent checks for missing impls and duplicate explicit impls.
  • rustc_codegen_ssa checks the default-vs-explicit conflict during linking, using the dependency formats selected for the final artifact.

Fixes #156320.

@rustbot rustbot added 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. labels May 9, 2026
@qaijuang qaijuang marked this pull request as ready for review May 9, 2026 20:28
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 9, 2026
@rustbot rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 9, 2026
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented May 9, 2026

r? @mejrs

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

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: compiler
  • compiler expanded to 73 candidates
  • Random selection from 19 candidates

@qaijuang
Copy link
Copy Markdown
Contributor Author

qaijuang commented May 9, 2026

cc @bjorn3

@mejrs
Copy link
Copy Markdown
Contributor

mejrs commented May 9, 2026

r? @jdonszelmann (or @bjorn3 perhaps)

@rustbot rustbot assigned jdonszelmann and unassigned mejrs May 9, 2026
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented May 9, 2026

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

@bjorn3
Copy link
Copy Markdown
Member

bjorn3 commented May 10, 2026

Thinking about this a bit more, I'm wondering if maybe check_externally_implementable_items should be done only at link time? tcx.used_crate_source() may report that only a dylib exists even when at link time an rlib will be available and linked in. And tcx.dependency_formats() will report everything as NotLinked inside rlibs and would ideally only be called during the link step for -Zno-link/-Zlink-only to work (it is currently broken because dependency_formats is called during codegen. i've been working on moving this call to the link step for the past several months). On the other hand for all cases that the check caught before this PR, doing it before linking is fine. So splitting it into separate checks for duplicate explicit impls and for a default impl conflicting with an explicit impl would preserve early error messages (especially for cargo check) What do you think @jdonszelmann?

@qaijuang
Copy link
Copy Markdown
Contributor Author

Thinking about this a bit more, I'm wondering if maybe check_externally_implementable_items should be done only at link time? tcx.used_crate_source() may report that only a dylib exists even when at link time an rlib will be available and linked in. And tcx.dependency_formats() will report everything as NotLinked inside rlibs and would ideally only be called during the link step for -Zno-link/-Zlink-only to work (it is currently broken because dependency_formats is called during codegen.

Nice catch @bjorn3

So splitting it into separate checks for duplicate explicit impls and for a default impl conflicting with an explicit impl would preserve early error messages (especially for cargo check)

right? seems like the next best move

@qaijuang qaijuang force-pushed the eii-dylib-default-override branch from dfc410a to 44e8bb8 Compare May 11, 2026 15:11
@qaijuang qaijuang changed the title Reject EII overrides of dylib defaults Reject linked dylib EII default overrides May 11, 2026
@qaijuang
Copy link
Copy Markdown
Contributor Author

@bjorn3 I'm done with the split, what do you think?

@rust-bors

This comment has been minimized.

@rustbot

This comment has been minimized.

@rustbot

This comment has been minimized.

@rustbot rustbot added has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 14, 2026
@qaijuang qaijuang force-pushed the eii-dylib-default-override branch from 5af5be7 to b41cef3 Compare May 14, 2026 20:14
@rustbot rustbot removed has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 14, 2026
@jdonszelmann
Copy link
Copy Markdown
Contributor

@bjorn3 that approach seems reasonable to me

Comment thread compiler/rustc_codegen_ssa/src/back/link.rs Outdated
Comment thread compiler/rustc_codegen_ssa/src/base.rs Outdated
@qaijuang qaijuang force-pushed the eii-dylib-default-override branch from 3827ccb to 621745e Compare May 28, 2026 17:36
@rust-log-analyzer

This comment has been minimized.

@qaijuang qaijuang force-pushed the eii-dylib-default-override branch from 621745e to 370c16b Compare May 28, 2026 20:04
@rust-log-analyzer

This comment has been minimized.

@qaijuang qaijuang force-pushed the eii-dylib-default-override branch from a982691 to 370c16b Compare May 28, 2026 22:16
@qaijuang qaijuang force-pushed the eii-dylib-default-override branch from 370c16b to a7003f8 Compare May 28, 2026 22:19
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented May 28, 2026

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.

Copy link
Copy Markdown
Contributor Author

@qaijuang qaijuang left a comment

Choose a reason for hiding this comment

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

somehow, this got removed from main, making tests/ui/eii/default/call_impl.rs regress, which is why I did the rebase.

View changes since this review

@@ -1,3 +1,4 @@
//@ no-prefer-dynamic
Copy link
Copy Markdown
Contributor Author

@qaijuang qaijuang May 28, 2026

Choose a reason for hiding this comment

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

@@ -1,3 +1,4 @@
//@ no-prefer-dynamic
Copy link
Copy Markdown
Contributor Author

@qaijuang qaijuang May 28, 2026

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

Don't allow overriding an EII default defined in a dylib

6 participants