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

Make WHERE_CLAUSES_OBJECT_SAFETY a regular object safety violation #125380

Merged
merged 2 commits into from
Jun 4, 2024

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented May 21, 2024

The issue

In #50781, we have known about unsound where clauses in function arguments:

trait Impossible {}

trait Foo {
    fn impossible(&self)
    where
        Self: Impossible;
}

impl Foo for &() {
    fn impossible(&self)
    where
        Self: Impossible,
    {}
}

// `where` clause satisfied for the object, meaning that the function now *looks* callable.
impl Impossible for dyn Foo {}

fn main() {
    let x: &dyn Foo = &&();
    x.impossible();
}

... which currently segfaults at runtime because we try to call a method in the vtable that doesn't exist. :(

What did u change

This PR removes the WHERE_CLAUSES_OBJECT_SAFETY lint and instead makes it a regular object safety violation. I choose to make this into a hard error immediately rather than a deny because of the time that has passed since this lint was authored, and the single (1) regression (see below).

That means that it's OK to mention where Self: Trait where clauses in your trait, but making such a trait into a dyn Trait object will report an object safety violation just like where Self: Sized, etc.

trait Impossible {}

trait Foo {
    fn impossible(&self)
    where
        Self: Impossible; // <~ This definition is valid, just not object-safe.
}

impl Foo for &() {
    fn impossible(&self)
    where
        Self: Impossible,
    {}
}

fn main() {
    let x: &dyn Foo = &&(); // <~ THIS is where we emit an error.
}

Regressions

From a recent crater run, there's only one crate that relies on this behavior: #124305 (comment). The crate looks unmaintained and there seems to be no dependents.

Further

We may later choose to relax this (e.g. when the where clause is implied by the supertraits of the trait or something), but this is not something I propose to do in this FCP.

For example, given:

trait Tr {
  fn f(&self) where Self: Blanket;
}

impl<T: ?Sized> Blanket for T {}

Proving that some placeholder S implements S: Blanket would be sufficient to prove that the same (blanket) impl applies for both Concerete: Blanket and dyn Trait: Blanket.

Repeating here that I don't think we need to implement this behavior right now.


r? lcnr

@rustbot rustbot added PG-exploit-mitigations Project group: Exploit mitigations 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-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative labels May 21, 2024
@rustbot
Copy link
Collaborator

rustbot commented May 21, 2024

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred in src/librustdoc/clean/types.rs

cc @camelid

HIR ty lowering was modified

cc @fmease

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

Some changes occurred in compiler/rustc_sanitizers

cc @rust-lang/project-exploit-mitigations, @rcvalle

@compiler-errors
Copy link
Member Author

@rfcbot fcp merge

@rfcbot

This comment was marked as off-topic.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels May 21, 2024
@compiler-errors
Copy link
Member Author

Sorry y'all lol

@rfcbot cancel

@rfcbot
Copy link

rfcbot commented May 21, 2024

@compiler-errors proposal cancelled.

@rfcbot rfcbot removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels May 21, 2024
@compiler-errors compiler-errors added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. T-types Relevant to the types team, which will review and decide on the PR/issue. and removed T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 21, 2024
@compiler-errors
Copy link
Member Author

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented May 21, 2024

Team member @compiler-errors has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels May 24, 2024
@rfcbot
Copy link

rfcbot commented May 24, 2024

🔔 This is now entering its final comment period, as per the review above. 🔔

@bors
Copy link
Contributor

bors commented May 27, 2024

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

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Jun 3, 2024
@rfcbot
Copy link

rfcbot commented Jun 3, 2024

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 3, 2024
…li-obk

Make `WHERE_CLAUSES_OBJECT_SAFETY` a regular object safety violation

#### The issue

In rust-lang#50781, we have known about unsound `where` clauses in function arguments:

```rust
trait Impossible {}

trait Foo {
    fn impossible(&self)
    where
        Self: Impossible;
}

impl Foo for &() {
    fn impossible(&self)
    where
        Self: Impossible,
    {}
}

// `where` clause satisfied for the object, meaning that the function now *looks* callable.
impl Impossible for dyn Foo {}

fn main() {
    let x: &dyn Foo = &&();
    x.impossible();
}
```

... which currently segfaults at runtime because we try to call a method in the vtable that doesn't exist. :(

#### What did u change

This PR removes the `WHERE_CLAUSES_OBJECT_SAFETY` lint and instead makes it a regular object safety violation. I choose to make this into a hard error immediately rather than a `deny` because of the time that has passed since this lint was authored, and the single (1) regression (see below).

That means that it's OK to mention `where Self: Trait` where clauses in your trait, but making such a trait into a `dyn Trait` object will report an object safety violation just like `where Self: Sized`, etc.

```rust
trait Impossible {}

trait Foo {
    fn impossible(&self)
    where
        Self: Impossible; // <~ This definition is valid, just not object-safe.
}

impl Foo for &() {
    fn impossible(&self)
    where
        Self: Impossible,
    {}
}

fn main() {
    let x: &dyn Foo = &&(); // <~ THIS is where we emit an error.
}
```

#### Regressions

From a recent crater run, there's only one crate that relies on this behavior: rust-lang#124305 (comment). The crate looks unmaintained and there seems to be no dependents.

#### Further

We may later choose to relax this (e.g. when the where clause is implied by the supertraits of the trait or something), but this is not something I propose to do in this FCP.

For example, given:

```
trait Tr {
  fn f(&self) where Self: Blanket;
}

impl<T: ?Sized> Blanket for T {}
```

Proving that some placeholder `S` implements `S: Blanket` would be sufficient to prove that the same (blanket) impl applies for both `Concerete: Blanket` and `dyn Trait: Blanket`.

Repeating here that I don't think we need to implement this behavior right now.

----

r? lcnr
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 3, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#122597 (Show files produced by `--emit foo` in json artifact notifications)
 - rust-lang#124486 (Add tracking issue and unstable book page for `"vectorcall"` ABI)
 - rust-lang#125380 (Make `WHERE_CLAUSES_OBJECT_SAFETY` a regular object safety violation)
 - rust-lang#125690 (ARM Target Docs Update)
 - rust-lang#125865 (Fix ICE caused by ignoring EffectVars in type inference)
 - rust-lang#125893 (Handle all GVN binops in a single place.)
 - rust-lang#125909 (rustdoc: add a regression test for a former blanket impl synthesis ICE)
 - rust-lang#125918 (Revert: create const block bodies in typeck via query feeding)
 - rust-lang#125919 (Remove stray "this")

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 3, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#122597 (Show files produced by `--emit foo` in json artifact notifications)
 - rust-lang#124486 (Add tracking issue and unstable book page for `"vectorcall"` ABI)
 - rust-lang#125380 (Make `WHERE_CLAUSES_OBJECT_SAFETY` a regular object safety violation)
 - rust-lang#125690 (ARM Target Docs Update)
 - rust-lang#125865 (Fix ICE caused by ignoring EffectVars in type inference)
 - rust-lang#125893 (Handle all GVN binops in a single place.)
 - rust-lang#125909 (rustdoc: add a regression test for a former blanket impl synthesis ICE)
 - rust-lang#125918 (Revert: create const block bodies in typeck via query feeding)
 - rust-lang#125919 (Remove stray "this")

r? `@ghost`
`@rustbot` modify labels: rollup
@matthiaskrgr
Copy link
Member

@bors r- rollup=iffy

#125931

@bors 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 3, 2024
@compiler-errors
Copy link
Member Author

I tried blessing the codegen tests and nothing happened

@bors r=oli-obk rollup=never

@bors
Copy link
Contributor

bors commented Jun 3, 2024

📌 Commit 511f1cf has been approved by oli-obk

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 3, 2024
@bors
Copy link
Contributor

bors commented Jun 3, 2024

⌛ Testing commit 511f1cf with merge e6ebf60...

bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 3, 2024
…-obk

Make `WHERE_CLAUSES_OBJECT_SAFETY` a regular object safety violation

#### The issue

In rust-lang#50781, we have known about unsound `where` clauses in function arguments:

```rust
trait Impossible {}

trait Foo {
    fn impossible(&self)
    where
        Self: Impossible;
}

impl Foo for &() {
    fn impossible(&self)
    where
        Self: Impossible,
    {}
}

// `where` clause satisfied for the object, meaning that the function now *looks* callable.
impl Impossible for dyn Foo {}

fn main() {
    let x: &dyn Foo = &&();
    x.impossible();
}
```

... which currently segfaults at runtime because we try to call a method in the vtable that doesn't exist. :(

#### What did u change

This PR removes the `WHERE_CLAUSES_OBJECT_SAFETY` lint and instead makes it a regular object safety violation. I choose to make this into a hard error immediately rather than a `deny` because of the time that has passed since this lint was authored, and the single (1) regression (see below).

That means that it's OK to mention `where Self: Trait` where clauses in your trait, but making such a trait into a `dyn Trait` object will report an object safety violation just like `where Self: Sized`, etc.

```rust
trait Impossible {}

trait Foo {
    fn impossible(&self)
    where
        Self: Impossible; // <~ This definition is valid, just not object-safe.
}

impl Foo for &() {
    fn impossible(&self)
    where
        Self: Impossible,
    {}
}

fn main() {
    let x: &dyn Foo = &&(); // <~ THIS is where we emit an error.
}
```

#### Regressions

From a recent crater run, there's only one crate that relies on this behavior: rust-lang#124305 (comment). The crate looks unmaintained and there seems to be no dependents.

#### Further

We may later choose to relax this (e.g. when the where clause is implied by the supertraits of the trait or something), but this is not something I propose to do in this FCP.

For example, given:

```
trait Tr {
  fn f(&self) where Self: Blanket;
}

impl<T: ?Sized> Blanket for T {}
```

Proving that some placeholder `S` implements `S: Blanket` would be sufficient to prove that the same (blanket) impl applies for both `Concerete: Blanket` and `dyn Trait: Blanket`.

Repeating here that I don't think we need to implement this behavior right now.

----

r? lcnr
@rust-log-analyzer
Copy link
Collaborator

The job aarch64-gnu failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
   Compiling rustdoc v0.0.0 (/checkout/src/librustdoc)
[RUSTC-TIMING] tracing_tree test:false 1.163
[RUSTC-TIMING] tracing_subscriber test:false 6.675
[RUSTC-TIMING] regex test:false 9.746
##[error]The runner has received a shutdown signal. This can happen when the runner service is stopped, or a manually started runner is canceled.

Session terminated, killing shell...::group::Clock drift check
  network time:  ...killed.
##[error]The operation was canceled.
Cleaning up orphan processes

@bors
Copy link
Contributor

bors commented Jun 4, 2024

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 4, 2024
@fmease
Copy link
Member

fmease commented Jun 4, 2024

@bors retry

@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 Jun 4, 2024
@bors
Copy link
Contributor

bors commented Jun 4, 2024

⌛ Testing commit 511f1cf with merge 90d6255...

@bors
Copy link
Contributor

bors commented Jun 4, 2024

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 90d6255 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 4, 2024
@bors bors merged commit 90d6255 into rust-lang:master Jun 4, 2024
7 checks passed
@rustbot rustbot added this to the 1.80.0 milestone Jun 4, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (90d6255): 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.6% [0.1%, 1.1%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (secondary -5.5%)

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)
- - 0
Improvements ✅
(secondary)
-5.5% [-5.5%, -5.5%] 1
All ❌✅ (primary) - - 0

Cycles

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

Binary size

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

Bootstrap: 673.132s -> 673.151s (0.00%)
Artifact size: 318.94 MiB -> 319.02 MiB (0.02%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. merged-by-bors This PR was explicitly merged by bors. PG-exploit-mitigations Project group: Exploit mitigations S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-types Relevant to the types team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet