Reverse folder hierarchy by eggyal · Pull Request #98311 · rust-lang/rust

eggyal · 2022-06-20T20:11:10Z

#91318 introduced a trait for infallible folders distinct from the fallible version. For some reason (completely unfathomable to me now that I look at it with fresh eyes), the infallible trait was a supertrait of the fallible one: that is, all fallible folders were required to also be infallible. Moreover the Error associated type was defined on the infallible trait! It's so absurd that it has me questioning whether I was entirely sane.

This trait reverses the hierarchy, so that the fallible trait is a supertrait of the infallible one: all infallible folders are required to also be fallible (which is a trivial blanket implementation). This of course makes much more sense! It also enables the Error associated type to sit on the fallible trait, where it sensibly belongs.

There is one downside however: folders expose a tcx accessor method. Since the blanket fallible implementation for infallible folders only has access to a generic F: TypeFolder, we need that trait to expose such an accessor to which we can delegate. Alternatively it's possible to extract that accessor into a separate HasTcx trait (or similar) that would then be a supertrait of both the fallible and infallible folder traits: this would ensure that there's only one unambiguous tcx method, at the cost of a little additional boilerplate. If desired, I can submit that as a separate PR.

r? @jackh726

jackh726 · 2022-06-20T21:21:28Z

compiler/rustc_infer/src/infer/resolve.rs

                    .unwrap_region_constraints()
                    .opportunistic_resolve_var(rid);
-                self.tcx().reuse_or_mk_region(r, ty::ReVar(resolved))
+                TypeFolder::tcx(self).reuse_or_mk_region(r, ty::ReVar(resolved))


Does FallibleTypeFolder::tcx(self) not work?

Yeah, it does; but what difference?

Just thinking about why we can't remove TypeFolder::tcx in favor of just using FallibleTypeFolder::tcx (since it's a super trait)

Because the blanket impl needs to get the tcx from somewhere, but only has a generic F: TypeFolder.

Shall I pull the tcx() accessor out into its own trait?

jackh726 · 2022-06-20T21:22:25Z

Which is implemented more? Would it make more sense to rename TypeFolder -> InfallibleTypeFolder and FallibleTypeFolder -> TypeFolder?

eggyal · 2022-06-20T21:52:40Z

The infallible version has 33 implementations; the fallible one has only 3. So the way we already have it is perhaps a little more convenient.

jackh726 · 2022-06-20T23:30:20Z

@bors r+

bors · 2022-06-20T23:30:23Z

📌 Commit 2526b08 has been approved by jackh726

bors · 2022-06-20T23:30:24Z

🌲 The tree is currently closed for pull requests below priority 1000. This pull request will be tested once the tree is reopened.

…jackh726 Reverse folder hierarchy rust-lang#91318 introduced a trait for infallible folders distinct from the fallible version. For some reason (completely unfathomable to me now that I look at it with fresh eyes), the infallible trait was a supertrait of the fallible one: that is, all fallible folders were required to also be infallible. Moreover the `Error` associated type was defined on the infallible trait! It's so absurd that it has me questioning whether I was entirely sane. This trait reverses the hierarchy, so that the fallible trait is a supertrait of the infallible one: all infallible folders are required to also be fallible (which is a trivial blanket implementation). This of course makes much more sense! It also enables the `Error` associated type to sit on the fallible trait, where it sensibly belongs. There is one downside however: folders expose a `tcx` accessor method. Since the blanket fallible implementation for infallible folders only has access to a generic `F: TypeFolder`, we need that trait to expose such an accessor to which we can delegate. Alternatively it's possible to extract that accessor into a separate `HasTcx` trait (or similar) that would then be a supertrait of both the fallible and infallible folder traits: this would ensure that there's only one unambiguous `tcx` method, at the cost of a little additional boilerplate. If desired, I can submit that as a separate PR. r? `@jackh726`

…jackh726 Reverse folder hierarchy rust-lang#91318 introduced a trait for infallible folders distinct from the fallible version. For some reason (completely unfathomable to me now that I look at it with fresh eyes), the infallible trait was a supertrait of the fallible one: that is, all fallible folders were required to also be infallible. Moreover the `Error` associated type was defined on the infallible trait! It's so absurd that it has me questioning whether I was entirely sane. This trait reverses the hierarchy, so that the fallible trait is a supertrait of the infallible one: all infallible folders are required to also be fallible (which is a trivial blanket implementation). This of course makes much more sense! It also enables the `Error` associated type to sit on the fallible trait, where it sensibly belongs. There is one downside however: folders expose a `tcx` accessor method. Since the blanket fallible implementation for infallible folders only has access to a generic `F: TypeFolder`, we need that trait to expose such an accessor to which we can delegate. Alternatively it's possible to extract that accessor into a separate `HasTcx` trait (or similar) that would then be a supertrait of both the fallible and infallible folder traits: this would ensure that there's only one unambiguous `tcx` method, at the cost of a little additional boilerplate. If desired, I can submit that as a separate PR. r? ``@jackh726``

JohnTitor · 2022-06-21T11:07:34Z

Failed in rollup: #98327 (comment)
@bors r-

eggyal · 2022-06-21T11:34:25Z

@rustbot ready

jackh726 · 2022-06-21T15:08:51Z

@bors r+

bors · 2022-06-21T15:08:53Z

📌 Commit 4038e66d8e25e4a078d7246cdaa0885f94355322 has been approved by jackh726

bors · 2022-06-21T16:25:24Z

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

jackh726 · 2022-06-21T16:36:35Z

@bors delegate+

r=me after rebase

bors · 2022-06-21T16:36:37Z

✌️ @eggyal can now approve this pull request

@jackh726

rust-lang#91318 introduced a trait for infallible folders distinct from the fallible version. For some reason (completely unfathomable to me now that I look at it with fresh eyes), the infallible trait was a supertrait of the fallible one: that is, all fallible folders were required to also be infallible. Moreover the `Error` associated type was defined on the infallible trait! It's so absurd that it has me questioning whether I was entirely sane. This trait reverses the hierarchy, so that the fallible trait is a supertrait of the infallible one: all infallible folders are required to also be fallible (which is a trivial blanket implementation). This of course makes much more sense! It also enables the `Error` associated type to sit on the fallible trait, where it sensibly belongs. There is one downside however: folders expose a `tcx` accessor method. Since the blanket fallible implementation for infallible folders only has access to a generic `F: TypeFolder`, we need that trait to expose such an accessor to which we can delegate. Alternatively it's possible to extract that accessor into a separate `HasTcx` trait (or similar) that would then be a supertrait of both the fallible and infallible folder traits: this would ensure that there's only one unambiguous `tcx` method, at the cost of a little additional boilerplate. If desired, I can submit that as a separate PR. r? @jackh726

eggyal · 2022-06-21T17:25:22Z

@bors r=jackh726

bors · 2022-06-21T17:25:23Z

📌 Commit 75203ee has been approved by jackh726

…jackh726 Reverse folder hierarchy rust-lang#91318 introduced a trait for infallible folders distinct from the fallible version. For some reason (completely unfathomable to me now that I look at it with fresh eyes), the infallible trait was a supertrait of the fallible one: that is, all fallible folders were required to also be infallible. Moreover the `Error` associated type was defined on the infallible trait! It's so absurd that it has me questioning whether I was entirely sane. This trait reverses the hierarchy, so that the fallible trait is a supertrait of the infallible one: all infallible folders are required to also be fallible (which is a trivial blanket implementation). This of course makes much more sense! It also enables the `Error` associated type to sit on the fallible trait, where it sensibly belongs. There is one downside however: folders expose a `tcx` accessor method. Since the blanket fallible implementation for infallible folders only has access to a generic `F: TypeFolder`, we need that trait to expose such an accessor to which we can delegate. Alternatively it's possible to extract that accessor into a separate `HasTcx` trait (or similar) that would then be a supertrait of both the fallible and infallible folder traits: this would ensure that there's only one unambiguous `tcx` method, at the cost of a little additional boilerplate. If desired, I can submit that as a separate PR. r? `@jackh726`

…jackh726 Reverse folder hierarchy rust-lang#91318 introduced a trait for infallible folders distinct from the fallible version. For some reason (completely unfathomable to me now that I look at it with fresh eyes), the infallible trait was a supertrait of the fallible one: that is, all fallible folders were required to also be infallible. Moreover the `Error` associated type was defined on the infallible trait! It's so absurd that it has me questioning whether I was entirely sane. This trait reverses the hierarchy, so that the fallible trait is a supertrait of the infallible one: all infallible folders are required to also be fallible (which is a trivial blanket implementation). This of course makes much more sense! It also enables the `Error` associated type to sit on the fallible trait, where it sensibly belongs. There is one downside however: folders expose a `tcx` accessor method. Since the blanket fallible implementation for infallible folders only has access to a generic `F: TypeFolder`, we need that trait to expose such an accessor to which we can delegate. Alternatively it's possible to extract that accessor into a separate `HasTcx` trait (or similar) that would then be a supertrait of both the fallible and infallible folder traits: this would ensure that there's only one unambiguous `tcx` method, at the cost of a little additional boilerplate. If desired, I can submit that as a separate PR. r? ``@jackh726``

…jackh726 Reverse folder hierarchy rust-lang#91318 introduced a trait for infallible folders distinct from the fallible version. For some reason (completely unfathomable to me now that I look at it with fresh eyes), the infallible trait was a supertrait of the fallible one: that is, all fallible folders were required to also be infallible. Moreover the `Error` associated type was defined on the infallible trait! It's so absurd that it has me questioning whether I was entirely sane. This trait reverses the hierarchy, so that the fallible trait is a supertrait of the infallible one: all infallible folders are required to also be fallible (which is a trivial blanket implementation). This of course makes much more sense! It also enables the `Error` associated type to sit on the fallible trait, where it sensibly belongs. There is one downside however: folders expose a `tcx` accessor method. Since the blanket fallible implementation for infallible folders only has access to a generic `F: TypeFolder`, we need that trait to expose such an accessor to which we can delegate. Alternatively it's possible to extract that accessor into a separate `HasTcx` trait (or similar) that would then be a supertrait of both the fallible and infallible folder traits: this would ensure that there's only one unambiguous `tcx` method, at the cost of a little additional boilerplate. If desired, I can submit that as a separate PR. r? ```@jackh726```

…askrgr Rollup of 9 pull requests Successful merges: - rust-lang#96412 (Windows: Iterative `remove_dir_all`) - rust-lang#98126 (Mitigate MMIO stale data vulnerability) - rust-lang#98149 (Set relocation_model to Pic on emscripten target) - rust-lang#98194 (Leak pthread_{mutex,rwlock}_t if it's dropped while locked.) - rust-lang#98298 (Point to type parameter definition when not finding variant, method and associated item) - rust-lang#98311 (Reverse folder hierarchy) - rust-lang#98401 (Add tracking issues to `--extern` option docs.) - rust-lang#98429 (Use correct substs in enum discriminant cast) - rust-lang#98431 (Suggest defining variable as mutable on `&mut _` type mismatch in pats) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

eggyal force-pushed the reverse-folder-hierarchy branch from 5ae67b2 to 77f8b93 Compare June 20, 2022 20:17

rust-highfive assigned jackh726 Jun 20, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 20, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 20, 2022

eggyal force-pushed the reverse-folder-hierarchy branch from 77f8b93 to 2526b08 Compare June 20, 2022 20:46

jackh726 reviewed Jun 20, 2022

View reviewed changes

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 20, 2022

matthiaskrgr mentioned this pull request Jun 21, 2022

Rollup of 6 pull requests #98321

Closed

JohnTitor mentioned this pull request Jun 21, 2022

Rollup of 7 pull requests #98327

Closed

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 21, 2022

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 Jun 21, 2022

eggyal force-pushed the reverse-folder-hierarchy branch 2 times, most recently from 2ffbd50 to 4038e66 Compare June 21, 2022 11:39

bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 21, 2022

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jun 21, 2022

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 21, 2022

eggyal added 2 commits June 21, 2022 17:38

Remove unecessary references to TypeFolder::Error

75203ee

eggyal force-pushed the reverse-folder-hierarchy branch from 4038e66 to 75203ee Compare June 21, 2022 16:39

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 21, 2022

Dylan-DPC mentioned this pull request Jun 24, 2022

Rollup of 9 pull requests #98455

Closed

Dylan-DPC mentioned this pull request Jun 25, 2022

Rollup of 9 pull requests #98480

Closed

JohnTitor mentioned this pull request Jun 25, 2022

Rollup of 9 pull requests #98484

Closed

matthiaskrgr mentioned this pull request Jun 25, 2022

Rollup of 9 pull requests #98486

Merged

bors merged commit 65187f5 into rust-lang:master Jun 25, 2022

rustbot added this to the 1.64.0 milestone Jun 25, 2022

eggyal deleted the reverse-folder-hierarchy branch June 28, 2022 06:04

Uh oh!

Conversation

eggyal commented Jun 20, 2022

Uh oh!

jackh726 Jun 20, 2022

Choose a reason for hiding this comment

Uh oh!

eggyal Jun 20, 2022

Choose a reason for hiding this comment

Uh oh!

jackh726 Jun 20, 2022

Choose a reason for hiding this comment

Uh oh!

eggyal Jun 21, 2022

Choose a reason for hiding this comment

Uh oh!

jackh726 commented Jun 20, 2022

Uh oh!

eggyal commented Jun 20, 2022

Uh oh!

jackh726 commented Jun 20, 2022

Uh oh!

bors commented Jun 20, 2022

Uh oh!

bors commented Jun 20, 2022

Uh oh!

JohnTitor commented Jun 21, 2022

Uh oh!

eggyal commented Jun 21, 2022

Uh oh!

jackh726 commented Jun 21, 2022

Uh oh!

bors commented Jun 21, 2022

Uh oh!

bors commented Jun 21, 2022

Uh oh!

jackh726 commented Jun 21, 2022

Uh oh!

bors commented Jun 21, 2022

Uh oh!

eggyal commented Jun 21, 2022

Uh oh!

bors commented Jun 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants