Print the two types in the span label for transmute errors. by Mark-Simulacrum · Pull Request #42304 · rust-lang/rust

Mark-Simulacrum · 2017-05-29T23:40:30Z

Fixes#37157. I'm not entirely happy with the changes here but overall it's better in my opinion; we certainly avoid the odd language in that issue, which changes to:

error[E0512]: transmute called with differently sized types: <C as TypeConstructor<'a>>::T (size can vary because of <C as TypeConstructor>::T) to <C as TypeConstructor<'b>>::T (size can vary because of <C as TypeConstructor>::T)
--> test.rs:8:5
|
8 | ::std::mem::transmute(x)
| ^^^^^^^^^^^^^^^^^^^^^ transmuting between <C as TypeConstructor<'a>>::T and <C as TypeConstructor<'b>>::T
error: aborting due to previous error(s)

rust-highfive · 2017-05-29T23:40:41Z

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

nikomatsakis · 2017-05-30T16:13:01Z

at minimum, this should be {} and {} -- but it definitely feels like we can do better. If nothing else, these error messages are really long, and I think it'd be better to break it up in parts. Let me see if I can draft a suggestion.

nikomatsakis · 2017-05-30T16:15:03Z

Maybe something like this?

error[E0512]: transmute called with two types that may have different sizes
--> test.rs:8:5
|
8 | ::std::mem::transmute(x)
| ^^^^^^^^^^^^^^^^^^^^^ source and target type of transmute may have different sizes
|
| note: source type is `<C as TypeConstructor<'a>>::T` (size can vary because of `<C as TypeConstructor>::T`)
| note: target type is `<C as TypeConstructor<'b>>::T` (size can vary because of `<C as TypeConstructor>::T`)

nikomatsakis · 2017-05-30T16:15:17Z

cc @estebank

estebank · 2017-05-30T17:09:43Z

For the general case, I feel something along the lines of this would be ok:

error[E0512]: transmute called with two types that may have different sizes
--> test.rs:8:5
|
8 | ::std::mem::transmute(x)
| ^^^^^^^^^^^^^^^^^^^^^ source and target type of transmute may have different sizes
|
| note: source type is `<C as TypeConstructor<'a>>::T`
it's size can vary because of `<C as TypeConstructor>::T`
| note: target type is `<C as TypeConstructor<'b>>::T`
it's size can vary because of `<C as TypeConstructor>::T`

but I'd like to have a specific message for this case along the lines of

error[E0512]: transmute called with two types that may have different sizes
--> test.rs:8:5
|
8 | ::std::mem::transmute(x)
| ^^^^^^^^^^^^^^^^^^^^^ source and target type of transmute may have different sizes
|
| note: source type is `<C as TypeConstructor<'a>>::T` and target type is `<C as TypeConstructor<'b>>::T`
their size can vary because of `<C as TypeConstructor>::T`

For example, in

fntakes_u8(_:u8){}fnmain(){unsafe{takes_u8(::std::mem::transmute(0u16));}}

I feel the current output is relatively ok:

error[E0512]: transmute called with differently sized types: u16 (16 bits) to u8 (8 bits)
--> <anon>:4:25
|
4 | unsafe { takes_u8(::std::mem::transmute(0u16)); }
| ^^^^^^^^^^^^^^^^^^^^^ transmuting between 16 bits and 8 bits

and would look ridiculous under the above proposal:

error[E0512]: transmute called with two types that may have different sizes
--> <anon>:4:25
|
4 | unsafe { takes_u8(::std::mem::transmute(0u16)); }
| ^^^^^^^^^^^^^^^^^^^^^ source and target type of transmute may have different sizes
| note: source type is `u16`
16 bits
| note: target type is `u8`
8 bits

Because of all of this, I would check for the case where there's an unknown layout in the source and target. If they are, present my second proposed output, otherwise, @nikomatsakis proposed output. If both source and target are not of unknown layout, either have a single-line note or (my preference) keep the current label.

Part of the problem here is that this is two different errors conceptually, the compiler knows in the last example that the types have different sizes, while the other case they might be of different sizes, so they warrant slightly different output.

Mark-Simulacrum · 2017-05-30T17:12:25Z

Hm, okay. I can't say I entirely follow the discussion quite right now, but I'll look into implementing things in this way soon.

nikomatsakis · 2017-05-30T21:11:43Z

@Mark-Simulacrum great let me know if you need anything

estebank · 2017-05-31T00:05:03Z

@Mark-Simulacrum It'd also be great if you also moved these tests to ui.

nikomatsakis · 2017-05-31T15:41:24Z

would look ridiculous under the above proposal

fwiw, I don't think that final version looks particularly ridiculous. In fact, I maybe slightly prefer it. I think in particular I'd like to migrate towards a version where the "main error message" is fairly generic, and the details are exposed elsewhere. I think a lot of times we try to pack too much info into the header. But we really need to try and schedule some time to draw up some overall guidelines in this respect.

But I'm also happy with a special case. Don't care that much.

Mark-Simulacrum · 2017-05-31T16:56:24Z

The span here was entirely not helpful as far as I can tell; but I'm open to re-adding it if there's disagreement.

Mark-Simulacrum · 2017-05-31T16:56:59Z

I'm not sure if there's much point to this branch myself; I can't come up with an example that would follow it.

Mark-Simulacrum · 2017-05-31T16:57:56Z

Should we change this error to also follow the same pattern of a simple header and notes to explain it?

I think that would be best. The error text should be something like can't transmute zero-sized type.

Mark-Simulacrum · 2017-05-31T16:58:27Z

Updated; left a few comments, let me know what you think.

estebank

I'm ok with the output.

estebank · 2017-05-31T17:04:06Z

I think that would be best. The error text should be something like can't transmute zero-sized type.

Mark-Simulacrum · 2017-05-31T17:31:55Z

Updated the zero-sized case as well.

nikomatsakis · 2017-06-01T19:34:58Z

Nit: "called with types of different sizes" sounds more grammatical to me

nikomatsakis · 2017-06-01T19:35:56Z

This looks good to me modulo the nit about the error message phrasing.

Mark-Simulacrum · 2017-06-01T19:56:49Z

@bors r=nikomatsakis

Fixed the nit.

bors · 2017-06-01T19:56:50Z

📌 Commit e7b8935 has been approved by nikomatsakis

Mark-Simulacrum · 2017-06-01T23:32:49Z

@bors rollup

Mark-Simulacrum · 2017-06-01T23:33:54Z

@bors r-

I need to fix the UI tests to not be dependent on 32/64-bit (they use usize/isize).

arielb1 · 2017-06-13T20:49:28Z

@Mark-Simulacrum

I think we might want to have tests only run on 32-bit or 64-bit architectures, and have separate paths for that.

nikomatsakis · 2017-06-14T21:11:42Z

certainly the short-term hack is just to have two tests, one for 32-bit and one for 64-bit? But I thought that the idea of being able to add custom "filters" into the ui tests was a nice long-term solution. Not sure where that issue ran off to.

Mark-Simulacrum · 2017-06-14T21:17:29Z

Yeah, I hope to get around to adding the two tests later this week, I just need to figure out the specific syntax and what architectures I need to ignore.

Also moves a few transmute tests to UI tests to better test their output.

Mark-Simulacrum · 2017-06-18T17:42:34Z

I just ignored the tests on 32-bit targets. I personally don't know if it's worth testing on those, the tests are just for diagnostics. If we want 32 bit tests I'll spin up a VM to generate those tests.

alexcrichton · 2017-06-22T14:45:09Z

ping r? @nikomatsakis

Or @rust-lang/compiler I think Niko's out of town, so if someone else can take a look that'd be great!

nikomatsakis · 2017-06-22T14:47:34Z

@bors r+

bors · 2017-06-22T14:47:35Z

📌 Commit d09cf46 has been approved by nikomatsakis

nikomatsakis · 2017-06-22T14:48:10Z

@bors r-

nikomatsakis · 2017-06-22T14:48:53Z

Actually, I have some mild reservations. I just want to be sure: are those tests checking anything important? e.g., do we have other tests that show that the size of points on 32-bit machines is correctly handled, etc?

nikomatsakis · 2017-06-22T14:50:07Z

@Mark-Simulacrum

I personally don't know if it's worth testing on those, the tests are just for diagnostics.

I guess my question is -- are there other tests that test similar conditions and do run on 32-bit?

Mark-Simulacrum · 2017-06-22T14:54:55Z

Yes, there are tests (the compile-fail tests that this edits, but doesn't move to UI, for one) that test all cases as far as I can tell. If you'd like, we can wait on this (probably closing it for now) until we get some form of ERROR-x86 or something like that so we can properly test across both platforms easily.

nikomatsakis · 2017-06-22T15:01:32Z

@Mark-Simulacrum ok, cool :)

nikomatsakis · 2017-06-22T15:01:37Z

@bors r+

bors · 2017-06-22T15:01:38Z

📌 Commit d09cf46 has been approved by nikomatsakis

Mark-Simulacrum · 2017-06-22T21:53:13Z

@bors rollup-

Since it failed CI last time.

bors · 2017-06-23T00:32:46Z

⌛ Testing commit d09cf46 with merge 7379620...

Print the two types in the span label for transmute errors. Fixes#37157. I'm not entirely happy with the changes here but overall it's better in my opinion; we certainly avoid the odd language in that issue, which changes to: ``` error[E0512]: transmute called with differently sized types: <C as TypeConstructor<'a>>::T (size can vary because of <C as TypeConstructor>::T) to <C as TypeConstructor<'b>>::T (size can vary because of <C as TypeConstructor>::T) --> test.rs:8:5 | 8 | ::std::mem::transmute(x) | ^^^^^^^^^^^^^^^^^^^^^ transmuting between <C as TypeConstructor<'a>>::T and <C as TypeConstructor<'b>>::T error: aborting due to previous error(s) ```

bors · 2017-06-23T03:31:08Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 7379620 to master...

@nikomatsakis

… r=nikomatsakis compilertest (UI test): Support custom normalization. Closes#42434. Adds this header for UI tests: ```rust // normalize-stderr-32bit: "fn() (32 bits)" -> "fn() ($PTR bits)" ``` It will normalize the `stderr` output on 32-bit platforms, by replacing all instances of `fn() (32 bits)` by `fn() ($PTR bits)`. Extends the UI tests in #42304 and #41968 to 32-bit targets. r? @nikomatsakis

@nikomatsakis

… r=nikomatsakis compilertest (UI test): Support custom normalization. Closes#42434. Adds this header for UI tests: ```rust // normalize-stderr-32bit: "fn() (32 bits)" -> "fn() ($PTR bits)" ``` It will normalize the `stderr` output on 32-bit platforms, by replacing all instances of `fn() (32 bits)` by `fn() ($PTR bits)`. Extends the UI tests in #42304 and #41968 to 32-bit targets. r? @nikomatsakis

rust-highfive assigned nikomatsakis May 29, 2017

Mark-Simulacrum added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 29, 2017

nikomatsakis reviewed May 30, 2017
View reviewed changes

Mark-Simulacrum 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-review Status: Awaiting review from the assignee but also interested parties. labels May 30, 2017

Mark-Simulacrum force-pushed the issue-37157 branch from 9ce7f5a to 22f9b0eCompare May 31, 2017 16:55

Mark-Simulacrum commented May 31, 2017
View reviewed changes

estebank reviewed May 31, 2017
View reviewed changes

Mark-Simulacrum force-pushed the issue-37157 branch from 22f9b0e to 708bc6cCompare May 31, 2017 17:31

Mark-Simulacrum added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 31, 2017

nikomatsakis reviewed Jun 1, 2017
View reviewed changes

Mark-Simulacrum force-pushed the issue-37157 branch from 708bc6c to e7b8935Compare June 1, 2017 19:56

Mark-Simulacrum 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-review Status: Awaiting review from the assignee but also interested parties. labels Jun 1, 2017

Mark-Simulacrum added 2 commits June 18, 2017 10:36

Update transmute size lints.
effa869
Also moves a few transmute tests to UI tests to better test their output.

Update UI tests to be platform independent
d09cf46

Mark-Simulacrum force-pushed the issue-37157 branch from 7898f41 to d09cf46Compare June 18, 2017 17:35

Mark-Simulacrum added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 18, 2017

bors merged commit d09cf46 into rust-lang:masterJun 23, 2017

kennytm mentioned this pull request Jul 6, 2017
compilertest (UI test): Support custom normalization. #43083
Merged

Mark-Simulacrum deleted the issue-37157 branch June 8, 2019 13:48

Uh oh!

Uh oh!

Conversation

Mark-Simulacrum commented May 29, 2017

Uh oh!

rust-highfive commented May 29, 2017

Uh oh!

nikomatsakisMay 30, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikomatsakis commented May 30, 2017

Uh oh!

nikomatsakis commented May 30, 2017

Uh oh!

estebank commented May 30, 2017

Uh oh!

Mark-Simulacrum commented May 30, 2017

Uh oh!

nikomatsakis commented May 30, 2017

Uh oh!

estebank commented May 31, 2017

Uh oh!

nikomatsakis commented May 31, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mark-SimulacrumMay 31, 2017

Choose a reason for hiding this comment

Uh oh!

Mark-SimulacrumMay 31, 2017

Choose a reason for hiding this comment

Uh oh!

Mark-SimulacrumMay 31, 2017

Choose a reason for hiding this comment

Uh oh!

estebankMay 31, 2017

Choose a reason for hiding this comment

Uh oh!

Mark-Simulacrum commented May 31, 2017

Uh oh!

estebank left a comment

Choose a reason for hiding this comment

Uh oh!

estebankMay 31, 2017

Choose a reason for hiding this comment

Uh oh!

Mark-Simulacrum commented May 31, 2017

Uh oh!

nikomatsakisJun 1, 2017

Choose a reason for hiding this comment

Uh oh!

nikomatsakis commented Jun 1, 2017

Uh oh!

Mark-Simulacrum commented Jun 1, 2017

Uh oh!

bors commented Jun 1, 2017

Uh oh!

Mark-Simulacrum commented Jun 1, 2017

Uh oh!

Mark-Simulacrum commented Jun 1, 2017

Uh oh!

arielb1 commented Jun 13, 2017

Uh oh!

nikomatsakis commented Jun 14, 2017

Uh oh!

Mark-Simulacrum commented Jun 14, 2017

Uh oh!

Mark-Simulacrum commented Jun 18, 2017

Uh oh!

alexcrichton commented Jun 22, 2017

Uh oh!

nikomatsakis commented Jun 22, 2017

Uh oh!

bors commented Jun 22, 2017

Uh oh!

nikomatsakis commented Jun 22, 2017

Uh oh!

nikomatsakis commented Jun 22, 2017

Uh oh!

nikomatsakis commented Jun 22, 2017

nikomatsakisMay 30, 2017•
edited
Loading

nikomatsakis commented May 31, 2017•
edited
Loading