rustc_metadata: Deduplicate code between encode_info_for_item, encode_info_for_foreign_item, encode_info_for_trait_item, and encode_info_for_impl_item

[pitaj]

For #77393

r? @petrochenkov

Todo:

• Dedupe item and foreign_item
• Review for item/foreign_item dedupe
  • Function / parameter naming?
• Dedupe trait_item and impl_item
• Dedupe item_foreign_item_common and trait_impl_item_common

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @petrochenkov (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[petrochenkov]

@pitaj
I'll look at this some time closer to the end of the week, most likely.

[bors]

☔ The latest upstream changes (presumably #78077) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

[petrochenkov]

I'm going to reassign this to r? @oli-obk who requested this change in #77375 (comment).
With so few things shared and so many boolean flags passed, I'm not sure this is an improvement.
Perhaps #77393 needs to be closed as not-an-issue instead.

[pitaj]

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

[oli-obk]

Yea, these boolean flags do not make the code less fragile. I think we should still do this PR, but just remove all the boolean flags and keep the individual logic to each call site. We can also create multiple functions if we want to share more, but I think starting out with just the minimal implementation that deduplicates the code that is actually shared is the best way forward. Once that is a real improvement over the status quo, we can look at the call sites to see whether creating more functions for a subset of the call sites is helpful

[pitaj]

I've removed the boolean flags. Turns out they have 8 lines fully in common. I've seen some other opportunities for dedupe.

• generics, explicit_predicates, inferred_outlives are included in 9 other methods
  Only encode_generics is used on its own, all other instances have all three
• Could encode_variances_of be modified to ignore items without variances? (Or maybe the underlying TyCtxt::variances_of)
• Could encode_item_type be modified to ignore non-type items? (Or maybe the underlying TyCtxt::type_of)
• Maybe we could create encode_fn_sig that likewise ignores non-fn items (Or maybe the underlying TyCtxt::fn_sig)

[oli-obk]

We already have a conditional function like this: encode_optimized_mir. But I really dislike that (it caused some problems in a branch I wrote this week, so my dislike has a lot of bias and may be entirely unfounded). I'm actually moving away from the conditional version in my branch and changing the callers to decide whether to call it.

grouping generics, predicates and outlives bounds together does make sense. We could call it encode_bounds maybe?

[pitaj]

grouping generics, predicates and outlives bounds together does make sense. We could call it encode_bounds maybe?

I added encode_bounds. I moved the bodies of encode_explicit_predicates and encode_inferred_outlives into encode_bounds, since they weren't used anywhere but in encode_bounds after the other changes.

I'm actually moving away from the conditional version in my branch and changing the callers to decide whether to call it.

I seems to me that logic already exists in the TyCtxt methods. Consider variances_of, where the check occurs both in rustc_metadata::encoder and in rustc_typeck::variance

rust/compiler/rustc_metadata/src/rmeta/encoder.rs

Lines 1378 to 1384 in 430feb2

	match item.kind {
	hir::ItemKind::Enum(..)
	| hir::ItemKind::Struct(..)
	| hir::ItemKind::Union(..)
	| hir::ItemKind::Fn(..) => self.encode_variances_of(def_id),
	_ => {}
	}

rust/compiler/rustc_metadata/src/rmeta/encoder.rs

Lines 1732 to 1735 in 430feb2

	if let hir::ForeignItemKind::Fn(..) = nitem.kind {
	record!(self.tables.fn_sig[def_id] <- tcx.fn_sig(def_id));
	self.encode_variances_of(def_id);
	}

rust/compiler/rustc_typeck/src/variance/mod.rs

Lines 48 to 55 in 430feb2

	Node::Item(item) => match item.kind {
	hir::ItemKind::Enum(..)
	| hir::ItemKind::Struct(..)
	| hir::ItemKind::Union(..)
	| hir::ItemKind::Fn(..) => {}
	_ => unsupported(),
	},

rust/compiler/rustc_typeck/src/variance/mod.rs

Lines 69 to 73 in 430feb2

	Node::ForeignItem(item) => match item.kind {
	hir::ForeignItemKind::Fn(..) => {}
	_ => unsupported(),
	},

I'm not sure how possible it is to reuse this logic in encoder, but that would be pretty nice.

[oli-obk]

Ok, I see. Those kind of checks are okay. The case of encode_optimized_mir it was using self.tcx.mir_keys(LOCAL_CRATE).contains(&def_id) which seems very ad-hoc. The checks that you showed are explicitly checking for items that we should ignore/encode, so that's ok. Basically if the "root information" for making the decision is checked, I'm fine with it, but if it's just a symptom of the decision, that's too fragile imo.

[crlf0710]

@pitaj Ping from triage! What's the current status of this?

[pitaj]

Thanks for checking in. I haven't forgotten about this, just been focused on other things.

[JohnCSimon]

triage: I'm closing this PR as inactive

@pitaj - if this PR is still in progress, please feel free to reopen