Resolve instance for SymFn in global/naked asm by compiler-errors · Pull Request #140374 · rust-lang/rust

compiler-errors · 2025-04-27T22:04:10Z

Instance::expect_resolve ensures that we're actually going from trait item -> impl item.

rustbot · 2025-04-27T22:04:14Z

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

rustbot · 2025-04-27T22:04:16Z

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

lcnr · 2025-04-28T14:07:22Z

r=me after comment/rename on Instance::new

rustbot · 2025-04-28T14:41:17Z

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri, @RalfJung, @oli-obk, @lcnr

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

Some changes occurred in coverage instrumentation.

cc @Zalathar

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

compiler-errors · 2025-04-28T15:24:39Z

@bors r=lcnr rollup

bors · 2025-04-28T15:24:42Z

📌 Commit 844b1e2 has been approved by lcnr

It is now in the queue for this repository.

RalfJung · 2025-04-28T15:27:14Z

- pub fn new(def_id: DefId, args: GenericArgsRef<'tcx>) -> Instance<'tcx> {
+ /// Creates a new [`InstanceKind::Item`] from the `def_id` and `args`. Note that this
+ /// does not resolve trait items to their corresponding impl items -- to do that, use
+ /// [`Instance::expect_resolve`] instead.


As a compiler dev I would be rather clueless about whether I want to "resolve trait items to their corresponding impl items" or not -- I'm not even sure what exactly this means. So I'm afraid this comment isn't very helpful in terms of giving guidance for when I should use which of these functions.

Do you have a suggestion for a new comment? Because just saying it is not very helpful as a comment is... well, also not very actionable 😆

See the comment on https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/struct.Instance.html#method.try_resolve to understand what it means to resolve a trait item to its corresponding impl item.

I guess what matters is: "Are you certain that the body of def_id is the actual body you want to use. This is not the case for trait methods or functions we may stub out with a separate implementation"

Do you have a suggestion for a new comment? Because just saying it is not very helpful as a comment is... well, also not very actionable 😆
I'm not sure how to suggest a better comment without having any clue about when this function should or should not be used.^^ You made const-eval use new_raw so I guess it is the right thing to use for anything I encountered in the compiler so far...

You made const-eval use new_raw so I guess it is the right thing to use for anything I encountered in the compiler so far...
CTFE uses Instance::expect_resolve all throughout, so that's definitely not true. The only usage sites I've touched are the ones that attempt to directly evaluate const bodies.

I guess what matters is: "Are you certain that the body of def_id is the actual body you want to use. This is not the case for trait methods or functions we may stub out with a separate implementation"
That sounds helpful. Are there docs for the concept of "resolve" that we can link to? I guess it refers to going from a trait-method pair to the actual impl? Also, is there ever a downside to using resolve when we don't have to?
new_raw is scary as a name so now I feel like every use of this function needs a comment justifying it, but I'm not sure what the justification looks like in the query wrappers.

Also, is there ever a downside to using resolve when we don't have to?
Query calls, so perf. I'll open a follow-up to reduce the number of new_raw calls b/c it'll need a perf run and I see no reason to block this PR unnecessarily, but yes, the name was intentionally chosen to discourage usage of this function and to prompt compiler devs to question whether it's the appropriate function to call -- it's the root cause of this bug after all.

compiler-errors · 2025-04-28T15:48:43Z

@bors r=lcnr

bors · 2025-04-28T15:48:46Z

📌 Commit 9ac697f has been approved by lcnr

It is now in the queue for this repository.

RalfJung · 2025-04-28T15:53:11Z

+ /// implementation, and which may not even have a body themselves. Usages of
+ /// this function should probably use [`Instance::expect_resolve`], or if run
+ /// in a polymorphic environment or within a lint (that may encounter ambiguity)
+ /// [`Instance::expect_resolve`] instead.


Suggested change
/// implementation, and which may not even have a body themselves. Usages of
/// this function should probably use [`Instance::expect_resolve`], or if run
/// in a polymorphic environment or within a lint (that may encounter ambiguity)
/// [`Instance::expect_resolve`] instead.
/// implementation, and which may not even have a body themselves. Instead of
/// this function, you probably should use use [`Instance::expect_resolve`], or if run
/// in a polymorphic environment or within a lint (that may encounter ambiguity)
/// [`Instance::expect_resolve`] instead.
Also, this recommends expect_resolve twice now...

compiler-errors · 2025-04-28T16:20:56Z

@bors r=lcnr rollup

bors · 2025-04-28T16:20:58Z

📌 Commit 8f55844 has been approved by lcnr

It is now in the queue for this repository.

…lcnr Resolve instance for SymFn in global/naked asm `Instance::expect_resolve` ensures that we're actually going from trait item -> impl item. Fixesrust-lang#140373

…enton Rollup of 9 pull requests Successful merges: - rust-lang#139308 (add autodiff inline) - rust-lang#140276 (Do not compute type_of for impl item if impl where clauses are unsatisfied) - rust-lang#140302 (Move inline asm check to typeck, properly handle aliases) - rust-lang#140323 (Implement the internal feature `cfg_target_has_reliable_f16_f128`) - rust-lang#140374 (Resolve instance for SymFn in global/naked asm) - rust-lang#140391 (Rename sub_ptr to offset_from_unsigned in docs) - rust-lang#140394 (Make bootstrap git tests more self-contained) - rust-lang#140396 (Workaround for windows-gnu rust-lld test failure) - rust-lang#140402 (only return nested goals for `Certainty::Yes`) r? `@ghost` `@rustbot` modify labels: rollup

ChrisDenton · 2025-04-28T21:45:08Z

failed in rollup #140411 (comment)

@bors r-

…w, r=<try> Less `Instance::new_raw` For perf. Based on rust-lang#140374. r? `@ghost`

…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#134232 (Share the naked asm impl between cg_ssa and cg_clif) - rust-lang#139624 (Don't allow flattened format_args in const.) - rust-lang#140090 (Check bare function idents for non snake-case name) - rust-lang#140203 (Issue an error when using `no_mangle` on language items) - rust-lang#140450 (ast: Remove token visiting from AST visitor) - rust-lang#140498 (Misc tweaks to HIR typeck (mostly w.r.t. checking calls)) - rust-lang#140504 (transmutability: ensure_sufficient_stack when answering query) - rust-lang#140506 (unstable-book: fix capitalization) - rust-lang#140516 (Replace use of rustc_type_ir by rustc_middle) Failed merges: - rust-lang#140374 (Resolve instance for SymFn in global/naked asm) r? `@ghost` `@rustbot` modify labels: rollup

bors · 2025-04-30T21:37:49Z

☔ The latest upstream changes (presumably #140520) made this pull request unmergeable. Please resolve the merge conflicts.

compiler-errors · 2025-04-30T23:17:24Z

@bors r=lcnr

bors · 2025-04-30T23:17:26Z

📌 Commit 0b3b674 has been approved by lcnr

It is now in the queue for this repository.

compiler-errors · 2025-05-01T16:24:52Z

@bors r-

compiler-errors · 2025-05-05T14:47:07Z

@bors r=lcnr

bors · 2025-05-05T14:47:10Z

📌 Commit 833c212 has been approved by lcnr

It is now in the queue for this repository.

…lcnr Resolve instance for SymFn in global/naked asm `Instance::expect_resolve` ensures that we're actually going from trait item -> impl item. Fixesrust-lang#140373

…llaumeGomez Rollup of 11 pull requests Successful merges: - rust-lang#139764 (Consistent trait bounds for ExtractIf Debug impls) - rust-lang#140035 (Implement RFC 3503: frontmatters) - rust-lang#140080 (mir-opt: Use one MirPatch in MatchBranchSimplification) - rust-lang#140115 (mir-opt: execute MatchBranchSimplification after GVN) - rust-lang#140357 (bypass linker configuration and cross target check on `x check`) - rust-lang#140374 (Resolve instance for SymFn in global/naked asm) - rust-lang#140393 (std: get rid of `sys_common::process`) - rust-lang#140532 (Fix RustAnalyzer discovery of rustc's `stable_mir` crate) - rust-lang#140559 (Removing rustc_type_ir in the rustc_infer codebase) - rust-lang#140636 (implement `PanicTracker` to track `t` panics) - rust-lang#140661 (Make `-Zfixed-x18` into a target modifier) r? `@ghost` `@rustbot` modify labels: rollup

…llaumeGomez Rollup of 11 pull requests Successful merges: - rust-lang#140080 (mir-opt: Use one MirPatch in MatchBranchSimplification) - rust-lang#140115 (mir-opt: execute MatchBranchSimplification after GVN) - rust-lang#140357 (bypass linker configuration and cross target check on `x check`) - rust-lang#140374 (Resolve instance for SymFn in global/naked asm) - rust-lang#140559 (Removing rustc_type_ir in the rustc_infer codebase) - rust-lang#140605 (`fn check_opaque_type_parameter_valid` defer error) - rust-lang#140636 (implement `PanicTracker` to track `t` panics) - rust-lang#140661 (Make `-Zfixed-x18` into a target modifier) - rust-lang#140670 (calculate step duration in a panic-safe way) - rust-lang#140672 (Deeply normalize in the new solver in WF) - rust-lang#140676 (Update books) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#140374 - compiler-errors:global_asm-bug, r=lcnr Resolve instance for SymFn in global/naked asm `Instance::expect_resolve` ensures that we're actually going from trait item -> impl item. Fixesrust-lang#140373

rustbot assigned davidtwco Apr 27, 2025

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 Apr 27, 2025

lcnr reviewed Apr 28, 2025
View reviewed changes

Comment threadtests/ui/asm/naked-asm-mono-sym-fn.rs Outdated

lcnr assigned lcnr and unassigned davidtwco Apr 28, 2025

compiler-errors force-pushed the global_asm-bug branch from 3d7402b to 9cb3727Compare April 28, 2025 14:41

compiler-errors force-pushed the global_asm-bug branch from 9cb3727 to 844b1e2Compare April 28, 2025 14:44

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 28, 2025

RalfJung reviewed Apr 28, 2025
View reviewed changes

compiler-errors force-pushed the global_asm-bug branch 3 times, most recently from d30ad80 to 9ac697fCompare April 28, 2025 15:48

RalfJung reviewed Apr 28, 2025
View reviewed changes

compiler-errors force-pushed the global_asm-bug branch from 9ac697f to 8f55844Compare April 28, 2025 15:54

ChrisDenton mentioned this pull request Apr 28, 2025
Rollup of 9 pull requests #140411
Closed

bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 30, 2025
Auto merge ofrust-lang#140518- compiler-errors:less-instance-new-ra…
31d35eb
…w, r=<try> Less `Instance::new_raw` For perf. Based on rust-lang#140374. r? `@ghost`

matthiaskrgr mentioned this pull request Apr 30, 2025
Rollup of 9 pull requests #140520
Merged

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 30, 2025

compiler-errors force-pushed the global_asm-bug branch from 30d1fcb to 0b3b674Compare April 30, 2025 23:17

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 30, 2025

This comment has been minimized.
Sign in to view

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 1, 2025

compiler-errors added 2 commits May 5, 2025 13:17

Resolve instance for SymFn in global/naked asm
3a1ee64

Rename Instance::new to Instance::new_raw and add a note that it is raw
833c212

compiler-errors force-pushed the global_asm-bug branch from 0b3b674 to 833c212Compare May 5, 2025 14:46

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 5, 2025

GuillaumeGomez mentioned this pull request May 5, 2025
Rollup of 11 pull requests #140674
Closed

GuillaumeGomez mentioned this pull request May 5, 2025
Rollup of 11 pull requests #140682
Merged

bors merged commit 1e90557 into rust-lang:masterMay 6, 2025

rustbot added this to the 1.88.0 milestone May 6, 2025

Uh oh!

Uh oh!

Conversation

compiler-errors commented Apr 27, 2025

Uh oh!

rustbot commented Apr 27, 2025

Uh oh!

rustbot commented Apr 27, 2025

Uh oh!

Uh oh!

lcnr commented Apr 28, 2025

Uh oh!

rustbot commented Apr 28, 2025

Uh oh!

compiler-errors commented Apr 28, 2025

Uh oh!

bors commented Apr 28, 2025

Uh oh!

RalfJungApr 28, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

compiler-errorsApr 28, 2025

Choose a reason for hiding this comment

Uh oh!

compiler-errorsApr 28, 2025

Choose a reason for hiding this comment

Uh oh!

lcnrApr 28, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RalfJungApr 28, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

compiler-errorsApr 28, 2025

Choose a reason for hiding this comment

Uh oh!

RalfJungApr 28, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

compiler-errorsApr 28, 2025

Choose a reason for hiding this comment

Uh oh!

compiler-errors commented Apr 28, 2025

Uh oh!

bors commented Apr 28, 2025

Uh oh!

RalfJungApr 28, 2025

Choose a reason for hiding this comment

Uh oh!

compiler-errors commented Apr 28, 2025

Uh oh!

bors commented Apr 28, 2025

Uh oh!

ChrisDenton commented Apr 28, 2025

Uh oh!

bors commented Apr 30, 2025

Uh oh!

compiler-errors commented Apr 30, 2025

Uh oh!

bors commented Apr 30, 2025

Uh oh!

This comment has been minimized.

compiler-errors commented May 1, 2025

Uh oh!

compiler-errors commented May 5, 2025

Uh oh!

bors commented May 5, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

RalfJungApr 28, 2025•
edited
Loading

lcnrApr 28, 2025•
edited
Loading

RalfJungApr 28, 2025•
edited
Loading

RalfJungApr 28, 2025•
edited
Loading