Fix v0 symbol mangling bug

[camelid]

Fixes #83611.

r? @jackh726

[camelid]

It seems @eddyb was suggesting a slightly different approach: #83611 (comment)

[camelid]

By the way, why do so many SymbolMangler functions return Result<T, !> instead of just T?

[camelid]

How do I add a test for this? I wasn't able to find any existing name mangling tests.

[tmiasko]

--emit llvm-ir output contains both mangled function name and demangled one (obtained from mangled one), so a codegen test should do just fine.

[jackh726]

This looks exactly like what I was expecting. Just needs a test (and comment).

[jackh726]

Also, based on @eddyb's comment, we should just do a simple validation here.

When we encounter an ExistentialPredicate::Trait, we should store that in a local variable last_trait_ref: Binder<'tcx, TraitRef> then when we see an ExistentialPredicate::Projection, we want to check that 1) the binders match and 2) the projection's trait is the same as the last trait

[camelid]

I added some assertions, are they what you expected? 65c04123f81

[camelid]

The assertions panic on the test case :/

thread 'rustc' panicked at 'assertion failed: `(left == right)`
  left: `std::ops::FnMut<(&u8,)>`,
 right: `std::ops::FnOnce<(&u8,)>`', compiler/rustc_symbol_mangling/src/v0.rs:507:21
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

error: internal compiler error: unexpected panic

note: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/issues/new?labels=C-bug%2C+I-ICE%2C+T-compiler&template=ice.md

note: rustc 1.53.0-dev running on x86_64-apple-darwin

note: compiler flags: -Z symbol-mangling-version=v0

query stack during panic:
#0 [symbol_name] computing the symbol for `f::<&dyn for<'r> std::ops::FnMut(&'r u8)>`
#1 [collect_and_partition_mono_items] collect_and_partition_mono_items
end of query stack

I think the issue is that the projection for the FnMut arguments is actually projecting out of FnOnce, since FnOnce is a supertrait of FnMut. What should I do?

[jackh726]

So, you should be able to get the trait that defines the associated type by

traits::supertraits(tcx, trait_ref).filter(|r| {
  tcx
    .associated_items(r.def_id())
    .find_by_name_and_kind(tcx, assoc_name, ty::AssocKind::Type, r.def_id())
    .is_some()
}).next()

[camelid]

So, you should be able to get the trait that defines the associated type by

traits::supertraits(tcx, trait_ref).filter(|r| {
  tcx
    .associated_items(r.def_id())
    .find_by_name_and_kind(tcx, assoc_name, ty::AssocKind::Type, r.def_id())
    .is_some()
}).next()

It looks like in order to do that I would need to add a dependency on rustc_infer. Is that okay to do?

[eddyb]

It seems @eddyb was suggesting a slightly different approach: #83611 (comment)

Please add my example as a test - I don't see how in_binder can correctly work without being around everything it applies to.

r? @nikomatsakis

[camelid]

Please add my example as a test - I don't see how in_binder can correctly work without being around everything it applies to.

Sure, I'm happy to add your test case, but I have never written a codegen test before and am new to the symbol mangling code, so I would appreciate extra help :)

[tmiasko]

Codegen tests are located in src/test/codegen. They use LLVM FileCheck https://www.llvm.org/docs/CommandGuide/FileCheck.html to verify output. In this case I would check that LLVM IR contains correctly demangled names in the comments:

// compile-flags: -Zsymbol-mangling-version=v0 -Cno-prepopulate-passes 

pub fn test<T>() {}

fn main() {
    // CHECK: ; call a::test::<&dyn for<'a> core::ops::function::FnMut<(&'a u8,), Output = ()>>
    test::<&dyn FnMut(&u8)>();
    // CHECK: ; call a::test::<for<'a> fn(&'a dyn for<'b> core::ops::function::FnOnce<(&'b u8,), Output = &'b u8> + 'a) -> &'a u8>
    test::<for<'a> fn(&'a dyn for<'b> FnOnce(&'b u8) -> &'b u8) -> &'a u8>();
}

You can cross verify output with a toolchain before refactoring, e.g., d23e08448332425a84ae23124bea4dbd685536ce.

[camelid]

So, you should be able to get the trait that defines the associated type by

traits::supertraits(tcx, trait_ref).filter(|r| {
  tcx
    .associated_items(r.def_id())
    .find_by_name_and_kind(tcx, assoc_name, ty::AssocKind::Type, r.def_id())
    .is_some()
}).next()

That's giving errors:

error[E0308]: mismatched types
   --> compiler/rustc_symbol_mangling/src/v0.rs:511:55
    |
511 |                         traits::supertraits(self.tcx, projection.trait_ref(self.tcx)).find(|r| {
    |                                                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected struct `Binder`, found struct `ExistentialTraitRef`
    |
    = note: expected struct `Binder<'_, rustc_middle::ty::TraitRef<'_>>`
               found struct `ExistentialTraitRef<'_>`

error[E0308]: mismatched types
   --> /Users/user/rust4/library/core/src/macros/mod.rs:39:35
    |
35  | / macro_rules! assert_eq {
36  | |     ($left:expr, $right:expr $(,)?) => ({
37  | |         match (&$left, &$right) {
38  | |             (left_val, right_val) => {
39  | |                 if !(*left_val == *right_val) {
    | |                                   ^^^^^^^^^^ expected struct `ExistentialTraitRef`, found enum `Option`
...   |
61  | |     });
62  | | }
    | |_- in this expansion of `assert_eq!`
    | 
   ::: compiler/rustc_symbol_mangling/src/v0.rs:522:21
    |
522 |                       assert_eq!(last_trait_ref.skip_binder(), assoc_item_parent_trait);
    |                       ------------------------------------------------------------------ in this macro invocation
    |
    = note: expected struct `ExistentialTraitRef<'_>`
                 found enum `Option<Binder<'_, rustc_middle::ty::TraitRef<'_>>>`

error: aborting due to 2 previous errors

[jackh726]

You'll have to convert the ExistentialTraitRef to a normal one via with_self_ty.

[eddyb]

@camelid @tmiasko We don't use codegen tests for testing symbol names.
Check out src/test/ui/symbol-names instead, e.g:

rust/src/test/ui/symbol-names/impl1.rs

Lines 61 to 73 in 5b0ab79

	// Test type mangling, by putting them in an `impl` header.
	impl Bar for [&'_ (dyn Foo<Assoc = extern "C" fn(&u8, ...)> + AutoTrait); 3] {
	#[rustc_symbol_name]
	//[legacy]~^ ERROR symbol-name(_ZN209_$LT$$u5b$$RF$dyn$u20$impl1..Foo$u2b$Assoc$u20$$u3d$$u20$extern$u20$$u22$C$u22$$u20$fn$LP$$RF$u8$C$$u20$...$RP$$u2b$impl1..AutoTrait$u3b$$u20$3$u5d$$u20$as$u20$impl1..main..$u7b$$u7b$closure$u7d$$u7d$..Bar$GT$6method
	//[legacy]~| ERROR demangling(<[&dyn impl1::Foo+Assoc = extern "C" fn(&u8, ::.)+impl1::AutoTrait; 3] as impl1::main::{{closure}}::Bar>::method
	//[legacy]~| ERROR demangling-alt(<[&dyn impl1::Foo+Assoc = extern "C" fn(&u8, ::.)+impl1::AutoTrait; 3] as impl1::main::{{closure}}::Bar>::method)
	//[v0]~^^^^ ERROR symbol-name(_RNvXNCNvCs21hi0yVfW1J_5impl14mains_0ARDNtB6_3Foop5AssocFG_KCRL0_hvEuNtB6_9AutoTraitEL_j3_NtB2_3Bar6method)
	//[v0]~| ERROR demangling(<[&dyn impl1[17891616a171812d]::Foo<Assoc = for<'a> extern "C" fn(&'a u8, ...)> + impl1[17891616a171812d]::AutoTrait; 3: usize] as impl1[17891616a171812d]::main::{closure#1}::Bar>::method)
	//[v0]~| ERROR demangling-alt(<[&dyn impl1::Foo<Assoc = for<'a> extern "C" fn(&'a u8, ...)> + impl1::AutoTrait; 3] as impl1::main::{closure#1}::Bar>::method)
	#[rustc_def_path]
	//[legacy]~^ ERROR def-path(<[&dyn Foo<Assoc = for<'r> extern "C" fn(&'r u8, ...)> + AutoTrait; 3] as main::{closure#1}::Bar>::method)
	//[v0]~^^ ERROR def-path(<[&dyn Foo<Assoc = for<'r> extern "C" fn(&'r u8, ...)> + AutoTrait; 3] as main::{closure#1}::Bar>::method)
	fn method(&self) {}

These tests include demangling output, which is important to confirm correct roundtrip.

[michaelwoerister]

What's the state of this PR? Is a test case the only thing that remains to do?

[jackh726]

I think tests and a review from @nikomatsakis

[nikomatsakis]

@jackh726 I don't really understand what I'm meant to review, I think. =) Maybe we can touch on this tomorrow morning?

[jackh726]

@nikomatsakis yeah we can do that

[camelid]

Hmm, I can't quite get the code working. Here's my current version:

                ty::ExistentialPredicate::Projection(projection) => {
                    let assoc_item = self.tcx.associated_item(projection.item_def_id);

                    let last_trait_ref = last_trait_ref
                        .expect("trait predicate must come before projection predicate");
                    assert_eq!(last_trait_ref.bound_vars(), predicate.bound_vars());
                    // Use a type that can't appear in defaults of type parameters.
                    let dummy_self = self.tcx.mk_ty_infer(ty::FreshTy(0));
                    let assoc_item_parent_trait =
                        traits::supertraits(self.tcx, projection.trait_ref(self.tcx).with_self_ty(self.tcx, dummy_self)).find(|r| {
                            self.tcx
                                .associated_items(r.def_id())
                                .find_by_name_and_kind(
                                    self.tcx,
                                    assoc_item.ident,
                                    assoc_item.kind,
                                    r.def_id(),
                                )
                                .is_some()
                        }).unwrap();
                    assert_eq!(last_trait_ref.skip_binder(), ty::ExistentialTraitRef::erase_self_ty(self.tcx, assoc_item_parent_trait.skip_binder()));

                    let name = assoc_item.ident;
                    self.push("p");
                    self.push_ident(&name.as_str());
                    self = projection.ty.print(self)?;
                }

This code fails to compile:

error[E0308]: mismatched types
   --> compiler/rustc_symbol_mangling/src/v0.rs:513:55
    |
513 |                         traits::supertraits(self.tcx, projection.trait_ref(self.tcx).with_self_ty(self.tcx, dummy_self)).find(|r| {
    |                                                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected struct `Binder`, found struct `rustc_middle::ty::TraitRef`
    |
    = note: expected struct `Binder<'_, rustc_middle::ty::TraitRef<'_>, >`
               found struct `rustc_middle::ty::TraitRef<'_>`

Then I tried using Binder::dummy, but that causes the !has_escaping_bound_vars assertions to fail. So then I tried using Binder::bind, but that fails with

error: internal compiler error: compiler/rustc_middle/src/ty/fold.rs:824:17: Trying to collect bound vars with a bound region: DebruijnIndex(0) BoundRegion { var: 0, kind: BrAnon(0) }

thread 'rustc' panicked at 'Box<Any>', /Users/user/rust4/library/std/src/panic.rs:59:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

note: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/issues/new?labels=C-bug%2C+I-ICE%2C+T-compiler&template=ice.md

note: rustc 1.53.0-dev running on x86_64-apple-darwin

note: compiler flags: -Z threads=1 -Z ui-testing -Z deduplicate-diagnostics=no -Z emit-future-incompat-report -Z unstable-options -Z symbol-mangling-version=v0 -C prefer-dynamic -C rpath -C debuginfo=0

query stack during panic:
#0 [symbol_name] computing the symbol for `test::<&dyn for<'r> std::ops::FnMut(&'r u8)>`
#1 [collect_and_partition_mono_items] collect_and_partition_mono_items
end of query stack
error: aborting due to previous error

What should I do?

[jackh726]

Should be Binder::bind_with_vars(trait_ref, last_trait_ref.bound_vars())

[jackh726]

Might have a cleaner way to approach this. @nikomatsakis and I discussed, and would also definitely like to see the example in my comment as a test case too.

[jackh726]

@nikomatsakis this is ready :)

[jackh726]

Ugh don't remember how to make this reproducible locally

[camelid]

Is it expected that this changed from ::method to ::metSYMBOL_HASH?

EDIT: This is an outdated diff, but the same change seems to be present in the current diff as well.

[jackh726]

Oh this file shouldn't be here currently...(mostly because I didn't think hard enough about what regex to use here to make it reproducible and we don't care about legacy here anyways)

[nikomatsakis]

@bors r+

[bors]

📌 Commit 8345908 has been approved by nikomatsakis