Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Symbol ambiguity with polymorphization and new mangling scheme #75326

Closed
davidtwco opened this issue Aug 9, 2020 · 2 comments · Fixed by #75518
Closed

Symbol ambiguity with polymorphization and new mangling scheme #75326

davidtwco opened this issue Aug 9, 2020 · 2 comments · Fixed by #75518
Labels
-Zpolymorphize Unstable option: Polymorphization.

Comments

@davidtwco
Copy link
Member

rustc will not currently bootstrap with -Zsymbol-mangling-version=v0 and -Zpolymorphization=on due to the following issue:

Consider the following example:

// build-pass
// compile-flags: -Zpolymorphize=on -Zsymbol-mangling-version=v0

pub(crate) struct Foo<'a, I, E>(I, &'a E);

impl<'a, I, T: 'a, E> Iterator for Foo<'a, I, E>
where
    I: Iterator<Item = &'a (T, E)>,
{
    type Item = T;

    fn next(&mut self) -> Option<Self::Item> {
        self.find(|_| true)
    }
}

fn main() {
    let mut a = Foo([(1u32, 1u16)].iter(), &1u16);
    let mut b = Foo([(1u16, 1u32)].iter(), &1u32);
    let _ = a.next();
    let _ = b.next();
}

..which produces the following error:

error: symbol `_RNCNvXCs4fqI2P2rA04_19impl_param_manglingINtB4_3FooppENtNtNtNtCsfnEnqCNU58Z_4core4iter6traits8iterator8Iterator4next0B4_` is already defined
  --> src/test/ui/polymorphization/impl-param-mangling.rs:13:19
   |
13 |         self.find(|_| true)
   |                   ^^^^^^^^

error: aborting due to previous error

(unmangled: <impl_param_mangling::Foo<_, _> as core::iter::traits::iterator::Iterator>::next::{closure#0})

Polymorphisation correctly determines that I and E are unused and that T is used in the closure (T is used by the argument). Therefore, multiple copies of this closure can be generated by polymorphization for each instantiation of T - however, this is not encoded in the mangling for the closure, which triggers assert_symbols_are_distinct.

@davidtwco davidtwco added the -Zpolymorphize Unstable option: Polymorphization. label Aug 9, 2020
@tesuji
Copy link
Contributor

tesuji commented Aug 9, 2020

cc @eddyb as maintainer of new mangling scheme.

@eddyb
Copy link
Member

eddyb commented Aug 9, 2020

Polymorphisation correctly determines that I and E are unused and that T is used in the closure (T is used by the argument).

Incredible, I wish I thought of this when the polymorphization work was ongoing, I guess I was overfocusing on shims.

So the problem is T is an alias for I::Item, not an "independent variable", the only reason it appears to work in other situations is the trait system is feeding you the value of T. Probably the simplest hackaround is to mangle trait impl "parameters".

But it seems more prudent to consider any parameter not named in the TraitRef as depending on all parameters that do appear in the TraitRef (or you can try to do something more clever by looking at the where clauses).

@bors bors closed this as completed in 1e58871 Aug 15, 2020
JohnTitor added a commit to JohnTitor/rust that referenced this issue Oct 15, 2020
…ms, r=eddyb

mangling: mangle impl params w/ v0 scheme

This PR modifies v0 symbol mangling to include all generic parameters from impl blocks (not just those used in the self type) - an alternative fix to rust-lang#75326.

```
original:
   _RNCNvXCs4fqI2P2rA04_19impl_param_manglingINtB4_3FooppENtNtNtNtCsfnEnqCNU58Z_4core4iter6traits8iterator8Iterator4next0B4_
//        |------------ B4_ ----------------|
// _R (N C (N v (X (C ((s 4fqI2p2rA04_) 19impl_param_mangling)) (I (N t B4_ 3Foo) pp E) (N t (N t (N t (N t (C ((s fnEnqCNU58Z_) 4core)) 4iter) 6traits) 8iterator) 8Iterator)) 4next) 0) B4_

modified:
   _RNvXINICs4fqI2P2rA04_11issue_753260pppEINtB5_3FooppENtNtNtNtCsfnEnqCNU58Z_4core4iter6traits8iterator8Iterator4nextB5_
// _R (N v (X (I (N I (C ((s 4fqI2P2rA04_) 11issue_75326)) 0) ppp E) (I (N t B5_ 3Foo) pp E) (N t (N t (N t (N t (C ((s fnEnqCNU58Z_) 4core)) 4iter) 6traits) 8iterator) 8Iterator)) 4next) B5_
//            |     ^                                              |
//            |     |                                              |
//            |     new impl namespace                             |
```

~~Submitted as a draft as after some discussion w/ @eddyb, I'm going to do some investigation into (yet more alternative) changes to polymorphization that might remove the necessity for this.~~

r? @eddyb
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Oct 15, 2020
…ms, r=eddyb

mangling: mangle impl params w/ v0 scheme

This PR modifies v0 symbol mangling to include all generic parameters from impl blocks (not just those used in the self type) - an alternative fix to rust-lang#75326.

```
original:
   _RNCNvXCs4fqI2P2rA04_19impl_param_manglingINtB4_3FooppENtNtNtNtCsfnEnqCNU58Z_4core4iter6traits8iterator8Iterator4next0B4_
//        |------------ B4_ ----------------|
// _R (N C (N v (X (C ((s 4fqI2p2rA04_) 19impl_param_mangling)) (I (N t B4_ 3Foo) pp E) (N t (N t (N t (N t (C ((s fnEnqCNU58Z_) 4core)) 4iter) 6traits) 8iterator) 8Iterator)) 4next) 0) B4_

modified:
   _RNvXINICs4fqI2P2rA04_11issue_753260pppEINtB5_3FooppENtNtNtNtCsfnEnqCNU58Z_4core4iter6traits8iterator8Iterator4nextB5_
// _R (N v (X (I (N I (C ((s 4fqI2P2rA04_) 11issue_75326)) 0) ppp E) (I (N t B5_ 3Foo) pp E) (N t (N t (N t (N t (C ((s fnEnqCNU58Z_) 4core)) 4iter) 6traits) 8iterator) 8Iterator)) 4next) B5_
//            |     ^                                              |
//            |     |                                              |
//            |     new impl namespace                             |
```

~~Submitted as a draft as after some discussion w/ @eddyb, I'm going to do some investigation into (yet more alternative) changes to polymorphization that might remove the necessity for this.~~

r? @eddyb
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Oct 16, 2020
…ms, r=eddyb

mangling: mangle impl params w/ v0 scheme

This PR modifies v0 symbol mangling to include all generic parameters from impl blocks (not just those used in the self type) - an alternative fix to rust-lang#75326.

```
original:
   _RNCNvXCs4fqI2P2rA04_19impl_param_manglingINtB4_3FooppENtNtNtNtCsfnEnqCNU58Z_4core4iter6traits8iterator8Iterator4next0B4_
//        |------------ B4_ ----------------|
// _R (N C (N v (X (C ((s 4fqI2p2rA04_) 19impl_param_mangling)) (I (N t B4_ 3Foo) pp E) (N t (N t (N t (N t (C ((s fnEnqCNU58Z_) 4core)) 4iter) 6traits) 8iterator) 8Iterator)) 4next) 0) B4_

modified:
   _RNvXINICs4fqI2P2rA04_11issue_753260pppEINtB5_3FooppENtNtNtNtCsfnEnqCNU58Z_4core4iter6traits8iterator8Iterator4nextB5_
// _R (N v (X (I (N I (C ((s 4fqI2P2rA04_) 11issue_75326)) 0) ppp E) (I (N t B5_ 3Foo) pp E) (N t (N t (N t (N t (C ((s fnEnqCNU58Z_) 4core)) 4iter) 6traits) 8iterator) 8Iterator)) 4next) B5_
//            |     ^                                              |
//            |     |                                              |
//            |     new impl namespace                             |
```

~~Submitted as a draft as after some discussion w/ @eddyb, I'm going to do some investigation into (yet more alternative) changes to polymorphization that might remove the necessity for this.~~

r? @eddyb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-Zpolymorphize Unstable option: Polymorphization.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants