Reduce boilerplate around infallible folders #91318
Merged
Conversation
This file contains 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
rust-highfive
added
the
S-waiting-on-review
|
Argh, this builds on #91230 but obviously once those commits are merged to master we're only really looking at the final commits here. Do you want me to close and wait for that before opening this PR? |
|
No need to close. Once it's merged into master, a rebase will make those commits disappear. |
|
Because this PR removes a lot of the boilerplate that #91230 introduced, it probably also negates most of the triggered merge conflicts. It may also be easier to review in aggregate with the other PR? Let me know if you'd like me to try breaking it up into smaller commits. |
eggyal
force-pushed
the
reduce-boilerplate-around-infallible-folders
branch
from
9dd77ef
to
5eef980
Compare
|
☔ The latest upstream changes (presumably #91299) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Okay, I was playing around with some alternative APIs (https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=49f3a0ef81b49627898e097883afbebe). Namely, I was trying to see if we need the extra I don't think we need I do think we need @eggyal can you remove |
eggyal
force-pushed
the
reduce-boilerplate-around-infallible-folders
branch
from
5eef980
to
619ce2c
Compare
|
This looks a lot better imo. I do want to just cc @rust-lang/compiler for this. This is a followup of #91230, which implements rust-lang/compiler-team#432. It slightly alters the Given that, I'll wait until tomorrow to r+ in case there are any objections. |
|
I quite like this change 👍 prefer the name While I am not completely happy with the way this is implemented, I can't think of a better way with the way rusts default trait impls work. Might be interesting to think of ways to change Rust so that we're be able to represent this using only one trait, but that's not necessary for this PR. imo it's fine to merge this without an MCP |
lcnr
reviewed
Dec 1, 2021
|
📌 Commit 33706ce9cbba3d4bb2c157200e61320ea4d3835c has been approved by |
bors
added
S-waiting-on-bors
|
✌️ @eggyal can now approve this pull request |
|
⌛ Testing commit 33706ce9cbba3d4bb2c157200e61320ea4d3835c with merge 913232afa15adc00815eaaaf7b9e1119d6b17363... |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
eggyal
force-pushed
the
reduce-boilerplate-around-infallible-folders
branch
from
33706ce
to
c6f057d
Compare
|
@bors retry |
bors
added
S-waiting-on-bors
|
@bors r+ seems like bors didn't pick it up |
|
📌 Commit c6f057d5ea6093ba7490ad99f158a8d2efcbf9b0 has been approved by |
apiraino
added
the
T-compiler
eggyal
force-pushed
the
reduce-boilerplate-around-infallible-folders
branch
from
c6f057d
to
cf683e6
Compare
|
@bors r=jackh726 |
|
📌 Commit cf683e6 has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
This was referenced Dec 2, 2021
|
Finished benchmarking commit (acbe444): comparison url. Summary: This change led to moderate relevant mixed results 🤷 in compiler performance.
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression |
cachegrind diff for the worst scenario (deeply-nested full check)Not sure whether this is again optimisation effects of LLVM/PGO, or whether perhaps (some of) the newly added delegating methods should be |
JohnTitor
added a commit
to JohnTitor/rust-semverver
that referenced
this pull request
Apr 15, 2022
eggyal
added a commit
to eggyal/rust
that referenced
this pull request
Jun 20, 2022
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
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Jun 21, 2022
…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
added a commit
to JohnTitor/rust
that referenced
this pull request
Jun 21, 2022
…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``
eggyal
added a commit
to eggyal/rust
that referenced
this pull request
Jun 21, 2022
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
Dylan-DPC
added a commit
to Dylan-DPC/rust
that referenced
this pull request
Jun 24, 2022
…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`
Dylan-DPC
added a commit
to Dylan-DPC/rust
that referenced
this pull request
Jun 25, 2022
…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
added a commit
to JohnTitor/rust
that referenced
this pull request
Jun 25, 2022
…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```
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Jun 25, 2022
…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````
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Further to #91230 (comment)
r? @jackh726