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.

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

Stabilize RFC3324 dyn upcasting coercion #118133

Merged
merged 1 commit into from Nov 22, 2023

Conversation

Urgau
Copy link
Contributor

@Urgau Urgau commented Nov 21, 2023

This PR stabilize the trait_upcasting feature, aka rust-lang/rfcs#3324.

The FCP was completed here: #65991 (comment).

And also remove the deref_into_dyn_supertrait lint which is now handled by dyn upcasting coercion.

Heavily inspired by #101718
Fixes #65991

@rustbot
Copy link
Collaborator

rustbot commented Nov 21, 2023

r? @b-naber

(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. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Nov 21, 2023
@rustbot
Copy link
Collaborator

rustbot commented Nov 21, 2023

The Miri subtree was changed

cc @rust-lang/miri

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@lcnr
Copy link
Contributor

lcnr commented Nov 21, 2023

Should we keep the deref_into_dyn_supertrait lint around, maybe changing its wording? These impls probably don't do what the user expects them to, given that they can't be used implicitly. I feel like pointing out "hey, this impl is now implicit via a coercion, consider removing it" is preferable to completely removing the lint

@Urgau
Copy link
Contributor Author

Urgau commented Nov 21, 2023

Sure, if those impls don't do what the user expects we should tell them.

What about this diagnostic change?

- warning: `dyn Foo<'_>` implements `Deref` with supertrait `Bar<'_>` as target
+ warning: `dyn Foo<'_>` implements `Deref` with supertrait `Bar<'_>` as target is shadowed by coercion
   --> $DIR/migrate-lint-deny-regions.rs:8:1
    |
 LL | impl<'a> Deref for dyn Foo<'a> {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 ...
 LL |     type Target = dyn Bar<'a>;
    |     -------------------------- target type is set here
    |
-   = warning: this will change its meaning in a future release!
-   = note: for more information, see issue #89460 <https://github.com/rust-lang/rust/issues/89460>
+   = help: consider removing this implementation in favour for the implicit coercion

@lcnr
Copy link
Contributor

lcnr commented Nov 21, 2023

maybe something like

- warning: `dyn Foo<'_>` implements `Deref` with supertrait `Bar<'_>` as target
+ warning: this `Deref` implementation is covered by the implicit super-trait coercion
   --> $DIR/migrate-lint-deny-regions.rs:8:1
    |
 LL | impl<'a> Deref for dyn Foo<'a> {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 ...
 LL |     type Target = dyn Bar<'a>;
    |     -------------------------- target type is set here
    |
-   = warning: this will change its meaning in a future release!
-   = note: for more information, see issue #89460 <https://github.com/rust-lang/rust/issues/89460>
+   = help: consider removing this implementation or replacing it with a method instead

something like this maybe? 🤔

@Urgau
Copy link
Contributor Author

Urgau commented Nov 21, 2023

Done. I've restored the lint, removed the future incompatibility mark and changed the diagnostic output as discussed.

Copy link
Member

@WaffleLapkin WaffleLapkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r? WaffleLapkin
r=me with the nit fixed


Also, could you open a reference PR editing this page?

src/tools/miri/tests/fail/dyn-upcast-trait-mismatch.rs Outdated Show resolved Hide resolved
@rustbot rustbot assigned WaffleLapkin and unassigned b-naber Nov 22, 2023
@WaffleLapkin
Copy link
Member

@bors delegate+

@bors
Copy link
Contributor

bors commented Nov 22, 2023

✌️ @Urgau, you can now approve this pull request!

If @WaffleLapkin told you to "r=me" after making some further change, please make that change, then do @bors r=@WaffleLapkin

Aka trait_upcasting feature.

And also adjust the `deref_into_dyn_supertrait` lint.
@Urgau
Copy link
Contributor Author

Urgau commented Nov 22, 2023

Also, could you open a reference PR editing this page?

@crlf0710 already has one opened rust-lang/reference#1259 (from his previous attempt as stabilizing this feature).

@Urgau
Copy link
Contributor Author

Urgau commented Nov 22, 2023

@bors r=@WaffleLapkin

@bors
Copy link
Contributor

bors commented Nov 22, 2023

📌 Commit 4c2d6de has been approved by WaffleLapkin

It is now in the queue for this repository.

@bors 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 Nov 22, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 22, 2023
…affleLapkin

Stabilize RFC3324 dyn upcasting coercion

This PR stabilize the `trait_upcasting` feature, aka rust-lang/rfcs#3324.

The FCP was completed here: rust-lang#65991 (comment).

~~And also remove the `deref_into_dyn_supertrait` lint which is now handled by dyn upcasting coercion.~~

Heavily inspired by rust-lang#101718
Fixes rust-lang#65991
@bors
Copy link
Contributor

bors commented Nov 22, 2023

⌛ Testing commit 4c2d6de with merge 6d2b84b...

@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@bors
Copy link
Contributor

bors commented Nov 22, 2023

☀️ Test successful - checks-actions
Approved by: WaffleLapkin
Pushing 6d2b84b to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 22, 2023
@bors bors merged commit 6d2b84b into rust-lang:master Nov 22, 2023
12 checks passed
@rustbot rustbot added this to the 1.76.0 milestone Nov 22, 2023
@Urgau
Copy link
Contributor Author

Urgau commented Nov 22, 2023

@rustbot label +relnotes

@rustbot rustbot added the relnotes Marks issues that should be documented in the release notes of the next release. label Nov 22, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (6d2b84b): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.2% [0.2%, 0.2%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.6% [0.4%, 0.7%] 4
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.1% [-2.1%, -2.1%] 1
All ❌✅ (primary) - - 0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-2.1% [-2.4%, -1.9%] 3
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -2.1% [-2.4%, -1.9%] 3

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 675.017s -> 675.132s (0.02%)
Artifact size: 313.79 MiB -> 313.79 MiB (0.00%)

@gabrielecastellano
Copy link

Hi, thanks for this!
I was wondering why this was not included in release 1.75.0.

@Urgau
Copy link
Contributor Author

Urgau commented Jan 4, 2024

I was wondering why this was not included in release 1.75.0.

Because when this PR landed on the 22 of November we were already in track for 1.76.
In general you can look at the milestone of each PR to see in which version it will be included.

@gabrielecastellano
Copy link

Thank you for the explanation!

oli-obk added a commit to oli-obk/rust that referenced this pull request Jan 22, 2024
…ing, r=WaffleLapkin"

This reverts commit 6d2b84b, reversing
changes made to 73bc121.
oli-obk added a commit to oli-obk/rust that referenced this pull request Jan 22, 2024
…ing, r=WaffleLapkin"

This reverts commit 6d2b84b, reversing
changes made to 73bc121.
oli-obk added a commit to oli-obk/rust that referenced this pull request Jan 22, 2024
…ing, r=WaffleLapkin"

This reverts commit 6d2b84b, reversing
changes made to 73bc121.
oli-obk added a commit to oli-obk/rust that referenced this pull request Jan 22, 2024
…ing, r=WaffleLapkin"

This reverts commit 6d2b84b, reversing
changes made to 73bc121.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 22, 2024
…abilization, r=lcnr

Revert stabilization of trait_upcasting feature

Reverts rust-lang#118133

This reverts commit 6d2b84b, reversing changes made to 73bc121.

The feature has a soundness bug:

* rust-lang#120222

It is unclear to me whether we'll actually want to destabilize, but I thought it was still prudent to open the PR for easy destabilization once we get there.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 22, 2024
…abilization, r=lcnr

Revert stabilization of trait_upcasting feature

Reverts rust-lang#118133

This reverts commit 6d2b84b, reversing changes made to 73bc121.

The feature has a soundness bug:

* rust-lang#120222

It is unclear to me whether we'll actually want to destabilize, but I thought it was still prudent to open the PR for easy destabilization once we get there.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 23, 2024
Rollup merge of rust-lang#120233 - oli-obk:revert_trait_obj_upcast_stabilization, r=lcnr

Revert stabilization of trait_upcasting feature

Reverts rust-lang#118133

This reverts commit 6d2b84b, reversing changes made to 73bc121.

The feature has a soundness bug:

* rust-lang#120222

It is unclear to me whether we'll actually want to destabilize, but I thought it was still prudent to open the PR for easy destabilization once we get there.
oli-obk added a commit to oli-obk/rust that referenced this pull request Jan 23, 2024
…ing, r=WaffleLapkin"

This reverts commit 6d2b84b, reversing
changes made to 73bc121.
oli-obk added a commit to oli-obk/rust that referenced this pull request Jan 24, 2024
…ing, r=WaffleLapkin"

This reverts commit 6d2b84b, reversing
changes made to 73bc121.
oli-obk added a commit to oli-obk/rust that referenced this pull request Jan 24, 2024
…ing, r=WaffleLapkin"

This reverts commit 6d2b84b, reversing
changes made to 73bc121.
MabezDev pushed a commit to esp-rs/rust that referenced this pull request Feb 9, 2024
…ing, r=WaffleLapkin"

This reverts commit 6d2b84b, reversing
changes made to 73bc121.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. relnotes Marks issues that should be documented in the release notes of the next release. 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. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tracking issue for dyn upcasting coercion
10 participants