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

save-analysis: Pull associated type definition using `qpath_def` #59894

Merged
merged 2 commits into from Apr 12, 2019

Conversation

Projects
None yet
10 participants
@Xanewok
Copy link
Member

commented Apr 11, 2019

Closes rust-lang/rls#1390

This (probably?) fixes the case where we run the save-analysis code on the following snippet:

trait Test<'a> {
    type Thing: Test2;
}

trait Test2 {
    fn print();
}

#[allow(unused)]
fn example<T>(t: T)
    where T: for<'a> Test<'a>
{
    T::Thing::print(); //~ ERROR cannot extract an associated type from a higher-ranked trait bound in this context
    // ^ only errors in save-analysis mode
}

The chain is as follows:

  • culprit is hir_ty_to_ty
  • which calls ast_ty_to_ty in the ItemCtxt
  • which calls associated_path_to_ty
  • which finally fails on projected_ty_from_poly_trait_ref
    if let Some(trait_ref) = poly_trait_ref.no_bound_vars() {
    self.tcx().mk_projection(item_def_id, trait_ref.substs)
    } else {
    // no late-bound regions, we can just ignore the binder
    span_err!(
    self.tcx().sess,
    span,
    E0212,
    "cannot extract an associated type from a higher-ranked trait bound \
    in this context"
    );
    self.tcx().types.err
    }

I'm not exactly sure why hir_ty_to_ty fails - is it because it needs more setup from typeck to work correctly? I'm guessing the fix is okay since we just pull the already typeck'd data (we run save-analysis after all the analysis passes are complete) from the tables.

With this change we can 'go to def' on all segments in the T::Thing::print() path.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Apr 11, 2019

r? @cramertj

(rust_highfive has picked a reviewer for you, use r? to override)

@cramertj

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

r? @eddyb

@rust-highfive rust-highfive assigned eddyb and unassigned cramertj Apr 11, 2019

@@ -379,7 +379,7 @@ pub fn check_crate<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>)
}
}

/// A quasi-deprecated helper used in rustdoc and save-analysis to get
/// A quasi-deprecated helper used in rustdoc and clippy to get

This comment has been minimized.

Copy link
@eddyb

eddyb Apr 12, 2019

Member

cc @oli-obk @Manishearth We should maybe have an issue for getting clippy off of this.

This comment has been minimized.

Copy link
@Manishearth

Manishearth Apr 13, 2019

Member

cc @flip1995 in case this is on your list of clippy utilities

@eddyb

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2019

📌 Commit bcd263e has been approved by eddyb

Centril added a commit to Centril/rust that referenced this pull request Apr 12, 2019

Rollup merge of rust-lang#59894 - Xanewok:save-assoc-ty-qpath, r=eddyb
save-analysis: Pull associated type definition using `qpath_def`

Closes rust-lang/rls#1390

This (probably?) fixes the case where we run the save-analysis code on the following snippet:
```rust
trait Test<'a> {
    type Thing: Test2;
}

trait Test2 {
    fn print();
}

#[allow(unused)]
fn example<T>(t: T)
    where T: for<'a> Test<'a>
{
    T::Thing::print(); //~ ERROR cannot extract an associated type from a higher-ranked trait bound in this context
    // ^ only errors in save-analysis mode
}
```

The chain is as follows:
- culprit is `hir_ty_to_ty`
- which calls `ast_ty_to_ty` in the `ItemCtxt`
- which calls `associated_path_to_ty`
- which finally fails on `projected_ty_from_poly_trait_ref`
https://github.com/rust-lang/rust/blob/3de0106789468b211bcc3a25c09c0cf07119186d/src/librustc_typeck/collect.rs#L212-L224

I'm not exactly sure why `hir_ty_to_ty` fails - is it because it needs more setup from typeck to work correctly? I'm guessing the fix is okay since we just pull the already typeck'd data (we run save-analysis after all the analysis passes are complete) from the tables.

With this change we can 'go to def' on all segments in the `T::Thing::print()` path.

bors added a commit that referenced this pull request Apr 12, 2019

Auto merge of #59910 - Centril:rollup-yjv7b06, r=Centril
Rollup of 15 pull requests

Successful merges:

 - #59680 (Document the -Z flag to the rustc book)
 - #59711 (Add back the substring test)
 - #59806 (compiletest: Improve no_prefer_dynamic docs)
 - #59809 (Make trait_methods_not_found use a lock)
 - #59811 (Kill dead code dominator code.)
 - #59814 (Fix broken links on std::boxed doc page)
 - #59821 (improve unknown enum variant errors)
 - #59831 (Remove strange formatting in `Ordering` docs.)
 - #59836 (std::ops::Div examples: correct nominator to numerator)
 - #59857 (SGX target: fix cfg(test) build)
 - #59876 (Update TRPL to use mdbook 0.2)
 - #59880 (Remove note about transmute for float bitpatterns.)
 - #59889 (Update diagnostics.rs)
 - #59891 (Fix the link to sort_by_cached_key)
 - #59894 (save-analysis: Pull associated type definition using `qpath_def`)

Failed merges:

r? @ghost

@bors bors merged commit bcd263e into rust-lang:master Apr 12, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@Xanewok Xanewok deleted the Xanewok:save-assoc-ty-qpath branch Apr 12, 2019

@cuviper

This comment has been minimized.

Copy link
Member

commented May 14, 2019

@Xanewok

This comment has been minimized.

Copy link
Member Author

commented May 16, 2019

@pietroalbini @rust-lang/release should this be backported?

@cuviper

This comment has been minimized.

Copy link
Member

commented May 16, 2019

@Xanewok only when/if it gets beta-accepted by the compiler team.

@Xanewok

This comment has been minimized.

Copy link
Member Author

commented May 16, 2019

only when/if it gets beta-accepted by the compiler team.

I imagined this decision has always been made primarily by release/infra teams, just wanted to bring this to their attention since the backport window will close soon.

Let's try with another team: @rust-lang/compiler should this be backported? This fixes save-analysis compilation for gstreamer (and also for its reverse deps). It was requested in rust-lang/rls#1390 (comment) and also on reddit, as @cuviper pointed out.

@pietroalbini

This comment has been minimized.

Copy link
Member

commented May 16, 2019

I imagined this decision has always been made primarily by release/infra teams, just wanted to bring this to their attention since the backport window will close soon.

Decisions for beta backports are made by the team responsible for the area of the codebase affected by the changes (most of the time compiler, sometimes libs or rustdoc), and the release team doesn't have a say on what gets backported.

For stable backports the process for beta still applies, and in addition to that the release team has to approve the point release as a whole.

@pnkfelix

This comment has been minimized.

Copy link
Member

commented May 16, 2019

discussed at T-compiler meeting. approved for beta-backport.

bors added a commit that referenced this pull request May 18, 2019

Auto merge of #60881 - Xanewok:beta-save-assoc-ty-qpath, r=pietroalbini
[beta] save-analysis: Pull associated type definition using `qpath_def`

Beta backport of #59894.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.