Skip to content

Reject linked dylib EII default overrides#156370

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

Reject linked dylib EII default overrides#156370
qaijuang wants to merge 2 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?

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

5 participants