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

Rust can't infer appropriate generics for function when it should #89242

Closed
TheRawMeatball opened this issue Sep 25, 2021 · 8 comments
Closed
Labels
C-bug Category: This is a bug. regression-from-stable-to-stable Performance or correctness regression from one stable version to another. S-has-mcve Status: A Minimal Complete and Verifiable Example has been found for this issue T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Milestone

Comments

@TheRawMeatball
Copy link

TheRawMeatball commented Sep 25, 2021

I was working on some generic code when I hit this bug, so I made this MRE:

use std::borrow::Borrow;

trait TraitA<'a> {
    type Return;
    fn get(&'a self) -> Self::Return;
}
trait TraitB<T>
where
    for<'a> <Self::TrA as TraitA<'a>>::Return: Borrow<T>,
{
    type TrA: for<'a> TraitA<'a>;
}
fn takes_trait_b<T: TraitB<String>>(x: T)
where
    for<'a> <T::TrA as TraitA<'a>>::Return: Borrow<String>,
{
}

impl<T: 'static> TraitB<T> for T {
    type TrA = Returner<T>;
}

struct Returner<T>(T);
impl<'a, T: 'static> TraitA<'a> for Returner<T> {
    type Return = &'a T;

    fn get(&'a self) -> Self::Return {
        &self.0
    }
}

fn test() {
    takes_trait_b::<String>(String::new());
    takes_trait_b(String::new());
}

(playground link: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=ce2ea68678f61b329a6cec59310c1aea)

I'd expect both calls to takes_trait_b to succeed, but the second call doesn't as rust seemingly tries to uphold the where clause for any possible T

This has been observed on rustc 1.57.0-nightly (aa8f2d432 2021-09-18)

Adding two extra where clauses makes both calls compile on stable, but this example still fails on nightly: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=aaa115e139635787f9b033a1418c7f06

Seems like a regression.

@TheRawMeatball TheRawMeatball added the C-bug Category: This is a bug. label Sep 25, 2021
@Mark-Simulacrum Mark-Simulacrum added this to the 1.56.0 milestone Sep 25, 2021
@Mark-Simulacrum Mark-Simulacrum added the regression-from-stable-to-beta Performance or correctness regression from stable to beta. label Sep 25, 2021
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Sep 25, 2021
@Mark-Simulacrum Mark-Simulacrum added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Sep 25, 2021
@Mark-Simulacrum
Copy link
Member

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=aaa115e139635787f9b033a1418c7f06 is failing on beta but working on stable, so tagging as a beta regression.

@camelid
Copy link
Member

camelid commented Sep 26, 2021

Seems like it could be related to #85499, so cc @jackh726.

@jackh726
Copy link
Member

Looking into this. From what I can tell, the version of this without the where clause doesn't compile on stable nor nightly (though the error is slightly different). However, while the version with the where clauses does compile on stable, it's actually not callable (because we can prove the where clause). On nightly, we just get a more eager error.

This is still a bug (and I'm looking into it), but I don't think it's strictly a regression. You can't actually use any less code than before.

@Mark-Simulacrum
Copy link
Member

This is still a bug (and I'm looking into it), but I don't think it's strictly a regression. You can't actually use any less code than before.

I think this certainly mitigates the likely impact of the regression, but it wouldn't surprise me too much if there exists "real world" code that is just uncallable (and not tested, so not discovered yet).

I think if we can fix the regression on nightly and backport it, that would be ideal.

@jackh726
Copy link
Member

Okay, so I investigated this a bit. It's kind of rough.

My work-through
  • We start by checking the expr takes_trait_b(()) with NoExpectation
  • Because the function takes one generic, we create a new variable _#0t
  • We then add the required obligations:
    • We care about Binder(TraitPredicate(<<<_#0t as TraitB<()>>::TrA as TraitA<ReLateBound(DebruijnIndex(0), BoundRegion { var: 0, kind: BrNamed(DefId(0:16 ~ issue_89242[81f3]::takes_trait_b::'a), 'a) })>>::Return as std::borrow::Borrow<()>>), [Region(BrNamed(DefId(0:16 ~ issue_89242[81f3]::takes_trait_b::'a), 'a))])
    • We normalize this, generating Binder(TraitPredicate(<<_#2t as TraitA<ReLateBound(DebruijnIndex(0), BoundRegion { var: 0, kind: BrNamed(DefId(0:16 ~ issue_89242[81f3]::takes_trait_b::'a), 'a) })>>::Return as std::borrow::Borrow<()>>), [Region(BrNamed(DefId(0:16 ~ issue_89242[81f3]::takes_trait_b::'a), 'a))])
      • And the following obligations:
        • Binder(ProjectionPredicate(ProjectionTy { substs: [_#0t, ()], item_def_id: DefId(0:12 ~ issue_89242[81f3]::TraitB::TrA) }, _#1t), []) (this one is unused, I think)
        • Binder(ProjectionPredicate(ProjectionTy { substs: [_#0t, ()], item_def_id: DefId(0:12 ~ issue_89242[81f3]::TraitB::TrA) }, _#2t), [])
        • Importantly, we can't replace <_#2t as TraitA<ReLateBound(DebruijnIndex(0), BoundRegion { var: 0, kind: BrNamed(DefId(0:16 ~ issue_89242[81f3]::takes_trait_b::'a), 'a) })>>::Return with an inference var + obligation, because we reference a late-bound region
  • We find that the type of the expr is fn(_#0t) {takes_trait_b::<_#0t>}
  • We then try to get the expr type, by calling structurally_resolved_type
    • That calls resolve_vars_with_obligations
    • Which calls select_obligations_where_possible
  • We try to select Binder(TraitPredicate(<<_#2t as TraitA<ReLateBound(DebruijnIndex(0), BoundRegion { var: 0, kind: BrNamed(DefId(0:16 ~ issue_89242[81f3]::takes_trait_b::'a), 'a) })>>::Return as std::borrow::Borrow<()>>), [Region(BrNamed(DefId(0:16 ~ issue_89242[81f3]::takes_trait_b::'a), 'a))])
    • "failed eq_trait_refs due to expected associated type, found &mut _#3t``"
    • "failed eq_trait_refs due to expected (), found associated type"
    • "failed eq_trait_refs due to expected associated type, found &_#3t``"
  • None of these are ambiguous, so we say that this trait is unimplemented
    • We then error
  • Normally, we just wouldn't make progress and then in confirm_builtin_call we would call check_argument_types, which then we relate the inputs
    • We then could make progress because we would set _#2t to ()

Basically, are potentially four ways forward

  1. Revert Normalize projections under binders #85499. Imo, this is a non-starter. That PR solves a lot of issues and the severity of this issue is imo low or medium, given that I don't think usable code could be created before anyways.
  2. Lazy normalization. One aspect of the problem essentially boils down to us trying to equate a projection with something else. I'll put "being able to be defer projecting a projection when there a bound vars i.e. using an inference var" under this, since I think ultimately lazy normalization will solve both issues.
  3. Make structurally_resolved_type not need to select obligations where possible, or otherwise just skip Unimplemented. Not sure this will work. Just commenting out select_where_possible doesn't build core.
  4. Rearrange the type checking code here to be able to subtype the input args with the new substs of the expr. Again, not sure if this is possible. The current code is set up in such a way that we can do autoderef first before doing expectation.

@apiraino
Copy link
Contributor

apiraino commented Oct 7, 2021

Discussed during the T-compiler triage meetings here and here. Consensus seems to be that it can be let slip.

@rustbot label -I-nominated

@apiraino apiraino removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Oct 14, 2021
@pietroalbini pietroalbini added regression-from-stable-to-stable Performance or correctness regression from one stable version to another. and removed regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels Jan 8, 2022
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Jan 8, 2022
@camelid camelid removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Jan 11, 2022
@jackh726
Copy link
Member

Stumbled across this again. This is basically #89196, because we try to prove the bound before we complete function resolution. It's a little different though, so I'll mark this to get included as a known-bug.

@jackh726 jackh726 added the S-has-mcve Status: A Minimal Complete and Verifiable Example has been found for this issue label Nov 22, 2022
@lolbinarycat
Copy link
Contributor

triage: closing, the linked MCVE now compiles properly on stable and nightly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. regression-from-stable-to-stable Performance or correctness regression from one stable version to another. S-has-mcve Status: A Minimal Complete and Verifiable Example has been found for this issue T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

8 participants