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

HRTB impl selection does not cover type parameters not directly related to the trait. #30867

Closed
eddyb opened this issue Jan 13, 2016 · 13 comments · Fixed by #131355
Closed

HRTB impl selection does not cover type parameters not directly related to the trait. #30867

eddyb opened this issue Jan 13, 2016 · 13 comments · Fixed by #131355
Labels
A-higher-ranked Area: Higher-ranked things (e.g., lifetimes, types, trait bounds aka HRTBs) A-trait-system Area: Trait system A-type-system Area: Type system C-bug Category: This is a bug. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-types Relevant to the types team, which will review and decide on the PR/issue.

Comments

@eddyb
Copy link
Member

eddyb commented Jan 13, 2016

The following doesn't compile, as it tries to assign U a concrete &'x i32 instead of &'a i32:

trait Unary<T> {}
impl<T, U, F: Fn(T) -> U> Unary<T> for F {}
fn unary<F: for<'a> Unary<&'a T>, T>() {}

fn test<F: for<'a> Fn(&'a i32) -> &'a i32>() {
    unary::<F, i32>()
}

Removing the U type parameter makes it work - U is only used for the associated type Fn::Output anyway:

#![feature(unboxed_closures)]

trait Unary<T> {}
impl<T, F: Fn<(T,)>> Unary<T> for F {}
fn unary<F: for<'a> Unary<&'a T>, T>() {}

fn test<F: for<'a> Fn(&'a i32) -> &'a i32>() {
    unary::<F, i32>()
}

AFAICT, the selection results in for<'a, U> F: Fn<(&'a i32,), Output=U> in the first case, and for<'a> F: Fn<(&'a i32,)> in the second case, and we refuse to allow U to be parameterized by 'a in the former case, even though it's only used to satisfy the sugary form of the Fn trait.

@eddyb
Copy link
Member Author

eddyb commented Jan 13, 2016

@nikomatsakis Is this the same issue as with the impls for fn(T) -> U not matching for<'a> fn(&'a i32) -> &'a i32?

@nikomatsakis
Copy link
Contributor

I'm trying to decide if this is an actual bug :) It does seem like the "HR capture" logic is being unnecessarily conservative. Let me poke at the debug logs and see if that reveals to me why it is rejecting the impl when you include U but not if you don't (also, it works if you do Fn(T) -> T).

@eddyb
Copy link
Member Author

eddyb commented Jan 14, 2016

@nikomatsakis I believe it breaks for type parameters which cannot be extracted from the TraitRef, i.e. <Unary<T> as Fn<(T,)>>. You should be able to relate multiple argument types, but not the return type (because it's an associated type and not part of the TraitRef).

@nikomatsakis
Copy link
Contributor

Hmm, the problem is a bit of a subtle one and, in this particular example, tied to the associated type projection. But really I think it can occur with dependent where clauses of any kind. (No I don't, see update below.) No doubt this is already filed somewhere under another guise. I'm not sure of the best fix, but let me lay out how the problem arises in terms of what the compiler actually does.

We start out with a trait reference to solve:

for<'a> F: Unary<'a i32>

We match this against the impl

impl<T, U, F: Fn(T) -> U> Unary<T> for F {}

Which, internally, is lowered to:

impl<T, U, F> Unary<T> for F
    where F: Fn<(T,)>, <F as Fn<(T,)>>::Output = U

In so doing, we first "open" the for<'a> binder, so that the trait we are searching for becomes:

F: Unary<&'0 i32>

where '0 is a region variable. We then check whether we can match this against the impl in question. We can, with Self=F and T=&'0 i32. U is left unconstrained. The where clauses on the impl are always processed after we perform the initial match. So we wind up saying that the impl applies and that we now have various subobligations. The '0 region variable appears in some of those, and in that case we "regeneralize" by adding back a for<'a> binder so that '0 becomes a bound region again:

F: Sized
for<'a> F: Fn<(&'a i32,)>
for<'a> &'a i32: Sized
for<'a> <F as Fn<(&'a i32,)>>::Output == $U
$U: Sized

Note that $U is what was substituted for the variable U (but never constrained). And this is where the problem arises.

I think that the basic strategy here is somewhat wrong. It would be OK in the absence of variables like U, which do not appear in the trait reference but which are constrained by projections, but it's wrong now. The problem is that $U was created "inside" the binder along with with $T and $Self but, unlike those other variables, it was never constrained. Thus we kind of silently promote it to live outside the scope of the for, since it is completely unconstrained. This is not necessarily wrong I guess but it is overly conservative.

One possible fix here would be to recursively (and before returning) handle the projection constraints, so that U winds up being "determined" to be &'0 i32, which could then be generalized to &'a i32 wherever it appeared. That may be the best short term fix at least, though I am not so keen on it overall because I don't like doing work recursively like that.

If we had had lazy normalization, this would be much easier, because then we could just unify U with <F as Fn<(&'0 i32,)>>::Output, which would then be generalized to a bound region as needed. That's probably my preferred fix, presuming it can be made to work. This is basically the same as processing the projection constraint recursively, except that we could defer the actual normalization until later (and we may never have to do it).

(The other solutions I can imagine are quite a bit more complicated. It seems like you would need to capture, in the tree, what variables are bound where. What makes this hard is that $U is shared across two obligations.)

UPDATE: Remove sentence I don't think. I think that for this problem to arise you really need an associated type, since you need a type variable that is not found in the trait reference, which is otherwise verboten.

@nikomatsakis
Copy link
Contributor

cc @arielb1 -- this seems related to the changes you made to order projection predicates earlier in the list. Does my earlier comment make sense to you?

@arielb1
Copy link
Contributor

arielb1 commented Jan 14, 2016

@nikomatsakis

I am not sure we should not evaluate associated type projections recursively - RFC447 basically forces this to work.

Unfortunately, it will work imperfectly without lazy normalization, because we can't represent a constraint like for<'a> <_ as Fn<(&'a u32,)>>::Output: Sized.

@nikomatsakis
Copy link
Contributor

On Thu, Jan 14, 2016 at 12:54:36PM -0800, arielb1 wrote:

I am not sure we should not evaluate associated type projections recursively - RFC447 basically forces this to work.

Sorry, the double negative in this sentence is throwing me a bit. I'm
not sure I understand what you are saying. Are you saying my proposed
solution would or would not work? Also, when you say evaluate, I
presume you are not referring to the evaluate family of fns in
select but just using a generic verb.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Jan 15, 2016 via email

@arielb1
Copy link
Contributor

arielb1 commented Jan 15, 2016

Can you elaborate just a bit on this? When does this kind of constraint arise?

impl<T> Foo for Option<T> where
    T: Fn(&u32),
    for<'a> <T as Fn(&'a u32)>::Output: Sized
{}

// evaluate <Option<_> as Foo>

@arielb1
Copy link
Contributor

arielb1 commented Jan 15, 2016

Sorry, the double negative in this sentence is throwing me a bit. I'm
not sure I understand what you are saying. Are you saying my proposed
solution would or would not work?

Recursively applying constraining-projections during confirmation should work, except for the type-inference case.

@nikomatsakis
Copy link
Contributor

@arielb1

Sorry for not replying. I'm afraid I still don't quite follow you, so let me try and work it out. First off, I guess you meant to have impl<T>, like so, and probably Fn<(&'a u32,)>:

impl<T> Foo for Option<T> where
    for<'a> T: Fn<(&'a u32,)>,
    for<'a> <T as Fn<(&'a u32,)>>::Output: Sized
{}

// evaluate <Option<_> as Foo>

I'm not quite sure what the last line means, but if we were to follow the model of the original problem, we'd start with something to prove like for<'a> Option<&'a X>: Foo. "Opening" the for and matching this against the impl yields:

Option<$T> = Option<&'0 X>

and hence T = &'0 X. This suggests that we would translate the where clauses on the impl to:

for<'a> &'0 X: Fn<(&'a u32,)>
for<'a> <&'0 X as Fn<(&'a u32,)>>::Output: Sized

which we would could then cover back up as:

for<'a, 'b> &'b X: Fn<(&'a u32,)>
for<'a, 'b> <&'b X as Fn<(&'a u32,)>>::Output: Sized

This all seems...OK. Ah, wait, I guess your point is that we would need to normalize the <&'b X as Fn<(&'a u32,)>>::Output. I feel like we have issues normalizing under for binders still, right?

@asajeffrey
Copy link

I was bitten by this bug... simplifying a lot, my code looked like this (https://play.rust-lang.org/?gist=76fd1bad2b276b335db7):

trait Cast<T> {
    fn cast(self) -> T;
}

impl<'a> Cast<&'a str> for &'static str {
    fn cast(self) -> &'a str { self }
}

struct Const<S>(S);

pub trait IterFunction1<I> 
    where I: Iterator
{
    fn apply(self, iter: I) -> I::Item;
}

impl<I,S,T> IterFunction1<I> for Const<S> 
    where I: Iterator<Item = T>, S: Cast<T>
{
    fn apply(self, _: I) -> T { self.0.cast() }
}

but this isn't lifetime-polymporphic:

fn expects_polymorphic_function1<F>(f: F)
    where F: for<'a> IterFunction1<std::vec::Drain<'a,&'a str>> 
{
    let mut vec = vec!["abc"];
    f.apply(vec.drain(..));
}

fn main() {
    expects_polymorphic_function1(Const("hi"));
}

produces:

<anon>:56:5: 56:34 error: type mismatch resolving `for<'a> <collections::vec::Drain<'a, &'a str> as core::iter::Iterator>::Item == &str`:
 expected bound lifetime parameter 'a,
    found concrete lifetime [E0271]
<anon>:56     expects_polymorphic_function1(Const("hi"));
              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
<anon>:56:5: 56:34 help: see the detailed explanation for E0271
<anon>:56:5: 56:34 note: required by `expects_polymorphic_function1`

but if you make the iterator item type a type argument, then it all works:

pub trait IterFunction2<I,T> 
    where I: Iterator<Item = T>
{
    fn apply(self, iter: I) -> T;
}

impl<I,S,T> IterFunction2<I,T> for Const<S> 
    where I: Iterator<Item = T>, S: Cast<T>
{
    fn apply(self, _: I) -> T { self.0.cast() }
}

fn expects_polymorphic_function2<F>(f: F)
    where F: for<'a> IterFunction2<std::vec::Drain<'a,&'a str>, &'a str> 
{
    let mut vec = vec!["abc"];
    f.apply(vec.drain(..));
}

fn main() {
    expects_polymorphic_function2(Const("hi"));
}

so I need to rewrite a lot of code to take an extra type argument in order to be lifetime-polymorphic!

asajeffrey pushed a commit to asajeffrey/parsell that referenced this issue Feb 19, 2016
The problem is rust-lang/rust#30867, which means that any type that
wants to be polymorphic over `I: Iterator`, and lifetime-polymorpic over `I::Item` needs to take
`I::Item` as a type parameter.

Concretely, this means that `Stateful<Str>` where `Str: Iterator` needs to be replaced
by `Stateful<Ch, Str>` where `Str: Iterator<Item = Ch>`, and ditto the other types.

While I was doing a root-and-branch rewrite, I fixed issues #4 and #5.
@nikomatsakis nikomatsakis added the A-trait-system Area: Trait system label May 8, 2017
@Mark-Simulacrum Mark-Simulacrum added the C-bug Category: This is a bug. label Jul 24, 2017
alecmocatta added a commit to alecmocatta/office-hours that referenced this issue Sep 13, 2018
I'm working on data-parallelism library, heavily inspired by Rayon but focused on describing certain computations on a dataset in an SQL-like manner, somewhat akin to Diesel.

One part of this combines HRTBs with associated types. I've bumped into a few issues that have made things a bit tricky ([#30472](rust-lang/rust#30472), [#30867](rust-lang/rust#30867), [#53943](rust-lang/rust#53943), and similar), the hardest to work around however has been that I don't believe it's currently possible to write this:
```rust
type Task = for<'a> <B as Abc<&'a A::Item>>::Task;
```
Currently I've resorted to a dummy trait, manually reimplemented on various structs without the reference, such that I can do:
```rust
type Task = <B as AbcDummy<A::Item>>::Task;
```
and (horribly) transmute between the two.

I'd be very interested to discuss

 1) issues I've bumped into combining HRTBs and associated types;
 2) how to get rid of this transmute and the dummy trait;
 3) any feedback on the library before I publish and promote it, given it's heavily inspired by your work on Rayon!

I can do any of the Monday or Friday times listed, though I have a preference for the 16 - 16.30 slots.

Again, much appreciation for you doing this, I think it's awesome!
@jonas-schievink jonas-schievink added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 5, 2020
@kadiwa4
Copy link
Contributor

kadiwa4 commented Aug 22, 2023

Both the original example and asajeffrey's example compile now.

@fmease fmease added A-higher-ranked Area: Higher-ranked things (e.g., lifetimes, types, trait bounds aka HRTBs) E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. labels Sep 24, 2024
@fmease fmease added the T-types Relevant to the types team, which will review and decide on the PR/issue. label Sep 24, 2024
bors added a commit to rust-lang-ci/rust that referenced this issue Oct 7, 2024
Add tests for some old fixed issues

Closes rust-lang#30867
Closes rust-lang#30472
Closes rust-lang#28994
Closes rust-lang#26719 (and migrates the relevant test to the new run-make)
Closes rust-lang#23600

cc `@jieyouxu` for the run-make-support changes

try-job: x86_64-msvc
@bors bors closed this as completed in cc5a24c Oct 8, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Oct 8, 2024
Rollup merge of rust-lang#131355 - clubby789:old-tests, r=jieyouxu

Add tests for some old fixed issues

Closes rust-lang#30867
Closes rust-lang#30472
Closes rust-lang#28994
Closes rust-lang#26719 (and migrates the relevant test to the new run-make)
Closes rust-lang#23600

cc `@jieyouxu` for the run-make-support changes

try-job: x86_64-msvc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-higher-ranked Area: Higher-ranked things (e.g., lifetimes, types, trait bounds aka HRTBs) A-trait-system Area: Trait system A-type-system Area: Type system C-bug Category: This is a bug. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants