Create a visitor for TypeFoldables and use it to fix cleanup issue #20298
#30317
Conversation
|
r? @brson (rust_highfive has picked a reviewer for you, use r? to override) |
jseyfried
force-pushed
the
refactor_type_folder
branch
from
7d4c2fd
to
5913159
Compare
| @@ -751,6 +658,18 @@ impl<'tcx> TypeFoldable<'tcx> for ty::Predicate<'tcx> { | |||
| ty::Predicate::ObjectSafe(trait_def_id), | |||
| } | |||
| } | |||
There was a problem hiding this comment.
Seems like maybe we should have fold_subitems_with be unimplemented! on such cases.
There was a problem hiding this comment.
@nikomatsakis My justification for having fold_subitems_with default to fold_with was that for types without corresponding methods in TypeVisitor, folding the subitems of the type is the same as folding the type.
On second though, I think it would nicest to have fold_with default to fold_subitems_with so that structural implementations always define fold_subitem_with to call fold_with on subitems and only define fold_with to call a method in TypeFolder when there is a corresponding method.
Did you want fold_subitems_with to be unimplemented! to reflect that users should only be calling fold_subitems_with when it is different from fold_with? If so, I think it would be better to enforce that at the type level by having a trait like TypeFolderOverridable define fold_subitems_with, or by using the original super_* setup.
There was a problem hiding this comment.
Did you want fold_subitems_with to be unimplemented! to reflect that users should only be calling fold_subitems_with when it is different from fold_with?
I think my reasoning was that outside users should always call fold_with, and the only reason to have fold_subitems_with was to use from the visitor method itself. I'm trying to think if there would be a correct scenario for calling fold_subitems_with on one of these cases where we haven't allowed for overriding in the visitor.
I think I agree with this comment though:
On second though, I think it would nicest to have fold_with default to fold_subitems_with so that structural implementations always define fold_subitem_with
...in the MIR visitor, I actually used the name super_fold_with, which I personally find more intuitive than fold_subitems_with. It also makes it clear that this is not something you normally want to call. Otherwise my setup was just as you described:
https://github.com/rust-lang/rust/blob/master/src/librustc/mir/visit.rs#L21-L23
Would you mind refactoring to use the super naming scheme and to reverse the defaults?
jseyfried
force-pushed
the
refactor_type_folder
branch
2 times, most recently
from
9bb241b
to
14c8af1
Compare
|
I rebased since a |
jseyfried
force-pushed
the
refactor_type_folder
branch
2 times, most recently
from
d99cc6c
to
aff9a33
Compare
|
☔ The latest upstream changes (presumably #30325) made this pull request unmergeable. Please resolve the merge conflicts. |
jseyfried
force-pushed
the
refactor_type_folder
branch
3 times, most recently
from
5d9a165
to
909b407
Compare
|
☔ The latest upstream changes (presumably #30389) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@jseyfried any thoughts on this question:
I still think that'd be somewhat more clear overall. But this is a nice PR, I'd hate to see it bitrot away. |
jseyfried
force-pushed
the
refactor_type_folder
branch
2 times, most recently
from
dedfed1
to
2a2bae7
Compare
|
☔ The latest upstream changes (presumably #30532) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Argh! @jseyfried I did not see that you had pushed those new commits! (Leaving a comment can help in that regard, since it generates an e-mail.) They look good though, r+ after rebase. |
jseyfried
force-pushed
the
refactor_type_folder
branch
from
2a2bae7
to
6327563
Compare
|
@nikomatsakis Just rebased. Sorry for the delay on this PR, I was off grid for a while over the holidays. I hadn't commented yet since I wanted to check that Travis CI was passing first -- the build was failing less than a minute in for a while last night for some reason but it looks like that got resolved. I had chosen |
|
@jseyfried thanks for updating it! |
|
@bors r+ |
|
📌 Commit 6327563 has been approved by |
`TypeFoldable`s can currently be visited inefficiently with an identity folder that is run only for its side effects. This creates a more efficient visitor for `TypeFoldable`s and uses it to implement `RegionEscape` and `HasProjectionTypes`, fixing cleanup issue #20298. This is a pure refactoring.
TypeFoldables can currently be visited inefficiently with an identity folder that is run only for its side effects. This creates a more efficient visitor forTypeFoldables and uses it to implementRegionEscapeandHasProjectionTypes, fixing cleanup issue #20298.This is a pure refactoring.