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.

Sign up for GitHub

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

Jump to bottom

Do not bring trait alias supertraits into scope #107803

Merged
merged 1 commit into from
Feb 9, 2023
Merged

Do not bring trait alias supertraits into scope #107803

merged 1 commit into from
Feb 9, 2023

Conversation

eggyal
Copy link
Contributor

@eggyal eggyal commented Feb 8, 2023

Fixes #107747
cc #41517

@rustbot
Copy link
Collaborator

rustbot commented Feb 8, 2023

r? @WaffleLapkin

(rustbot has picked a reviewer for you, use r? to override)

@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. labels Feb 8, 2023
eggyal
eggyal commented Feb 8, 2023
View reviewed changes
compiler/rustc_metadata/src/rmeta/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_hir_analysis/src/collect.rs Outdated Show resolved Hide resolved
compiler/rustc_middle/src/ty/trait_def.rs Outdated Show resolved Hide resolved
@WaffleLapkin
Copy link
Member

r? compiler

@rustbot rustbot assigned petrochenkov and unassigned WaffleLapkin Feb 8, 2023
@eggyal
Copy link
Contributor Author

eggyal commented Feb 8, 2023

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 8, 2023
@compiler-errors compiler-errors self-assigned this Feb 8, 2023
@eggyal eggyal force-pushed the do_not_bring_trait_alias_supertraits_into_scope branch from 7a8f3ab to bf05512 Compare February 8, 2023 17:02
@rustbot rustbot added the A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) label Feb 8, 2023
@eggyal eggyal force-pushed the do_not_bring_trait_alias_supertraits_into_scope branch 2 times, most recently from 80b7c07 to 1af7bf1 Compare February 8, 2023 17:54
@eggyal
Copy link
Contributor Author

eggyal commented Feb 8, 2023

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 8, 2023
@petrochenkov petrochenkov removed their assignment Feb 8, 2023
@eggyal eggyal force-pushed the do_not_bring_trait_alias_supertraits_into_scope branch from 1af7bf1 to 38ec810 Compare February 8, 2023 19:56
@compiler-errors
Copy link
Member

Thanks, yeah, I agree that inlining elaborate_bounds is a bit verbose, but I'd rather have it this way since trait-aliases are not yet stable.

@bors r+

@bors
Copy link
Contributor

bors commented Feb 8, 2023

📌 Commit 38ec810 has been approved by compiler-errors

It is now in the queue for this repository.

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 8, 2023
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Feb 8, 2023
@Dylan-DPC Dylan-DPC mentioned this pull request Feb 9, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 9, 2023
@bors
Auto merge of rust-lang#107853 - Dylan-DPC:rollup-macf1qo, r=Dylan-DPC
8996ea9 
Rollup of 6 pull requests

Successful merges:

 - rust-lang#107648 (unused-lifetimes: don't warn about lifetimes originating from expanded code)
 - rust-lang#107655 (rustdoc: use the same URL escape rules for fragments as for examples)
 - rust-lang#107659 (test: snapshot for derive suggestion in diff files)
 - rust-lang#107786 (Implement some tweaks in the new solver)
 - rust-lang#107803 (Do not bring trait alias supertraits into scope)
 - rust-lang#107815 (Disqualify `auto trait` built-in impl in new solver if explicit `impl` exists)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 16a4138 into rust-lang:master Feb 9, 2023
@rustbot rustbot added this to the 1.69.0 milestone Feb 9, 2023
@eggyal eggyal deleted the do_not_bring_trait_alias_supertraits_into_scope branch February 10, 2023 19:55
@eggyal
Copy link
Contributor Author

eggyal commented Feb 10, 2023

Would it be possible to pull this into the current beta? Else a PR that requires it will either have to wait for beta 1.69 on 9 March(?) or else include a workaround that I guess will then be reverted once that beta is released.

@rustbot label +beta-nominated

@rustbot
Copy link
Collaborator

rustbot commented Feb 10, 2023

Error: Label beta-nominated can only be set by Rust team members

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #t-infra on Zulip.

@compiler-errors
Copy link
Member

I don't think this deserves beta nomination, since it changes a nightly-only feature and can be worked around trivially.

@eggyal
Copy link
Contributor Author

eggyal commented Feb 10, 2023 •
edited

Ah, just saw your comment after I already asked on #t-release Zulip stream. Yeah you're probably right, but I thought I'd ask anyway. Thanks!

@eggyal
Copy link
Contributor Author

eggyal commented Feb 10, 2023 •
edited

Actually there is a deeper problem. This PR affects which imports must be brought into scope, or that end up unused if brought into scope unnecessarily. Hence if a commit that is so affected (such as the interim workaround for my WIP) is accepted into master, CI will cease working as soon as the stage0 compiler incorporates this PR. Getting this into stage0 asap (before any conflicting PRs are accepted) will avoid that.

@compiler-errors
Copy link
Member

I'd need a motivating example to understand what you mean.

My first instinct is that changes that require different code when compiled against beta vs nightly like this are typically worked around with #[cfg(bootstrap)] attributes. These are systematically removed next time that beta is bumped (during the next nightly promotion), so I don't think that CI is at risk here or anything like that.

@eggyal
Copy link
Contributor Author

eggyal commented Feb 10, 2023

One motivating example (my WIP contains many similar): using nightly, compiler/rustc_middle/src/ty/opaque_types.rs requires a use crate::ty::TypeFoldable for the calls to fold_with etc to resolve; however on beta that import is unused (because the module also imports TypeSuperFoldable, which is an alias in my WIP, that brings in TypeFoldable as its supertrait).

I wasn't aware of cfg(bootstrap). It sounds like it will do the trick—will look into it. Thanks again!

@eggyal
Copy link
Contributor Author

eggyal commented Feb 10, 2023

Of course even if I use cfg(bootstrap) in my workaround, someone else could push a PR that unwittingly doesn't use it and then causes CI to break.

@compiler-errors
Copy link
Member

@eggyal, in that case, their PRs will fail to build in CI on stage2 (which essentially emulates a build against a promoted beta), and they'll be notified of the failure. I don't think that many PRs will land that import TypeFoldable in the next handful weeks until the next nightly bump. If anything, your follow-up PR will have to wait a few weeks to get merged. I still don't think this is a good candidate for a beta backport.

@eggyal
Copy link
Contributor Author

eggyal commented Feb 10, 2023

Understood.

FWIW, it isn't just an import of TypeFoldable that could cause a problem but any trait alias where supertrait methods are called.

@compiler-errors
Copy link
Member

Yes, but also there are basically no users of trait aliases in the compiler except for the one that you plan on introducing. I know it's tough to work around subtle differences between the bootstrap compiler and nightly, but that's just part of working on the compiler, especially when using unstable features like this.

@eggyal eggyal mentioned this pull request Feb 11, 2023
Merged
eggyal added a commit to eggyal/rust that referenced this pull request Feb 13, 2023
@eggyal
Workaround issue rust-lang#107747
b409329 
Only required until fix rust-lang#107803 is merged into stage0 compiler, expected
when beta 1.69.0 is released on 2023-03-09, then this commit can be
reverted.
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 13, 2023
@bors
Auto merge of rust-lang#107924 - eggyal:move_fold_visit_traits_to_typ…
a3c9eed 
…e_lib_with_trait_alias, r=oli-obk

Move folding & visiting traits into type library

This is a rework of rust-lang#107712, following feedback on that PR.

In particular, this version uses trait aliases to reduce the API churn for trait consumers.  Doing so requires a workaround for rust-lang#107747 until its fix in rust-lang#107803 is merged into the stage0 compiler; this workaround, which uses conditional compilation based on the `bootstrap` configuration predicate, sits in dedicated commit b409329 for ease of reversion.

The possibility of the `rustc_middle` crate retaining its own distinct versions of each folding/visiting trait, blanket-implemented on all types that implement the respective trait in the type library, was also explored: however since this would necessitate making each `rustc_middle` trait a subtrait of the respective type library trait (so that such blanket implementations can delegate their generic methods), no benefit would be gained.

r? types
Jarcho pushed a commit to Jarcho/rust that referenced this pull request Feb 26, 2023
@bors
Auto merge of rust-lang#107924 - eggyal:move_fold_visit_traits_to_typ…
42d4bd7 
…e_lib_with_trait_alias, r=oli-obk

Move folding & visiting traits into type library

This is a rework of rust-lang#107712, following feedback on that PR.

In particular, this version uses trait aliases to reduce the API churn for trait consumers.  Doing so requires a workaround for rust-lang#107747 until its fix in rust-lang#107803 is merged into the stage0 compiler; this workaround, which uses conditional compilation based on the `bootstrap` configuration predicate, sits in dedicated commit b409329 for ease of reversion.

The possibility of the `rustc_middle` crate retaining its own distinct versions of each folding/visiting trait, blanket-implemented on all types that implement the respective trait in the type library, was also explored: however since this would necessitate making each `rustc_middle` trait a subtrait of the respective type library trait (so that such blanket implementations can delegate their generic methods), no benefit would be gained.

r? types
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@compiler-errors compiler-errors compiler-errors left review comments

@WaffleLapkin WaffleLapkin WaffleLapkin left review comments

Assignees

@compiler-errors compiler-errors

Labels
A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) 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.
Projects
None yet
Milestone
1.69.0
Development

Successfully merging this pull request may close these issues.

Trait aliases should not bring supertraits into scope
6 participants
@eggyal @rustbot @WaffleLapkin @compiler-errors @bors @petrochenkov