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

Track the finalizing node in the specialization graph #70535

Merged
merged 7 commits into from
Apr 1, 2020
Merged

Track the finalizing node in the specialization graph #70535

merged 7 commits into from
Apr 1, 2020

Conversation

jonas-schievink
Copy link
Contributor

Fixes #70419
Fixes #70442

r? @eddyb

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 29, 2020
@Centril Centril added F-specialization `#![feature(specialization)]` F-associated_type_defaults `#![feature(associated_type_defaults)]` labels Mar 29, 2020
@Centril
Copy link
Contributor

Centril commented Mar 29, 2020

cc @nikomatsakis

Comment on lines +105 to +116
let substs = tcx.infer_ctxt().enter(|infcx| {
let param_env = param_env.with_reveal_all();
let substs = rcvr_substs.rebase_onto(tcx, trait_def_id, impl_data.substs);
let substs = translate_substs(
&infcx,
param_env,
impl_data.impl_def_id,
substs,
leaf_def.defining_node,
);
infcx.tcx.erase_regions(&substs)
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit concerning that an inference context is needed for this. Shouldn't building the specialization graph also compute the necessary information to do this "more directly"? Or is this because of type parameters that are constrained by associated items?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like it kinda does, yeah. fulfill_implication is what needs the inference context since it unifies the impls, and it returns the Substs to rebase onto in translate_substs. It's also called when building the specialization graph, but there we just throw away the result. I'll look into storing the result, but I'm not sure I can figure out how everything works exactly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this does not look straightforward, and I think I also lack some understanding about the type checker to do this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fair, it's easier to infer everything through unification. I suppose the only thing that could be done ahead of time is to get the Substs for a less specialized impl, in terms of types from a more specialized impl.

That is, impl<T, U> Foo<U> for Vec<T> and impl<'a, X> Foo<() for Vec<&'a X> would give you Substs [&'a X, ()], and I guess if you had these on every edge in the specialization graph, you could walk "up" it (i.e. in the "less specialized" direction) by successive substitution.

Actually, is that what's necessary? Because it doesn't seem that hard to compute, you'd use translate_subst or w/e ahead of time and keep the result.
But it can be done in a separate PR.

src/librustc/traits/util.rs Outdated Show resolved Hide resolved
src/librustc/traits/util.rs Outdated Show resolved Hide resolved
Copy link
Member

@eddyb eddyb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments, but specialization it not my, uh, specialty.

@eddyb
Copy link
Member

eddyb commented Mar 29, 2020

r? @nikomatsakis

@rust-highfive rust-highfive assigned nikomatsakis and unassigned eddyb Mar 29, 2020
@bors

This comment has been minimized.

@rust-lang rust-lang deleted a comment from rust-highfive Apr 1, 2020
@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Apr 1, 2020

📌 Commit fd8f818 has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented Apr 1, 2020

🌲 The tree is currently closed for pull requests below priority 500, this pull request will be tested once the tree is reopened

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 1, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Apr 1, 2020
…ikomatsakis

Track the finalizing node in the specialization graph

Fixes rust-lang#70419
Fixes rust-lang#70442

r? @eddyb
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 1, 2020
Rollup of 6 pull requests

Successful merges:

 - rust-lang#70535 (Track the finalizing node in the specialization graph)
 - rust-lang#70590 (Miri: make backtrace function names and spans match up)
 - rust-lang#70616 (rustc_target::abi: rename FieldPlacement to FieldsShape.)
 - rust-lang#70626 (cargotest: remove webrender)
 - rust-lang#70649 (clean up E0468 explanation)
 - rust-lang#70662 (compiletest: don't use `std::io::stdout()`, as it bypasses `set_print`.)

Failed merges:

r? @ghost
@bors bors merged commit 0e0d84c into rust-lang:master Apr 1, 2020
@jonas-schievink jonas-schievink deleted the graph-refactor branch April 9, 2020 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-associated_type_defaults `#![feature(associated_type_defaults)]` F-specialization `#![feature(specialization)]` S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
6 participants