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

Regression: "no method found" error when calling same method twice, with HRTB impl #37154

Closed
mbrubeck opened this issue Oct 13, 2016 · 6 comments
Assignees
Labels
P-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@mbrubeck
Copy link
Contributor

mbrubeck commented Oct 13, 2016

The following code (playground link) results in a method resolution error on the second call to x.method(). It compiles successfully if x.method() is called only once:

trait Foo {
    fn method(&self) {}
}

struct Wrapper<T>(T);

impl<T> Foo for Wrapper<T> where for<'a> &'a T: IntoIterator<Item=&'a ()> {}

fn f(x: Wrapper<Vec<()>>) {
    x.method(); // This works.
    x.method(); // error: no method named `method`
}

The error is:

error: no method named `method` found for type `Wrapper<std::vec::Vec<()>>` in the current scope
  --> <anon>:11:7
   |
11 |     x.method(); // error: no method named `method`
   |       ^^^^^^
   |
   = help: items from traits can only be used if the trait is implemented and in scope; the following trait defines an item `method`, perhaps you need to implement it:
   = help: candidate #1: `Foo`

@jonas-schievink suggested this might be a bug in the projection cache or some other cache.

This is a regression from Rust 1.10.0-stable to 1.11.0-stable.

@sfackler sfackler added regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 13, 2016
@jonas-schievink
Copy link
Contributor

jonas-schievink commented Oct 13, 2016

In case it's useful to anyone, here's a rustc::traits=DEBUG log at the current master (commit d34318d)

EDIT:

Here is the log with rustc_typeck::check::method=DEBUG,rustc::traits=DEBUG,rustc::middle=DEBUG and a small debug! in method probing added by me: https://gist.github.com/jonas-schievink/9551715c4f2bc05a8359e1dafddf7f8e

In particular, the first call causes this:

DEBUG:rustc_typeck::check::method::probe: applicable_candidates: [Candidate { xform_self_ty: &Wrapper<_>, item: ImplOrTraitItem(Method { name: method(69), generics: Generics { parent: Some(DefId { krate: CrateNum(0), node: DefIndex(4) => test/45447b7afbd5e544f7d0f1df0fccd26014d9850130abd3f020b89ff96b82079f-exe::Foo[0] }), parent_regions: 0, parent_types: 1, regions: [], types: [], has_self: true }, predicates: GenericPredicates([]), fty: BareFnTy { unsafety: Normal, abi: Rust, sig: Binder(([&Self]; variadic: false)->()) }, explicit_self: ByReference(ReLateBound(DebruijnIndex { depth: 1 }, BrAnon(0)), MutImmutable), vis: Restricted(NodeId(0)), defaultness: Default, has_body: true, def_id: DefId { krate: CrateNum(0), node: DefIndex(5) => test/45447b7afbd5e544f7d0f1df0fccd26014d9850130abd3f020b89ff96b82079f-exe::Foo[0]::method[0] }, container: TraitContainer(DefId { krate: CrateNum(0), node: DefIndex(4) => test/45447b7afbd5e544f7d0f1df0fccd26014d9850130abd3f020b89ff96b82079f-exe::Foo[0] }) }), kind: ExtensionImplCandidate(DefId { krate: CrateNum(0), node: DefIndex(11) => test/45447b7afbd5e544f7d0f1df0fccd26014d9850130abd3f020b89ff96b82079f-exe::{{impl}}[0] }, Substs { params: [_] }, []), import_id: None }]
DEBUG:rustc_typeck::check::method::confirm: confirm(unadjusted_self_ty=Wrapper<std::vec::Vec<()>>, pick=Pick { item: ImplOrTraitItem(Method { name: method(69), generics: Generics { parent: Some(DefId { krate: CrateNum(0), node: DefIndex(4) => test/45447b7afbd5e544f7d0f1df0fccd26014d9850130abd3f020b89ff96b82079f-exe::Foo[0] }), parent_regions: 0, parent_types: 1, regions: [], types: [], has_self: true }, predicates: GenericPredicates([]), fty: BareFnTy { unsafety: Normal, abi: Rust, sig: Binder(([&Self]; variadic: false)->()) }, explicit_self: ByReference(ReLateBound(DebruijnIndex { depth: 1 }, BrAnon(0)), MutImmutable), vis: Restricted(NodeId(0)), defaultness: Default, has_body: true, def_id: DefId { krate: CrateNum(0), node: DefIndex(5) => test/45447b7afbd5e544f7d0f1df0fccd26014d9850130abd3f020b89ff96b82079f-exe::Foo[0]::method[0] }, container: TraitContainer(DefId { krate: CrateNum(0), node: DefIndex(4) => test/45447b7afbd5e544f7d0f1df0fccd26014d9850130abd3f020b89ff96b82079f-exe::Foo[0] }) }), kind: ExtensionImplPick(DefId { krate: CrateNum(0), node: DefIndex(11) => test/45447b7afbd5e544f7d0f1df0fccd26014d9850130abd3f020b89ff96b82079f-exe::{{impl}}[0] }), import_id: None, autoderefs: 0, autoref: Some(MutImmutable), unsize: None }, supplied_method_types=[])

While the log contains 2 (not sure why not just one) occurrences of this (this logging statement was added by me, so don't go searching the logs from master), after the first call has been resolved:

DEBUG:rustc_typeck::check::method::probe: consider_probe: obligation does not hold: Obligation(predicate=Binder(ProjectionPredicate(ProjectionTy { trait_ref: <&'a std::vec::Vec<()> as std::iter::IntoIterator>, item_name: Item(75) }, &'a ())),depth=0)

Which then causes method::probe to drop the candidate on the ground.

@jonas-schievink
Copy link
Contributor

This, like #36325, was introduced between these two nightlies:

Works on nightly-2016-06-04

rustc 1.11.0-nightly (7de2e6dc8 2016-06-03)
binary: rustc
commit-hash: 7de2e6dc828473b60aefe4d2140a602cbeb6d6f9
commit-date: 2016-06-03
host: x86_64-unknown-linux-gnu
release: 1.11.0-nightly

Broken on nightly-2016-06-05

rustc 1.11.0-nightly (12238b984 2016-06-04)
binary: rustc
commit-hash: 12238b984abfacb2cccea176f862c94aa1231fb5
commit-date: 2016-06-04
host: x86_64-unknown-linux-gnu
release: 1.11.0-nightly

These regressions likely have the same cause. I suspect the projection cache, introduced in #33816.

cc @nikomatsakis

@arielb1
Copy link
Contributor

arielb1 commented Oct 14, 2016

The projection cache indeed looks like it.

@nikomatsakis
Copy link
Contributor

So yes did a bit of digging and the projection cache is indeed doing some inappropriate caching here. I have a hacky fix; currently debating about a mildly less hacky one. Still not overly happy with how we handle skolemization today but that's another story. =)

@nikomatsakis
Copy link
Contributor

Pending PR: #37294

@brson
Copy link
Contributor

brson commented Oct 20, 2016

Thanks @nikomatsakis !

@brson brson added the P-high High priority label Oct 20, 2016
Manishearth added a commit to Manishearth/rust that referenced this issue Oct 22, 2016
…sakis

remove keys w/ skolemized regions from proj cache when popping skolemized regions

This addresses rust-lang#37154 (a regression). The projection cache was incorrectly caching the results for skolemized regions -- when we pop skolemized regions, we are supposed to drop cache keys for them (just as we remove those skolemized regions from the region inference graph). This is because those skolemized region numbers will be reused later with different meaning (and we have determined that the old ones don't leak out in any meaningful way).

I did a *somewhat* aggressive fix here of only removing keys that mention the skolemized regions. One could imagine just removing all keys added since we started the skolemization (as indeed I did in my initial commit). This more aggressive fix required fixing a latent bug in `TypeFlags`, as an aside.

I believe the more aggressive fix is correct; clearly there can be entries that are unrelated to the skoelemized region, and it's a shame to remove them. My one concern was that it *is* possible I believe to have some region variables that are created and related to skolemized regions, and maybe some of them could end up in the cache. However, that seems harmless enough to me-- those relations will be removed, and couldn't have impacted how trait resolution proceeded anyway (iow, the cache entry is not wrong, though it is kind of useless).

r? @pnkfelix
cc @arielb1
bors added a commit that referenced this issue Oct 22, 2016
remove keys w/ skolemized regions from proj cache when popping skolemized regions

This addresses #37154 (a regression). The projection cache was incorrectly caching the results for skolemized regions -- when we pop skolemized regions, we are supposed to drop cache keys for them (just as we remove those skolemized regions from the region inference graph). This is because those skolemized region numbers will be reused later with different meaning (and we have determined that the old ones don't leak out in any meaningful way).

I did a *somewhat* aggressive fix here of only removing keys that mention the skolemized regions. One could imagine just removing all keys added since we started the skolemization (as indeed I did in my initial commit). This more aggressive fix required fixing a latent bug in `TypeFlags`, as an aside.

I believe the more aggressive fix is correct; clearly there can be entries that are unrelated to the skoelemized region, and it's a shame to remove them. My one concern was that it *is* possible I believe to have some region variables that are created and related to skolemized regions, and maybe some of them could end up in the cache. However, that seems harmless enough to me-- those relations will be removed, and couldn't have impacted how trait resolution proceeded anyway (iow, the cache entry is not wrong, though it is kind of useless).

r? @pnkfelix
cc @arielb1
@bors bors closed this as completed in 483bc86 Oct 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. 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

7 participants