Add error explanation for E0038 #26163

jashank · 2015-06-10T02:17:34Z

I'd like to have added examples for each case but the overall error explanation started getting uncomfortably long. Instead, I've lifted and rewritten notes from src/librustc/middle/traits/object_safety.rs to be (somewhat) clearer.

Personally, I feel this should become five separate error codes for each ObjectSafetyViolation case, also counting MethodViolationCodes.

cc #24407

rust-highfive · 2015-06-10T02:17:51Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

nham · 2015-06-10T08:48:39Z

src/librustc/diagnostics.rs

@@ -284,6 +284,30 @@ This error indicates that an attempt was made to divide by zero (or take the
 remainder of a zero divisor) in a static or constant expression.
 "##,

+E0038: r##"
+This trait is not object safe, but the coercion of an implementation to a trait


This is perhaps a nitpick, but technically there are two ways to get a trait object value: casting and coercion. Instead of saying "coercion", it might be better to be vague here, something like "It is not allowed to create a trait object value for a trait that is not object safe"

nham · 2015-06-10T08:58:24Z

Do you think it'd be useful to link to RFC 255 here? That RFC is a bit technical, but it does discuss the object safety rules in detail.

jashank · 2015-06-10T10:34:11Z

@nham, Thanks for that; I've added both suggestions. While yes, RFC 255 is a bit technical, it is (with my 'user' hat on) reasonably clear to understand, and there is a precedent for pointing to an RFC for more details.

alexcrichton · 2015-06-10T15:10:51Z

@bors: r+ 7578947982b656053ced7891068bd8a6a3a66058

Thanks!

bors · 2015-06-11T09:58:32Z

⌛ Testing commit 7578947 with merge 4133fde...

bors · 2015-06-11T10:06:37Z

💔 Test failed - auto-mac-64-opt

nikomatsakis · 2015-06-11T13:29:55Z

(Just made a few minor suggestions for improvement; the current text is also good. And the link to the RFC mitigates some of my concerns about details, I guess.)

jashank · 2015-06-12T00:37:19Z

@nikomatsakis: I think what I was trying to say there was how Self: Sized could become a bound of the trait, but wound up with a weird, and unclear wording. I agree that it's easier to grasp if the rationale is more apparent. How about

the trait does not have Self: Sized as a bound, to enforce the abstraction of an unknown underlying implementation with potentially arbitrary size

as an explanation (inspired by huonw's description of Sized)?

(Also, do I prefer Self: Sized or Self : Sized?)

With regards to the rationale for (e.g.) restricting generic type parameters to create the vtable, duplicates detail from The Book, or RFC 255, and IMHO is a bit beyond the scope of --explain.

jashank · 2015-06-12T00:37:49Z

Additionally, the build failure on auto-mac-64-opt---

rustc: x86_64-apple-darwin/stage0/lib/rustlib/x86_64-apple-darwin/lib/librustc
<syntax macros>:2:1: 2:51 error: description for error code E0038 contains a line longer than 80 characters
<syntax macros>:2 __register_diagnostic ! { $ code , $ description } ) ; ( $ code : tt ) => (
                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
error: aborting due to previous error
make: *** [x86_64-apple-darwin/stage0/lib/rustlib/x86_64-apple-darwin/lib/stamp.rustc] Error 101

---appears to be entirely because of the RFC URL. Any suggestions about how to make that be a non-issue?

(edit: see rust-lang/prev.rust-lang.org#147)

michaelsproul · 2015-06-14T01:51:00Z

@jashank: I'll tweak the lint so that it allows URLs to go over the 80 character line limit.

The plugin that enforces error explanation line-length is a bit restrictive, particularly when attempting to use URLs in explanations (cf #26163). Jashank will still have to do some mucking around with `#[cfg(not(stage0))]` attributes or else wait until a snapshot with this commit lands.

nikomatsakis · 2015-06-15T20:20:14Z

@jashank sounds good to me!

bors · 2015-06-29T11:18:29Z

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

jashank · 2015-06-29T13:18:03Z

... do I have any regrets after rebasing and updating this?

(Also, still blocked by the as-yet-unlanded-in-a-snapshot #26290.)

GuillaumeGomez · 2015-06-30T08:08:51Z

src/librustc/diagnostics.rs

@@ -1089,6 +1116,8 @@ register_diagnostics! {
    E0017,
    E0022,
    E0038,


I comment here again: this line should be removed.

jashank · 2015-07-03T07:37:47Z

Ooops, sorry about that, @GuillaumeGomez. Rebasing again.

As for the Markdown, all of -, *, and + should be valid. I cannot find any precedent for which to use in the explanations. Grepping the tree for doc comments suggests that - is most commonly used, but * is also used. (FWIW, I prefer -.)

GuillaumeGomez · 2015-07-03T07:40:12Z

I use *, but if doc says * is fine, then just ignore my comment. ;-)

GuillaumeGomez · 2015-07-03T07:43:04Z

cc @Manishearth

bors · 2015-07-04T16:18:16Z

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

jashank · 2015-07-05T00:45:37Z

Rebasing, and hopefully I haven't completely screwed up this time.

(I wonder if 26290 has landed yet, which would make this safe to merge.)

GuillaumeGomez · 2015-07-05T10:15:34Z

You just have to squash now I guess.

michaelsproul · 2015-07-05T11:44:26Z

There's no new snapshot but there is a workaround - you can register a single error with register_diagnostic!; combine with cfg conditional compilation and voila!

// Some error that the stage0 compiler doesn't like (E0038 for now)
#[cfg(stage0)]
register_diagnostic! { E0038 }
#[cfg(not(stage0))]
register_diagnostic! { E0038, r##"
Blah blah blah
"##
}

michaelsproul · 2015-07-05T11:46:52Z

In terms of bootstrapping, this is fine, as it only excludes the error from the stage1 compiler.

@nham

I'd like to have added examples for each case but the overall error explanation started getting uncomfortably long. Instead, I've lifted and rewritten notes from src/librustc/middle/traits/object_safety.rs to be (somewhat) clearer. Various suggestions for the wording came from @nham and @nikomatsakis. cc #24407

jashank · 2015-07-05T12:01:35Z

I'm happy to wait for a new snapshot to land, because this isn't exactly a big change, and this is one less thing for the next snapshot wrangler to fix. (Also, apparently, there's a new snapshot coming Real Soon Now.)

Squashing this down to one commit, then Waiting for Snapshot.

Manishearth · 2015-07-14T14:01:58Z

I wrote a more comprehensive version of this: #26963

Sorry for taking your PR over, let me know if there's something from here I should pick up :)

jashank · 2015-07-15T02:23:17Z

Nope, I don't see anything that hasn't been covered.

Closing in favour of #26963.

rust-highfive assigned nikomatsakis Jun 10, 2015

nham reviewed Jun 10, 2015
View reviewed changes

michaelsproul mentioned this pull request Jun 14, 2015

Allow long URLs in error explanations. #26290

Merged

GuillaumeGomez reviewed Jun 30, 2015
View reviewed changes

michaelsproul mentioned this pull request Jul 14, 2015

Bunch of diagnostics fixes and additions #26963

Merged

jashank closed this Jul 15, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add error explanation for E0038 #26163

Add error explanation for E0038 #26163

jashank commented Jun 10, 2015

rust-highfive commented Jun 10, 2015

nham Jun 10, 2015

nham commented Jun 10, 2015

jashank commented Jun 10, 2015

alexcrichton commented Jun 10, 2015

bors commented Jun 11, 2015

bors commented Jun 11, 2015

nikomatsakis commented Jun 11, 2015

jashank commented Jun 12, 2015

jashank commented Jun 12, 2015

michaelsproul commented Jun 14, 2015

nikomatsakis commented Jun 15, 2015

bors commented Jun 29, 2015

jashank commented Jun 29, 2015

GuillaumeGomez Jun 30, 2015

jashank commented Jul 3, 2015

GuillaumeGomez commented Jul 3, 2015

GuillaumeGomez commented Jul 3, 2015

bors commented Jul 4, 2015

jashank commented Jul 5, 2015

GuillaumeGomez commented Jul 5, 2015

michaelsproul commented Jul 5, 2015

michaelsproul commented Jul 5, 2015

jashank commented Jul 5, 2015

Manishearth commented Jul 14, 2015

jashank commented Jul 15, 2015

@@ @@ -1089,6 +1116,8 @@ register_diagnostics! { @@
                   E0017,
                   E0022,
                   E0038,

Add error explanation for E0038 #26163

Add error explanation for E0038 #26163

Conversation

jashank commented Jun 10, 2015

rust-highfive commented Jun 10, 2015

nham Jun 10, 2015

Choose a reason for hiding this comment

nham commented Jun 10, 2015

jashank commented Jun 10, 2015

alexcrichton commented Jun 10, 2015

bors commented Jun 11, 2015

bors commented Jun 11, 2015

nikomatsakis commented Jun 11, 2015

jashank commented Jun 12, 2015

jashank commented Jun 12, 2015

michaelsproul commented Jun 14, 2015

nikomatsakis commented Jun 15, 2015

bors commented Jun 29, 2015

jashank commented Jun 29, 2015

GuillaumeGomez Jun 30, 2015

Choose a reason for hiding this comment

jashank commented Jul 3, 2015

GuillaumeGomez commented Jul 3, 2015

GuillaumeGomez commented Jul 3, 2015

bors commented Jul 4, 2015

jashank commented Jul 5, 2015

GuillaumeGomez commented Jul 5, 2015

michaelsproul commented Jul 5, 2015

michaelsproul commented Jul 5, 2015

jashank commented Jul 5, 2015

Manishearth commented Jul 14, 2015

jashank commented Jul 15, 2015