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

Clarify and tidy up explanation of E0038 #91387

Merged
merged 1 commit into from
Dec 3, 2021

Conversation

graydon
Copy link
Contributor

@graydon graydon commented Nov 30, 2021

I ran into E0038 (specifically the Self:Sized constraint on object-safety) the other day and it seemed to me that the explanations I found floating around the internet were a bit .. wrong. Like they didn't make sense. And then I went and checked the official explanation here and it didn't make sense either.

As far as I can tell (reading through the history of the RFCs), two totally different aspects of object-safety have got tangled up in much of the writing on the subject:

  • Object-safety related to "not even theoretically possible" issues. This includes things like "methods that take or return Self by value", which obviously will never work for an unsized type in a world with fixed-size stack frames (and it'd be an opaque type anyways, which, ugh). This sort of thing was originally decided method-by-method, with non-object-safe methods stripped from objects; but in RFC 0255 this sort of per-impossible-method reasoning was made into a per-trait safety property (with the escape hatch left in where users could mark methods where Self:Sized to have them stripped before the trait's object safety is considered).
  • Object-safety related to "totally possible but ergonomically a little awkward" issues. Specifically in a trait with Trait:Sized, there's no a priori reason why this constraint makes the trait impossible to make into an object -- imagine it had nothing but harmless &self-taking methods. No problem! Who cares if the Trait requires its implementing types to be sized? As far as I can tell reading the history here, in both RFC 0255 and then later in RFC 0546 it seems that the motivation for making Trait:Sized be non-object-safe has nothing to do with the impossibility of making objects out of such types, and everything to do with enabling "a trait object SomeTrait to implement the trait SomeTrait". That is, since dyn Trait is unsized, if Trait:Sized then you can never have the automatic (and reasonable) ergonomic implicit impl Trait for dyn Trait. And the authors of that RFC really wanted that automatic implicit implementation of Trait for dyn Trait. So they just defined Trait:Sized as non-object safe -- no dyn Trait can ever exist that the compiler can't synthesize such an impl for. Well enough!

However, I noticed in my reading-and-reconstruction that lots of documentation on the internet, including forum and Q&A site answers and (most worrying) the compiler explanation all kinda grasp at something like the first ("not theoretically possible") explanation, and fail to mention the second ("just an ergonomic constraint") explanation. So I figured I'd clean up the docs to clarify, maybe confuse the next person less (unless of course I'm misreading the history here and misunderstanding motives -- please let me know if so!)

While here I also did some cleanups:

  • Rewrote the preamble, trying to help the user get a little better oriented (I found the existing preamble a bit scattered).
  • Modernized notation (using dyn Trait)
  • Changed the section headings to all be written with the same logical sense: to all be written as "conditions that violate object safety" rather than a mix of that and the negated form "conditions that must not happen in order to ensure object safety".

I think there's a fair bit more to clean up in this doc -- the later sections get a bit rambly and I suspect there should be a completely separated-out section covering the where Self:Sized escape hatch for instructing the compiler to "do the old thing" and strip methods off traits when turning them into objects (it's a bit buried as a digression in the individual sub-error sections). But I did what I had time for now.

@rust-highfive
Copy link
Collaborator

r? @wesleywiser

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 30, 2021
@rust-log-analyzer

This comment has been minimized.

@graydon graydon force-pushed the E0038-clarification branch 2 times, most recently from 02c89c2 to 024162e Compare November 30, 2021 07:53
Copy link
Member

@wesleywiser wesleywiser left a comment

Choose a reason for hiding this comment

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

Just a few small comments. Overall, this seems like a great improvement over the current explanation!


Attempting to create a trait object for a non object-safe trait will trigger
this error.
In earlier editions of Rust, trait object types were written as plain `Trait`
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this sentence should move into the introduction or right after where dyn Trait is mentioned for the first time? As it is, it feels a bit like it's spliced between parts of the "what is/is not object safe" discussion.

Copy link
Member

Choose a reason for hiding this comment

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

Something like:

For any given trait Trait there may be a related type called the trait
object type
which is typically written as dyn Trait. In earlier editions of Rust,
trait object types were written as plain Trait (just the name of the trait, written in
type positions) but this was a bit too confusing, so we now write dyn Trait.

Not all traits are allowed to be used as trait object types. The traits that are allowed
to be used as trait object types are called "object-safe" traits. Attempting to use a trait
object type for a trait that is not object-safe will trigger error E0038.

Two general aspects of trait object types give rise to the restrictions:
...

Flows a bit better IMO but this is just a suggestion 🙂

Copy link
Contributor Author

@graydon graydon Nov 30, 2021

Choose a reason for hiding this comment

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

I had it like that at first, but then I rewrote to the current form because I wanted to preserve a nice thing about the existing docs, which is that it says "why you got this error" in a hurry -- "not all traits are allowed to be used as object types". That's the meat of the explanation.

But sure, if you like I'm happy to switch it back. I agree that it's a little more connected to the content in the first paragraph, just makes it longer and delays getting to the point.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that's a good point. A quick look at some of the other errors in this file suggests we don't follow a consistent pattern; some errors get straight to the point while others have more upfront exposition.

I think either version is better than what we currently have so I'm going to r+. If you feel strongly that you like your original version better, I'm also ok with landing that instead of the current version. Just let me know if you want to do that.

compiler/rustc_error_codes/src/error_codes/E0038.md Outdated Show resolved Hide resolved
@graydon
Copy link
Contributor Author

graydon commented Nov 30, 2021

Pushed suggested updates. Thanks for the quick review!

@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 1, 2021
@wesleywiser
Copy link
Member

Thanks @graydon!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Dec 2, 2021

📌 Commit 7907fa8 has been approved by wesleywiser

@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 Dec 2, 2021
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 2, 2021
…eywiser

Clarify and tidy up explanation of E0038

I ran into E0038 (specifically the `Self:Sized` constraint on object-safety) the other day and it seemed to me that the explanations I found floating around the internet were a bit .. wrong. Like they didn't make sense. And then I went and checked the official explanation here and it didn't make sense either.

As far as I can tell (reading through the history of the RFCs), two totally different aspects of object-safety have got tangled up in much of the writing on the subject:
  - Object-safety related to "not even theoretically possible" issues. This includes things like "methods that take or return Self by value", which obviously will never work for an unsized type in a world with fixed-size stack frames (and it'd be an opaque type anyways, which, ugh). This sort of thing was originally decided method-by-method, with non-object-safe methods stripped from objects; but in [RFC 0255](https://rust-lang.github.io/rfcs/0255-object-safety.html) this sort of per-impossible-method reasoning was made into a per-trait safety property (with the escape hatch left in where users could mark methods `where Self:Sized` to have them stripped before the trait's object safety is considered).
  - Object-safety related to "totally possible but ergonomically a little awkward" issues. Specifically in a trait with `Trait:Sized`, there's no a priori reason why this constraint makes the trait impossible to make into an object -- imagine it had nothing but harmless `&self`-taking methods. No problem! Who cares if the Trait requires its implementing types to be sized? As far as I can tell reading the history here, in both RFC 0255 and then later in [RFC 0546](https://rust-lang.github.io/rfcs/0546-Self-not-sized-by-default.html) it seems that the motivation for making `Trait:Sized` be non-object-safe has _nothing to do_ with the impossibility of making objects out of such types, and everything to do with enabling "[a trait object SomeTrait to implement the trait SomeTrait](https://rust-lang.github.io/rfcs/0546-Self-not-sized-by-default.html#motivation)". That is, since `dyn Trait` is unsized, if `Trait:Sized` then you can never have the automatic (and reasonable) ergonomic implicit `impl Trait for dyn Trait`. And the authors of that RFC really wanted that automatic implicit implementation of `Trait` for `dyn Trait`. So they just defined `Trait:Sized` as non-object safe -- no `dyn Trait` can ever exist that the compiler can't synthesize such an impl for. Well enough!

However, I noticed in my reading-and-reconstruction that lots of documentation on the internet, including forum and Q&A site answers and (most worrying) the compiler explanation all kinda grasp at something like the first ("not theoretically possible") explanation, and fail to mention the second ("just an ergonomic constraint") explanation. So I figured I'd clean up the docs to clarify, maybe confuse the next person less (unless of course I'm misreading the history here and misunderstanding motives -- please let me know if so!)

While here I also did some cleanups:

  - Rewrote the preamble, trying to help the user get a little better oriented (I found the existing preamble a bit scattered).
  - Modernized notation (using `dyn Trait`)
  - Changed the section headings to all be written with the same logical sense: to all be written as "conditions that violate object safety" rather than a mix of that and the negated form "conditions that must not happen in order to ensure object safety".

I think there's a fair bit more to clean up in this doc -- the later sections get a bit rambly and I suspect there should be a completely separated-out section covering the `where Self:Sized` escape hatch for instructing the compiler to "do the old thing" and strip methods off traits when turning them into objects (it's a bit buried as a digression in the individual sub-error sections). But I did what I had time for now.
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 2, 2021
…askrgr

Rollup of 12 pull requests

Successful merges:

 - rust-lang#89954 (Fix legacy_const_generic doc arguments display)
 - rust-lang#91321 (Handle placeholder regions in NLL type outlive constraints)
 - rust-lang#91329 (Fix incorrect usage of `EvaluatedToOk` when evaluating `TypeOutlives`)
 - rust-lang#91364 (Improve error message for incorrect field accesses through raw pointers)
 - rust-lang#91387 (Clarify and tidy up explanation of E0038)
 - rust-lang#91410 (Move `#![feature(const_precise_live_drops)]` checks earlier in the pipeline)
 - rust-lang#91435 (Improve diagnostic for missing half of binary operator in `if` condition)
 - rust-lang#91444 (disable tests in Miri that take too long)
 - rust-lang#91457 (Add additional test from rust issue number 91068)
 - rust-lang#91460 (Document how `last_os_error` should be used)
 - rust-lang#91464 (Document file path case sensitivity)
 - rust-lang#91466 (Improve the comments in `Symbol::interner`.)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit b269213 into rust-lang:master Dec 3, 2021
@rustbot rustbot added this to the 1.59.0 milestone Dec 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants