Make the laziness of type aliases dependent on the defining crate #114566
Conversation
rustbot
added
S-waiting-on-review
| @@ -256,6 +256,7 @@ pub(crate) struct CrateRoot { | |||
| has_alloc_error_handler: bool, | |||
| has_panic_handler: bool, | |||
| has_default_lib_allocator: bool, | |||
| type_aliases_are_lazy: bool, | |||
There was a problem hiding this comment.
Unfortunately, we don't store the language features used by a crate in the metadata and thus I had to modify the latter.
There was a problem hiding this comment.
Out of curiosity, I've tried to instead use CrateMetadataRef::get_item_attrs with CRATE_DEF_INDEX but for some reason that just returned an empty iterator. Maybe I was just missing something.
In any case, looking at ast::Attributes doesn't feel robust or clean to me (e.g. would I need to handle cfg_attr myself or would it have been normalized away by that point?).
There was a problem hiding this comment.
we erase most attributes cross crate, so that's probably why you are not seeing them
| if matches!(self.tcx().def_kind(did), DefKind::TyAlias) | ||
| && (ty.skip_binder().has_opaque_types() || self.tcx().features().lazy_type_alias) | ||
| if let DefKind::TyAlias = tcx.def_kind(did) | ||
| && (ty.skip_binder().has_opaque_types() || tcx.type_aliases_are_lazy(did.krate)) |
There was a problem hiding this comment.
Although I did not try it out, instead of invoking tcx.type_aliases_are_lazy here, alternatively I could invoke it when collecting the constraints from the generic arguments of weak projections. This should also fix #114468 but I haven't thought about the consequences yet. I don't like this as much as my current patch though as it doesn't address the points I raised in section “edition interoperability” in the PR description.
There was a problem hiding this comment.
We could also store the laziness in DefKind::TyAlias as a boolean field. Do you think that would resolve the issues entirely?
There was a problem hiding this comment.
This would also be an option but it requires updating quite a few places. I've just pushed a commit so you can decide for yourself if you prefer it this way. If you approve of it, I'll squash the commits.
One slight advantage of CrateRoot.type_aliases_are_lazy is the fact that we can just throw it out if / once lazy type aliases get stabilized in edition 202X since we can simply do
- type_aliases_are_lazy => { cdata.root.type_aliases_are_lazy }
+ type_aliases_are_lazy => { cdata.root.edition.at_least_rust_202x() }There was a problem hiding this comment.
I like the way it looks now. We can always convert to another scheme, but I think encoding this information in the type alias DefKind matches how we encode similar other differences (e.g. mutable vs nonmutable statics)
rustbot
added
A-query-system
| @@ -196,7 +196,9 @@ impl<'hir> Map<'hir> { | |||
| ItemKind::Macro(_, macro_kind) => DefKind::Macro(macro_kind), | |||
| ItemKind::Mod(..) => DefKind::Mod, | |||
| ItemKind::OpaqueTy(..) => DefKind::OpaqueTy, | |||
| ItemKind::TyAlias(..) => DefKind::TyAlias, | |||
| ItemKind::TyAlias(..) => { | |||
| DefKind::TyAlias { lazy: self.tcx.features().lazy_type_alias } | |||
There was a problem hiding this comment.
hmm... I wonder why we duplicate this logic with compiler/rustc_resolve/src/build_reduced_graph.rs ... Not the fault of this PR, but something we should investigate
|
Ok, I'm gonna squash the commits. |
fmease
force-pushed
the
type-alias-laziness-is-crate-specific
branch
from
8bac236
to
5468336
Compare
|
Some changes might have occurred in exhaustiveness checking cc @Nadrieril Some changes occurred to the core trait solver cc @rust-lang/initiative-trait-system-refactor Some changes occurred in need_type_info.rs cc @lcnr Some changes occurred in src/tools/clippy cc @rust-lang/clippy |
fmease
changed the title
DefKind
|
CI green |
|
@bors r+ rollup |
bors
added
S-waiting-on-bors
…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#114376 (Avoid exporting __rust_alloc_error_handler_should_panic more than once.) - rust-lang#114413 (Warn when #[macro_export] is applied on decl macros) - rust-lang#114497 (Revert rust-lang#98333 "Re-enable atomic loads and stores for all RISC-V targets") - rust-lang#114500 (Remove arm crypto target feature) - rust-lang#114566 (Store the laziness of type aliases in their `DefKind`) - rust-lang#114594 (Structurally normalize weak and inherent in new solver) - rust-lang#114596 (Rename method in `opt-dist`) r? `@ghost` `@rustbot` modify labels: rollup
lcnr
removed
the
WG-trait-system-refactor
…e-specific, r=oli-obk Store the laziness of type aliases in their `DefKind` Previously, we would treat paths referring to type aliases as *lazy* type aliases if the current crate had lazy type aliases enabled independently of whether the crate which the alias was defined in had the feature enabled or not. With this PR, the laziness of a type alias depends on the crate it is defined in. This generally makes more sense to me especially if / once lazy type aliases become the default in a new edition and we need to think about *edition interoperability*: Consider the hypothetical case where the dependency crate has an older edition (and thus eager type aliases), it exports a type alias with bounds & a where-clause (which are void but technically valid), the dependent crate has the latest edition (and thus lazy type aliases) and it uses that type alias. Arguably, the bounds should *not* be checked since at any time, the dependency crate should be allowed to change the bounds at will with a *non*-major version bump & without negatively affecting downstream crates. As for the reverse case (dependency: lazy type aliases, dependent: eager type aliases), I guess it rules out anything from slight confusion to mild annoyance from upstream crate authors that would be caused by the compiler ignoring the bounds of their type aliases in downstream crates with older editions. --- This fixes rust-lang#114468 since before, my assumption that the type alias associated with a given weak projection was lazy (and therefore had its variances computed) did not necessarily hold in cross-crate scenarios (which [I kinda had a hunch about](rust-lang#114253 (comment))) as outlined above. Now it does hold. `@rustbot` label F-lazy_type_alias r? `@oli-obk`
fmease
changed the title
DefKind
Previously, we would treat paths referring to type aliases as lazy type aliases if the current crate had lazy type aliases enabled independently of whether the crate which the alias was defined in had the feature enabled or not.
With this PR, the laziness of a type alias depends on the crate it is defined in. This generally makes more sense to me especially if / once lazy type aliases become the default in a new edition and we need to think about edition interoperability:
Consider the hypothetical case where the dependency crate has an older edition (and thus eager type aliases), it exports a type alias with bounds & a where-clause (which are void but technically valid), the dependent crate has the latest edition (and thus lazy type aliases) and it uses that type alias. Arguably, the bounds should not be checked since at any time, the dependency crate should be allowed to change the bounds at will with a non-major version bump & without negatively affecting downstream crates.
As for the reverse case (dependency: lazy type aliases, dependent: eager type aliases), I guess it rules out anything from slight confusion to mild annoyance from upstream crate authors that would be caused by the compiler ignoring the bounds of their type aliases in downstream crates with older editions.
This fixes #114468 since before, my assumption that the type alias associated with a given weak projection was lazy (and therefore had its variances computed) did not necessarily hold in cross-crate scenarios (which I kinda had a hunch about) as outlined above. Now it does hold.
@rustbot label F-lazy_type_alias
r? @oli-obk