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

In lifetime_generate, only select from all_impls(trait_id) on demand #932

Merged
merged 3 commits into from
Jan 11, 2024

Conversation

Chris-Hawblitzel
Copy link
Collaborator

Previously, the use of ctxt.tcx.all_impls(trait_id) was pulling in many unrelated definitions from the Rust library, which led to various failures when using a Rust library trait (e.g. IntoIterator). This pull request prunes away the unneeded definitions, omitting trait impls for types that aren't used.

@utaal-b utaal-b requested review from utaal-b and removed request for utaal December 20, 2023 11:16
Copy link
Contributor

@utaal-b utaal-b left a comment

Choose a reason for hiding this comment

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

Can you also confirm my understanding that this will not consider impls for which some of the type arguments are external?

// (We add each impl for T to this when we process T)
// (To avoid importing unnecessary impls, we delay processing impl until all t are used)
// t1 -> impl, ..., tn -> impl
typ_to_trait_impls: HashMap<DefId, Vec<DefId>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

This took me a bit to parse. Can we rename it to, e.g. typ_used_in_trait_impls or similar?

Comment on lines 1995 to 1999
for t in &datatypes {
if !state.typ_to_trait_impls.contains_key(t) {
state.typ_to_trait_impls.insert(*t, Vec::new());
}
_ => (),
state.typ_to_trait_impls.get_mut(&t).unwrap().push(impl_id);
Copy link
Contributor

Choose a reason for hiding this comment

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

You should be able to do:

state.typ_to_trait_impls.entry(&t).or_insert(Vec::new()).extend(datatypes.iter());

and skip the additional look-up.

Comment on lines 2002 to 2003
assert!(!state.impl_to_remaining_typs.contains_key(&impl_id));
state.impl_to_remaining_typs.insert(impl_id, (name.clone(), datatypes));
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can just assert that insert returned None.

// t1 -> impl, ..., tn -> impl
typ_to_trait_impls: HashMap<DefId, Vec<DefId>>,
// impl -> (t1, ..., tn) and process impl when t1...tn is empty
impl_to_remaining_typs: HashMap<DefId, (Id, Vec<DefId>)>,
Copy link
Contributor

Choose a reason for hiding this comment

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

remaining_typs_to_process_impl?

@Chris-Hawblitzel
Copy link
Collaborator Author

Can you also confirm my understanding that this will not consider impls for which some of the type arguments are external?

If some of the type arguments are unreached, then the impl itself will be unreached. External types should be unreachable since they are supposed to be disallowed from the verified code altogether. (There are some places we still need to check this, such as "impl InternalTrait for ExternalType", but that would be a separate well-formedness issue.)

@Chris-Hawblitzel Chris-Hawblitzel merged commit 64a0e2c into main Jan 11, 2024
5 checks passed
@Chris-Hawblitzel Chris-Hawblitzel deleted the erase_impl_assocs branch January 11, 2024 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants