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

Add AliasKind::Weak for type aliases. #108860

Merged
merged 3 commits into from
Jun 17, 2023
Merged

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Mar 7, 2023

type Foo<T: Debug> = Bar<T>; does not check T: Debug at use sites of Foo<NotDebug>, because in contrast to a

trait Identity {
    type Identity;
}
impl<T: Debug> Identity for T {
    type Identity = T;
}
<NotDebug as Identity>::Identity

type aliases do not exist in the type system, but are expanded to their aliased type immediately when going from HIR to the type layer.

Similarly:

  • a private type alias for a public type is a completely fine thing, even though it makes it a bit hard to write out complex times sometimes
  • rustdoc expands the type alias, even though often times users use them for documentation purposes
  • diagnostics show the expanded type, which is confusing if the user wrote a type alias and the diagnostic talks about another type that they don't know about.

For type alias impl trait, these issues do not actually apply in most cases, but sometimes you have a type alias impl trait like type Foo<T: Debug> = (impl Debug, Bar<T>);, which only really checks it for impl Debug, but by accident prevents Bar<T> from only being instantiated after proving T: Debug. This PR makes sure that we always check these bounds explicitly and don't rely on an implementation accident.

To not break all the type aliases out there, we only use it when the type alias contains an opaque type. We can decide to do this for all type aliases over an edition.

Or we can later extend this to more types if we figure out the back-compat concerns with suddenly checking such bounds.

As a side effect, easily allows fixing #108617, which I did.

fixes #108617

@rustbot
Copy link
Collaborator

rustbot commented Mar 7, 2023

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

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels Mar 7, 2023
@rust-log-analyzer

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Mar 8, 2023

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 8, 2023

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 8, 2023
@bors
Copy link
Contributor

bors commented Mar 8, 2023

⌛ Trying commit 5df0ff9b38e2ec1ac95f2e5da14052323e27da54 with merge 10ea3ca86b40e3a352e4b76cc61eb09d6e531b17...

@compiler-errors
Copy link
Member

r? @compiler-errors

@rustbot rustbot assigned compiler-errors and unassigned spastorino Mar 8, 2023
@bors
Copy link
Contributor

bors commented Mar 8, 2023

☀️ Try build successful - checks-actions
Build commit: 10ea3ca86b40e3a352e4b76cc61eb09d6e531b17 (10ea3ca86b40e3a352e4b76cc61eb09d6e531b17)

1 similar comment
@bors
Copy link
Contributor

bors commented Mar 8, 2023

☀️ Try build successful - checks-actions
Build commit: 10ea3ca86b40e3a352e4b76cc61eb09d6e531b17 (10ea3ca86b40e3a352e4b76cc61eb09d6e531b17)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (10ea3ca86b40e3a352e4b76cc61eb09d6e531b17): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.4% [0.4%, 0.5%] 3
Regressions ❌
(secondary)
0.4% [0.3%, 0.5%] 3
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.4% [0.4%, 0.5%] 3

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.1% [-2.9%, -1.3%] 5
All ❌✅ (primary) - - 0

Cycles

This benchmark run did not return any relevant results for this metric.

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Mar 8, 2023
@bors
Copy link
Contributor

bors commented Mar 10, 2023

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

Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

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

a few more nits, and then r=me

As that will be more difficult to figure out later, it would like to have a doc somewhere which explains the behavior of weak projections. How they differ from ordinary projections and how they're equal.

compiler/rustc_hir_analysis/src/coherence/orphan.rs Outdated Show resolved Hide resolved
compiler/rustc_privacy/src/lib.rs Outdated Show resolved Hide resolved
@@ -649,7 +649,7 @@ impl<'tcx> TypeVisitor<TyCtxt<'tcx>> for OrphanChecker<'tcx> {
| ty::RawPtr(..)
| ty::Never
| ty::Tuple(..)
| ty::Alias(ty::Projection, ..) => self.found_non_local_ty(ty),
| ty::Alias(ty::Projection | ty::Weak, ..) => self.found_non_local_ty(ty),
Copy link
Contributor

Choose a reason for hiding this comment

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

we still have to fix #99554 for all ty::Alias

@@ -1,9 +1,10 @@
#![feature(type_alias_impl_trait)]

// check-pass

type Foo = impl Fn() -> Foo;
Copy link
Contributor

Choose a reason for hiding this comment

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

how is that fixed? Though I guess this should work, so it's fine

Copy link
Contributor Author

@oli-obk oli-obk Mar 13, 2023

Choose a reason for hiding this comment

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

it's fixed by not normalizing Foo eagerly. So normalize the weak alias to the opaque type, and if we get the item_bounds, we get an unnormalized Foo.

Previously, when registering a hidden type, we used to instantiate the opaque type, which then contained itself, which we tried to instantiate, too, and thus cycled.

#![feature(type_alias_impl_trait)]

type Bar<'a, 'b> = impl PartialEq<Bar<'a, 'b>> + std::fmt::Debug;

fn bar<'a, 'b>(i: &'a i32) -> Bar<'a, 'b> {
//~^ ERROR can't compare `&i32` with `Bar<'a, 'b>`
Copy link
Contributor

Choose a reason for hiding this comment

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

why does this break? 🤔 is Bar<'a, 'b> a ty::Opaque here or a weak projection?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're not normalizing the obligations before matching them against candidates. Doing so will fix this case and other existing known-bug cases. I'll have a PR open for just that soon that we can then analyze on its own

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I opened #109115

Copy link
Member

Choose a reason for hiding this comment

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

We could also just run normalize_with_depth on the nested obligations in the assoc type normalizer, like the rest of the selection/etc. code does

@lcnr
Copy link
Contributor

lcnr commented Mar 13, 2023

r? @lcnr

@rust-log-analyzer

This comment has been minimized.

Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

r=me fixing tidy, addressing my one nit

compiler/rustc_trait_selection/src/solve/weak_types.rs Outdated Show resolved Hide resolved
Only use it when the type alias contains an opaque type.

Also does wf-checking on such type aliases.
@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 16, 2023

@bors r=compiler-errors

@bors
Copy link
Contributor

bors commented Jun 16, 2023

📌 Commit 79c2c98 has been approved by compiler-errors

It is now in the queue for this repository.

@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 Jun 16, 2023
@bors
Copy link
Contributor

bors commented Jun 17, 2023

⌛ Testing commit 79c2c98 with merge 0cc541e...

@bors
Copy link
Contributor

bors commented Jun 17, 2023

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 0cc541e to master...

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (0cc541e): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.3% [0.2%, 0.5%] 8
Regressions ❌
(secondary)
0.4% [0.2%, 0.8%] 7
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.3% [0.2%, 0.5%] 8

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
2.5% [2.5%, 2.5%] 1
Regressions ❌
(secondary)
2.1% [2.1%, 2.1%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-3.7% [-3.7%, -3.7%] 1
All ❌✅ (primary) 2.5% [2.5%, 2.5%] 1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 655.939s -> 656.882s (0.14%)

@lcnr
Copy link
Contributor

lcnr commented Jun 19, 2023

have we documented the behavior of weak type aliases somewhere? I want to avoid us getting stuck with an unclear implementation and weird surprises/bugs later on.

@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 19, 2023

Do we have documentation for projections anywhere? XD i'll happily add it nearby.

If not, where should the docs for all four aliases live?

@lcnr
Copy link
Contributor

lcnr commented Jun 19, 2023

rustc-dev-guide 🤔 there's also #60471 (comment)

I feel like we may want to restructure the dev guide to have a chapter "type system info dump" where we can put info about parts of the type system without having to figure out how to sensibly integrate them with the rest of the dev-guide. And then I would put "type aliases" as a subchapter there 🤔

@rylev
Copy link
Member

rylev commented Jun 21, 2023

@oli-obk @lcnr This seems to have caused a tiny bit of perf regression in some doc cases. Seems maybe get_auto_trait_impls was impacted - though I'm unsure why that would be. Thoughts?

@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 21, 2023

It's hard to tell. As none of our perf tests use TAITs, this is likely some inlining changes in the type folders:

-30,081,414  ???:<&rustc_middle::ty::list::List<rustc_middle::ty::subst::GenericArg> as rustc_type_ir::fold::TypeFoldable<rustc_middle::ty::context::TyCtxt>>::try_fold_with::<rustc_infer::infer::freshen::TypeFreshener>
 19,793,318  ???:<rustc_infer::infer::freshen::TypeFreshener as rustc_type_ir::fold::FallibleTypeFolder<rustc_middle::ty::context::TyCtxt>>::try_fold_ty
 18,941,215  ???:<rustc_middle::ty::sty::TraitRef as rustc_type_ir::fold::TypeFoldable<rustc_middle::ty::context::TyCtxt>>::try_fold_with::<rustc_infer::infer::freshen::TypeFreshener>
 12,763,035  ???:<rustc_infer::infer::freshen::TypeFreshener as rustc_type_ir::fold::FallibleTypeFolder<rustc_middle::ty::context::TyCtxt>>::try_fold_binder::<rustc_middle::ty::PredicateKind>
-12,290,330  ???:<rustc_middle::ty::sty::Binder<rustc_middle::ty::PredicateKind> as rustc_type_ir::fold::TypeSuperFoldable<rustc_middle::ty::context::TyCtxt>>::super_fold_with::<rustc_infer::infer::freshen::TypeFreshener>
-11,001,070  ???:rustc_trait_selection::traits::do_normalize_predicates
 10,896,326  ???:rustc_trait_selection::traits::normalize_param_env_or_error
  5,304,007  ???:<rustc_middle::ty::context::TyCtxt>::lang_items
  5,248,049  ???:<smallvec::SmallVec<[rustc_ast::ast::Attribute; 8]> as core::iter::traits::collect::Extend<rustc_ast::ast::Attribute>>::extend::<rustc_metadata::rmeta::decoder::DecodeIterator<rustc_ast::ast::Attribute>>
  5,206,509  ???:<rustc_trait_selection::traits::select::SelectionContext>::select
 -5,186,609  ???:<rustc_span::symbol::Symbol>::intern
  2,826,798  ???:<rustc_middle::ty::PredicateKind as rustc_type_ir::fold::TypeFoldable<rustc_middle::ty::context::TyCtxt>>::try_fold_with::<rustc_infer::infer::freshen::TypeFreshener>
 -2,384,760  ???:<rustc_trait_selection::traits::select::SelectionContext>::impl_or_trait_obligations
 -2,309,840  ???:<rustc_middle::ty::context::TyCtxt>::hygienic_eq
  2,140,440  ???:<rustc_infer::traits::Obligation<rustc_middle::ty::sty::Binder<rustc_middle::ty::TraitPredicate>>>::with_depth::<rustc_middle::ty::sty::Binder<rustc_middle::ty::TraitPredicate>>
 -2,126,043  ???:<&rustc_middle::ty::list::List<rustc_middle::ty::subst::GenericArg> as rustc_serialize::serialize::Decodable<rustc_metadata::rmeta::decoder::DecodeContext>>::decode
 -1,962,828  ???:<rustc_middle::ty::Ty as rustc_type_ir::fold::TypeSuperFoldable<rustc_middle::ty::context::TyCtxt>>::try_super_fold_with::<rustc_infer::infer::freshen::TypeFreshener>
 -1,639,261  ???:<hashbrown::raw::RawTable<(rustc_middle::ty::context::InternedInSet<rustc_middle::ty::list::List<rustc_middle::ty::subst::GenericArg>>, ())>>::reserve_rehash::<hashbrown::map::make_hasher<rustc_middle::ty::context::InternedInSet<rustc_middle::ty::list::List<rustc_middle::ty::subst::GenericArg>>, (), core::hash::BuildHasherDefault<rustc_hash::FxHasher>>::{closure
  1,610,331  ???:<hashbrown::raw::RawTable<(rustc_middle::ty::context::InternedInSet<rustc_middle::ty::list::List<rustc_middle::ty::Ty>>, ())>>::reserve_rehash::<hashbrown::map::make_hasher<rustc_middle::ty::context::InternedInSet<rustc_middle::ty::list::List<rustc_middle::ty::Ty>>, (), core::hash::BuildHasherDefault<rustc_hash::FxHasher>>::{closure
  1,562,392  ???:rustc_query_impl::plumbing::__rust_begin_short_backtrace::<rustc_query_impl::query_impl::impl_trait_ref::dynamic_query::{closure
  1,292,944  ???:<rustc_middle::ty::assoc::AssocItem>::ident
  1,059,942  ???:<rustc_middle::ty::assoc::AssocItems>::find_by_name_and_kind

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Spurious "bounds [...] not enforced in type aliases" when using TAIT
10 participants