-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Move folding & visiting traits into type library #107712
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
Closed
eggyal
wants to merge
8
commits into
rust-lang:master
from
eggyal:move_fold_visit_traits_to_type_lib
Closed
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
bea94aa
Refactor functions used by HasEscapingVarsVisitor
eggyal e0767d9
Refactor functions used by HasTypeFlagsVisitor
eggyal 2eb4609
Refactor default TypeVisitable::error_reported
eggyal f92b450
Project Binder and Predicate from Interner trait
eggyal 8fcd967
Make visiting generic over the interner
eggyal 8754bd3
Make folding generic over the interner
eggyal 59ee434
Move visiting traits to type library
eggyal ff87b7a
Move folding traits to type library
eggyal File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4868,6 +4868,7 @@ dependencies = [ | |
"rustc_macros", | ||
"rustc_serialize", | ||
"smallvec", | ||
"tracing", | ||
] | ||
|
||
[[package]] | ||
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could there be a trait alias for TypeFoldable filling in TyCtxt in rustc_middle? That would significantly reduce the diff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wasn't aware of trait aliases! Good idea, will look into it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay so, whilst a trait alias can be used in bounds such as these, it can't be used to implement the trait: so we'll still need
impl<'tcx> TypeFoldable<TyCtxt<'tcx>> for ...
followed by... where T: TypeFoldableAlias<'tcx>
—and hence we'd still end up modifying each of these bounds to refer to the alias instead, unless the alias is namedTypeFoldable
(which I think would be very confusing unless the trait itself is renamed something else?). So the diff will likely remain similar, but the boilerplate would be slightly reduced.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine to have a generic TypeFoldable that is almost never used and one that is used everywhere else. We do a similar thing for
Ty
after all. It's just that type aliases are stable.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created another branch with an alternative implementation using trait aliases, but as suspected the impact on the overall diff is not so significant: +1494-1309 across 99 files versus this PR's +1709-1434 across 112 files.
I'm happy to use/force-push that if it's preferred, but IMHO defining e.g.
TypeFoldable
to mean different things depending on whether the alias or the underlying trait are brought into scope is pretty confusing (and all the existing reexports certainly don't help to keep track of what each name means in a given context). In particular for each trait, that branch now has at very least the following:And the codebase variously uses each of the above in different and inconsistent ways. Certainly this could (and probably should) be rationalised, but fundamentally I think the naming conflict is unhelpful.
Incidentally, you may note that branch also removes some previously required imports from scope: this is a direct result of using trait aliases, which erroneously bring supertraits into scope (reported #107747). Once that bug is resolved, many of those imports will need to be re-added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! ❤️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On reflection, I'm not sure there's any benefit in blanket implementing each (existing)
middle
trait (parameterised by'tcx
) for all types implementing the respectiveir
trait (parameterised byTyCtxt<'tcx>
), because generic methods in themiddle
traits (e.g.TypeVisitable::visit_with
) are parameterised by a type that must implement some othermiddle
trait (e.g.V: TypeVisitor<'tcx>
) and, in delegating to their
trait, the blanket implementation must pass a value of that generic type to its equivalent method (ir::TypeVisitable::visit_with
), which requires that value also to implement the respectiveir
trait (ir::TypeVisitor<TyCtxt<'tcx>>
). However this cannot be guaranteed unless themiddle
traits are subtraits of their
traits, thus always requiring that their
traits be implemented and negating much benefit of retaining themiddle
traits; any remaining ergonomic benefit is then negated by ambiguities that such supertraits introduce when resolving associated items.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... I see. I'm assuming trait aliases won't allow
impls TraitAlias for Type
, thus leading to the branch you linked that still referencesTypeVisitable<TyCtxt<'tcx>>
almost as often as before.I'm convinced to reopen this PR, even if it's a bit annoying to have to do that all over the place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing the experiment. Please add this information to the PR so that it's clear why this design was chosen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, you cannot (currently?)
impl TraitAlias for ...
.Will open a new PR (am looking into some other changes anyway) and provide the detail as suggested. Thanks!