Stream: t-compiler

Topic: rfc 2203 const array exprs


davidtwco (Jun 02 2019 at 10:02, on Zulip):

hey @eddyb, I've implemented the steps you wrote up here and I've been trying to debug a few of the ICEs that have sprung up. Unfortunately, I've not been able to carve out much time for long debugging sessions as I'd have liked recently to tackle these failures. I was hoping you might have some thoughts on what the correct fix is (for the issues where I have an idea what's going wrong) and where I might go looking to fix the other problems.

There are four distinct failures that I'm seeing:

ui/huge-array.rs

error: internal compiler error: src/librustc_mir/interpret/eval_context.rs:139: The type checker should prevent reading from a never-written local

Here's the function it fails in:

fn generic<T: Copy>(t: T) {
    let s: [T; 1518600000] = [t; 1518600000];
}

Thus far, I've spent most of my time digging into this error. It appears that the qualify_consts pass determines that t should be a candidate for promotion. Then when constructing the promoted[0] MIR, it eventually hits this line where the LocalKind is an argument, so it stops. Then we end up with promoted MIR that looks like this:

    bb0: {
         _1 = _1;                         // bb0[0]: scope 0 at src/test/ui/huge-array.rs:8:31: 8:32
         _0 = move _1;                    // bb0[1]: scope 0 at src/test/ui/huge-array.rs:8:30: 8:45
         return;                          // bb0[2]: scope 0 at src/test/ui/huge-array.rs:8:30: 8:45
     }

And that causes the ICE because _1 is never written to. I've been working on the assumption that we're going wrong by considering the t a candidate, and after digging into the IsNotImplicitlyPromotable::in_operand check that we perform, I thought that the fix might be to consider the argument locals in the original MIR not implicitly promotable (and thus the t wouldn't be implicitly promotable either). I wasn't sure if this would have unintended consequences as I'm not that familiar with the const qualification/promotion code, so wanted to ask first.


ui/consts/rfc-2203-const-array-repeat-exprs/nll_borrowck.rs

error: internal compiler error: broken MIR in DefId(0:78 ~ nll_borrowck[317d]::non_constants[0]::no_impl_copy_empty_value_no_elements[0]) (NoSolution): could not prove Binder(TraitPredicate(<std::option::Option<Bar> as
 std::marker::Copy>))
  --> /home/david/projects/rust/rust0/src/test/ui/consts/rfc-2203-const-array-repeat-exprs/nll_borrowck.rs:77:37
   |
LL |         let arr: [Option<Bar>; 0] = [x; 0];
   |                                     ^^^^^^

(this test is one that I've added enumerating a bunch of the cases with repeat expressions)

My understanding of this failure is that by disabling the check from step 1 of your instructions (in your instructions you say tcx.borrowck_mode() == BorrowckMode::Mir but I think you meant tcx.borrowck_mode() != BorrowckMode::Mir). Where the rustc_typeck logic would previously have did the check for a type to be Copy, now it's getting to the Mir type check to do that, which I assume had a pre-existing bug for this case that just wasn't surfacing. I've not had time to really dig into this one.


ui/issues/issue-17913.rs

-       error: the type `[&usize; N]` is too big for the current architecture
+       error[E0080]: it is undefined behavior to use this value
+         --> $DIR/issue-17913.rs:11:1
+          |
+       LL | / fn main() {
+       LL | |     let n = 0_usize;
+       LL | |     let a: Box<_> = box [&n; 0xF000000000000000_usize];
+       LL | |     println!("{}", a[0xFFFFFF_usize]);
+       LL | | }
+          | |_^ type validation failed: encountered a pointer at .<deref>, but expected initialized plain (non-pointer) bytes
+          |
+          = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undef
ined behavior
2
3       error: aborting due to previous error
4

+       For more information about this error, try `rustc --explain E0080`.
5

I've not had time to dig into this error at all.


ui/nll/type-check-pointer-coercions.rs

50      LL |     x
51         |     ^ returning this value requires that `'a` must outlive `'b`
52
-       error: lifetime may not live long enough
-         --> $DIR/type-check-pointer-coercions.rs:24:5
-          |
-       LL | fn array_elem<'a, 'b>(x: &'a i32) -> *const &'b i32 {
-          |               --  -- lifetime `'b` defined here
-          |               |
-          |               lifetime `'a` defined here
-       ...
-       LL |     y
-          |     ^ returning this value requires that `'a` must outlive `'b`
-
-       error: lifetime may not live long enough
-         --> $DIR/type-check-pointer-coercions.rs:30:5
-          |
-       LL | fn array_coerce<'a, 'b>(x: &'a i32) -> *const [&'b i32; 3] {
-          |                 --  -- lifetime `'b` defined here
-          |                 |
-          |                 lifetime `'a` defined here
-       ...
-       LL |     y
-          |     ^ returning this value requires that `'a` must outlive `'b`
-
-       error: lifetime may not live long enough
-         --> $DIR/type-check-pointer-coercions.rs:36:5
-          |
-       LL | fn nested_array<'a, 'b>(x: &'a i32) -> *const [&'b i32; 2] {
-          |                 --  -- lifetime `'b` defined here
-          |                 |
-          |                 lifetime `'a` defined here
-       ...
-       LL |     y
-          |     ^ returning this value requires that `'a` must outlive `'b`
-
-       error: aborting due to 8 previous errors
+       error: aborting due to 5 previous errors
87
88

I've also not had time to dig into this.

davidtwco (Jun 02 2019 at 10:02, on Zulip):

Sorry for the gigantic braindump.

davidtwco (Jun 02 2019 at 10:23, on Zulip):

So, I can confirm that my theorized fix for the first issue fixes the problems with ui/nll/type-check-pointer-coercions.rs and ui/huge-array.rs.

eddyb (Jun 03 2019 at 10:17, on Zulip):

yeah _1 = _1 is just broken

eddyb (Jun 03 2019 at 10:18, on Zulip):

the argument locals should already be marked as not implicitly promotable, I thought we even had bugs fixed that were like this (cc @oli)

eddyb (Jun 03 2019 at 10:19, on Zulip):

@davidtwco hang on, we do have this: https://github.com/rust-lang/rust/blob/master/src/librustc_mir/transform/qualify_consts.rs#L650-L652

davidtwco (Jun 03 2019 at 10:20, on Zulip):

Should I add IsNotImplicitlyPromotable in that conditional?

eddyb (Jun 03 2019 at 10:22, on Zulip):

no

eddyb (Jun 03 2019 at 10:22, on Zulip):

@davidtwco I misspoke in the suggestions

davidtwco (Jun 03 2019 at 10:22, on Zulip):

It should be checking for IsNotPromotable? Rather than IsNotImplicitlyPromotable?

eddyb (Jun 03 2019 at 10:22, on Zulip):

this is what it looks like for implicit promotion https://github.com/rust-lang/rust/blob/master/src/librustc_mir/transform/qualify_consts.rs#L758-L760

eddyb (Jun 03 2019 at 10:22, on Zulip):

you should be checking both

davidtwco (Jun 03 2019 at 10:26, on Zulip):

Alright, that's building now.

eddyb (Jun 03 2019 at 10:32, on Zulip):

and you were right about the ==

eddyb (Jun 03 2019 at 10:32, on Zulip):

@davidtwco I've edited https://github.com/rust-lang/rust/issues/49147#issuecomment-494745914

davidtwco (Jun 03 2019 at 10:32, on Zulip):

That fixed one of the other ICEs too.

eddyb (Jun 03 2019 at 10:33, on Zulip):

note that where I suggested to put Rvalue::Repeat handling in qualify_consts was wrong

eddyb (Jun 03 2019 at 10:33, on Zulip):

since Rvalue::Ref is handled in assign, so should this

davidtwco (Jun 03 2019 at 10:34, on Zulip):

Cool, I'll make that change.

eddyb (Jun 03 2019 at 10:34, on Zulip):

so glad you're working on this :D

davidtwco (Jun 03 2019 at 10:35, on Zulip):

This is the only failure that remains: https://gist.github.com/davidtwco/7c891eaf0030bd2cde80af37511d09ac

eddyb (Jun 03 2019 at 10:35, on Zulip):

so that is weird

davidtwco (Jun 03 2019 at 10:35, on Zulip):

That being a test I added with every enumeration of copy/non-copy/const/non-const/etc.

eddyb (Jun 03 2019 at 10:37, on Zulip):

https://github.com/rust-lang/rust/blob/master/src/librustc_mir/borrow_check/nll/type_check/mod.rs#L519-L523

eddyb (Jun 03 2019 at 10:37, on Zulip):

this should already kick in if a Copy impl has lifetime bounds that can't be satisfied

eddyb (Jun 03 2019 at 10:37, on Zulip):

in fact I can prove it rly quickly

eddyb (Jun 03 2019 at 10:44, on Zulip):

@davidtwco https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=a4d57cddcb7e118e1c241f6f7d328119

eddyb (Jun 03 2019 at 10:45, on Zulip):

note that if you switch the edition to 2015 you'll get no error

eddyb (Jun 03 2019 at 10:46, on Zulip):

and this is where it is emitted from https://github.com/rust-lang/rust/blob/master/src/librustc_mir/borrow_check/nll/type_check/mod.rs#L501

eddyb (Jun 03 2019 at 10:47, on Zulip):

@davidtwco hang on, 2015 and 2018 on nightly are the same, so my instructions are wrong!

eddyb (Jun 03 2019 at 10:52, on Zulip):

or not wrong, since you would still need #![feature(nll)] which would make those hard errors...

eddyb (Jun 03 2019 at 10:52, on Zulip):

anyway I added a paragraph to step 1. in https://github.com/rust-lang/rust/issues/49147#issuecomment-494745914

davidtwco (Jun 03 2019 at 10:55, on Zulip):

To clarify, you expect this not to ICE with full NLL?

eddyb (Jun 03 2019 at 11:03, on Zulip):

yeah, it should error properly

davidtwco (Jun 03 2019 at 11:05, on Zulip):

That test has #![feature(nll)].

davidtwco (Jun 03 2019 at 11:06, on Zulip):

It fails with NLL.

davidtwco (Jun 03 2019 at 11:06, on Zulip):

Works fine with my AST check (as in, it works the way I’d expect, and doesn’t ICE).

eddyb (Jun 03 2019 at 11:09, on Zulip):

hmmm

eddyb (Jun 03 2019 at 11:10, on Zulip):

oooh https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=3b14136c6338d0fbad6ddf6817cdf00e

eddyb (Jun 03 2019 at 11:10, on Zulip):

hang on...

eddyb (Jun 03 2019 at 11:11, on Zulip):

@davidtwco right, I'm an idiot, what the MIR borrowck does is not what one might think it does

eddyb (Jun 03 2019 at 11:17, on Zulip):

@davidtwco added paragraph to 2. https://github.com/rust-lang/rust/issues/49147#issuecomment-494745914

eddyb (Jun 03 2019 at 11:18, on Zulip):

so the solution is to do the typeck check, wait... I misread it. ugh, time to edit again

eddyb (Jun 03 2019 at 11:25, on Zulip):

@davidtwco yeah so you need something like this https://github.com/rust-lang/rust/blob/c57ed9d9478dcd12c854a0ef4e83c7f384ade060/src/librustc_mir/borrow_check/nll/type_check/mod.rs#L1876-L1890

eddyb (Jun 03 2019 at 11:26, on Zulip):

I posted the correct check.. wait, I screwed up again aaaaa

eddyb (Jun 03 2019 at 11:35, on Zulip):

@davidtwco sorry for the back&forth, you can now re-review steps 1. and 2. https://github.com/rust-lang/rust/issues/49147#issuecomment-494745914

eddyb (Jun 03 2019 at 11:36, on Zulip):

I don't know how to emit a proper trait error from NLL typeck, maybe @Matthew Jasper does?

eddyb (Jun 03 2019 at 11:36, on Zulip):

but we can discuss that on the PR, a simple hard error would suffice to submit the first version

davidtwco (Jun 09 2019 at 20:19, on Zulip):

@eddyb Didn't have much time to work on this in the last week, got back to it today. I've found that changing the code to a hard error like you suggested didn't quite work. It appeared to cause an error in cases that were previously accepted, I worked around it by also checking type_is_copy_modulo_regions before emitting an error, but I suspect that isn't correct.

After that, the only remaining failure I've been running into is another could not prove Binder(TraitPredicate(<State as std::marker::Copy>)) error from broken MIR, this time in run-pass/issues/issue-23898.rs.

davidtwco (Jun 09 2019 at 20:20, on Zulip):
error: internal compiler error: broken MIR in DefId(0:17 ~ issue_23898[317d]::main[0]) (NoSolution): could not prove Binder(TraitPredicate(<State as std::marker::Copy>))
  --> /home/david/projects/rust/rust0/src/test/run-pass/issues/issue-23898.rs:9:5
   |
LL |     [State::ST_NULL; (State::ST_WHITESPACE as usize)];
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
davidtwco (Jun 09 2019 at 20:21, on Zulip):

I believe it is from these lines.

eddyb (Jun 10 2019 at 05:55, on Zulip):

interesting, I would've expected Operand::Copy to be generated when the MIR is built but I guess that's not a given

eddyb (Jun 10 2019 at 05:58, on Zulip):

those lines apply when you have Operand::Copy, which should've already passed type_is_copy_modulo_regions

davidtwco (Jun 10 2019 at 06:53, on Zulip):

those lines apply when you have Operand::Copy, which should've already passed type_is_copy_modulo_regions

Do you mean the lines I replaced w/ a hard error or the lines I think the current error is coming from?

davidtwco (Jun 11 2019 at 07:38, on Zulip):

@eddyb (in case you missed the above question)

eddyb (Jun 11 2019 at 07:38, on Zulip):

the latter

eddyb (Jun 11 2019 at 07:39, on Zulip):

you shouldn't be seeing Operand::Copy if the type doesn't at least appear to implement Copy

eddyb (Jun 11 2019 at 07:39, on Zulip):

if you do, it would be interesting to see where it might be coming from

davidtwco (Jun 11 2019 at 07:45, on Zulip):
error: repeated expression does not implement `std::marker::Copy`
  --> /home/david/projects/rust/rust0/src/test/ui/static/static-vec-repeat-not-constant.rs:3:24
   |
LL | static a: [isize; 2] = [foo(); 2];
   |                        ^^^^^^^^^^ the trait `std::marker::Copy` is not implemented for `isize`
   |
   = note: the `std::marker::Copy` trait is required because the repeated element will be copied
+       error: repeated expression does not implement `std::marker::Copy`
+         --> $DIR/type-check-pointer-coercions.rs:34:14
+          |
+       LL |     let z = &[[x; 2]; 3];
+          |              ^^^^^^^^^^^ the trait `std::marker::Copy` is not implemented for `[&i32; 2]`
+          |
+          = note: the `std::marker::Copy` trait is required because the repeated element will be copied
+       error: repeated expression does not implement `std::marker::Copy`
+         --> $DIR/issue-17913.rs:13:25
+          |
+       LL |     let a: Box<_> = box [&n; 0xF000000000000000_usize];
+          |                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `std::marker::Copy` is not implemented for `&usize`
+          |
+          = note: the `std::marker::Copy` trait is required because the repeated element will be copied

@eddyb some examples of the errors I get when I stop checking for type_is_copy_modulo_regions where I'm emitting the hard error.

eddyb (Jun 11 2019 at 07:46, on Zulip):

that makes sense, Operand::Move on Copy types is totally plausible

davidtwco (Jun 11 2019 at 07:46, on Zulip):

There are some instances where it's a generic type T and not a usize, i32, etc.

eddyb (Jun 11 2019 at 07:46, on Zulip):

like, it's presumably some temporary into which a value was constructed

eddyb (Jun 11 2019 at 07:47, on Zulip):

it would make sense for that to be moved

eddyb (Jun 11 2019 at 07:47, on Zulip):

so my initial suggestion of checking for Operand::Move is silly, your check is correct

eddyb (Jun 11 2019 at 07:47, on Zulip):

I just don't know why you'd get any ICEs

davidtwco (Jun 11 2019 at 07:48, on Zulip):

Well, we replaced the prove_trait_ref with the hard error in this case (conditional on type_is_copy_modulo_regions and Operand::Move, but it's the only other remaining prove_trait_reffor ConstraintCategory::CopyBound that is being hit now.

davidtwco (Jun 11 2019 at 07:59, on Zulip):

If I wrap the other check in a type_is_copy_modulo_regions (so we only run prove_trait_ref for types that are Copy (modulo regions)) then the test that was failing now passes. If you reckon that's the correct thing to do, I can tidy up my commits and throw a PR up shortly.

eddyb (Jun 11 2019 at 08:01, on Zulip):

@davidtwco it just means something is generating an Operand::Copy and shouldn't

davidtwco (Jun 11 2019 at 08:01, on Zulip):

So I should be working out how to stop that happening instead of adding a condition?

eddyb (Jun 11 2019 at 08:02, on Zulip):

yes

eddyb (Jun 11 2019 at 08:03, on Zulip):

@davidtwco if you search for Operand::Copy in librustc_mir/build you'll find that at least one of the places checks literally is_copy_modulo_regions before picking that

davidtwco (Jun 11 2019 at 16:10, on Zulip):

@eddyb Operand::Copy is being created by another change I've made, :face_palm:. In particular, when doing the actual promotion in the promote_consts pass, I am creating a Operand::Copy(promoted_place(ty, span)).

If I change it to Operand::Move then I get an error when compiling libstd:

error[E0507]: cannot move out of static item `promoted`
  --> src/libstd/sys/unix/os.rs:99:19
   |
99 |     let mut buf = [0 as c_char; TMPBUF_SZ];
   |                   ^^^^^^^^^^^^^^^^^^^^^^^^ move occurs because `promoted` has type `i8`, which does not implement the `Copy` trait

I'm unsure if the correct fix is to modify where we're calling prove_trait_ref so that it doesn't consider promoted places, or if I should be using Operand::Move when promoting and that I need to make a change elsewhere to avoid the error we're seeing in libstd.

eddyb (Jun 11 2019 at 17:33, on Zulip):

ohnoes

eddyb (Jun 11 2019 at 17:34, on Zulip):

@oli ^^ this is an instance where promoteds being places doesn't help :(

eddyb (Jun 11 2019 at 17:34, on Zulip):

@davidtwco so, what I would've expected is to not do any promotion to start off with

eddyb (Jun 11 2019 at 17:35, on Zulip):

and make sure stuff isn't broken

eddyb (Jun 11 2019 at 17:35, on Zulip):

the fact that you couldn't use Operand::Const, like I thought you could, is a big warning sign

eddyb (Jun 11 2019 at 17:36, on Zulip):

@oli maybe we need to go back to non-place-based promotion and, uhh, promote &base_local and then do &(*promoted)[runtime_index] if we have something like that to deal with :/

eddyb (Jun 11 2019 at 17:38, on Zulip):

@davidtwco for now, one thing you could do is ignore promoted places in that "must implement Copy" part of NLL borrowck type_check

davidtwco (Jun 11 2019 at 17:38, on Zulip):

I can do that.

eddyb (Jun 11 2019 at 17:39, on Zulip):

and open an issue about resolving the discordance between promotion for &expr and [expr; n]

davidtwco (Jun 11 2019 at 21:01, on Zulip):

@eddyb I've submitted #61749 with what I have, resolving the merge conflicts shortly. Haven't filed that issue yet.

oli (Jun 12 2019 at 06:45, on Zulip):

@eddyb I'm not sure how that would help? you'd just end up with a move out of borrowed context error

oli (Jun 12 2019 at 06:46, on Zulip):

instead what we can do is to allow moving out of promoteds similarly to how we allow "moving out of constants"

oli (Jun 12 2019 at 06:47, on Zulip):

well, not quite, since constants are their own Operand

eddyb (Jun 12 2019 at 06:48, on Zulip):

yes, "moving out of" only makes sense for places, and I'm saying promoteds not being places would make that go away

oli (Jun 12 2019 at 06:48, on Zulip):

oh

oli (Jun 12 2019 at 06:48, on Zulip):

I mean adding a new Operand variant is trivial

eddyb (Jun 12 2019 at 06:48, on Zulip):

like, you'd have a promoted of type &T for &expr and a promoted of type T for [expr; n]

oli (Jun 12 2019 at 06:48, on Zulip):

oh

eddyb (Jun 12 2019 at 06:49, on Zulip):

I'm pretty much talking of going back to how things were before you changed them

eddyb (Jun 12 2019 at 06:49, on Zulip):

a new Operand variant seems interesting, in that it wouldn't add onto Constant

oli (Jun 12 2019 at 06:49, on Zulip):

I'd really prefer that

oli (Jun 12 2019 at 06:49, on Zulip):

I mean my changes are easy to revert

oli (Jun 12 2019 at 06:49, on Zulip):

but iirc they cleaned up a few things nicely

eddyb (Jun 12 2019 at 06:50, on Zulip):

yeah I'm a bit torn

oli (Jun 12 2019 at 06:55, on Zulip):

how about we go with the Operand version and I solemly promise that if at some point you're still unhappy enough with promoteds being places, and want them changed, then I'll do the revert myself.

eddyb (Jun 12 2019 at 06:56, on Zulip):

uhm

eddyb (Jun 12 2019 at 06:56, on Zulip):

the Operand version would mean promoteds stop being places

oli (Jun 12 2019 at 06:56, on Zulip):

well

eddyb (Jun 12 2019 at 06:56, on Zulip):

if they're places we use Operand::Copy

oli (Jun 12 2019 at 06:56, on Zulip):

they'd be places and not

oli (Jun 12 2019 at 06:56, on Zulip):

you need them to be places for taking a reference

eddyb (Jun 12 2019 at 06:56, on Zulip):

by "being a place" I mean specifically Place being able to represent them

oli (Jun 12 2019 at 06:56, on Zulip):

PlaceBase::Promoted can't go away

eddyb (Jun 12 2019 at 06:57, on Zulip):

like I said, you can take the reference in the promoted MIR, I think it's how it used to work

eddyb (Jun 12 2019 at 06:57, on Zulip):

@oli then in that case changing Operand is a net negative

oli (Jun 12 2019 at 06:57, on Zulip):

right, that's not what I want :D

oli (Jun 12 2019 at 06:57, on Zulip):

how so?

eddyb (Jun 12 2019 at 06:57, on Zulip):

because Operand::{Copy,Move} can still take promoteds

oli (Jun 12 2019 at 06:57, on Zulip):

oh

oli (Jun 12 2019 at 06:57, on Zulip):

true

eddyb (Jun 12 2019 at 06:58, on Zulip):

like, that's my entire point :P

eddyb (Jun 12 2019 at 06:58, on Zulip):

you either move them from being places to a new Constant/Operand, or work around the problem in other ways

oli (Jun 12 2019 at 06:58, on Zulip):

I mean, Operand::Move can move out of boxes, why not out of promoteds?

eddyb (Jun 12 2019 at 06:58, on Zulip):

one concept I wanted to play around with is splitting places into "operands can refer to them" and "Rvalue::Ref can borrow them" and the latter would be more flexible, including indexing and whatnot

eddyb (Jun 12 2019 at 06:59, on Zulip):

@oli I don't want some weird accidents, I'd rather abuse Operand::Copy instead

oli (Jun 12 2019 at 06:59, on Zulip):

yes, this goes in hand with cleaning up projections

oli (Jun 12 2019 at 07:00, on Zulip):

ok, so I think it would be ok to move back to having some promoteds being &T and some being T and all of them being a custom Operand::Promoted.

davidtwco (Jun 12 2019 at 09:22, on Zulip):

For the feature gate test, @eddyb, what would be the best way in a ui test to observe if a rvalue has been promoted?

eddyb (Jun 12 2019 at 09:29, on Zulip):

@davidtwco other than the fact that it doesn't error when non-Copy?

eddyb (Jun 12 2019 at 09:29, on Zulip):

you could add a MIR test shrug

davidtwco (Jun 12 2019 at 09:37, on Zulip):

I'm struggling to find an example that doesn't compile w/out the feature gate and does with the feature gate.

eddyb (Jun 12 2019 at 09:38, on Zulip):

@davidtwco [None::<String>; 2]

davidtwco (Jun 12 2019 at 09:43, on Zulip):

Huh, I don't know why I didn't come up with that, thanks.

eddyb (Jun 12 2019 at 09:44, on Zulip):

I mean... it's pretty much the thing we want to enable, isn't it :P? I thought your tests already included examples like this, but maybe I didn't look close enough

eddyb (Jun 12 2019 at 10:21, on Zulip):

@davidtwco not sure what you mean in https://github.com/rust-lang/rust/pull/61749#issuecomment-501190070

davidtwco (Jun 12 2019 at 10:21, on Zulip):

I ran the same example that I added a test for, but with a build of master w/out my changes and that's the output.

davidtwco (Jun 12 2019 at 10:22, on Zulip):

(It might not have been exactly master, but it would have been a commit from a maximum of 24 hours ago)

eddyb (Jun 12 2019 at 10:24, on Zulip):

I also pasted that output

eddyb (Jun 12 2019 at 10:24, on Zulip):

my point was that the change in diagnostic output is not in the PR diff

eddyb (Jun 12 2019 at 10:24, on Zulip):

because the test is not in-tree before the PR

davidtwco (Jun 12 2019 at 10:26, on Zulip):

I understood what you meant. I wasn't sure there was much I could do about that (outside of landing another PR before this one), but for some reason I thought that your output might have been from stable and that's why you had said that so I wanted to check what it was on master, my bad.

eddyb (Jun 12 2019 at 10:26, on Zulip):

ah I see, that makes sense

eddyb (Jun 12 2019 at 10:26, on Zulip):

yeah I was thinking maybe we should land such a test. seems odd we don't have anything like this

davidtwco (Jun 12 2019 at 10:30, on Zulip):

If you want to then we can do that, I think that we could just compare against nightly's output and that'd probably be sufficient though.

eddyb (Jun 12 2019 at 10:31, on Zulip):

we can manually compare, but it's not in the PR diff

eddyb (Jun 12 2019 at 10:31, on Zulip):

which may give the wrong impression that the output didn't change

davidtwco (Jun 12 2019 at 10:34, on Zulip):

So far it hasn't?

davidtwco (Jun 12 2019 at 10:34, on Zulip):

Now that I've switched to using report_selection_error.

davidtwco (Jun 12 2019 at 10:34, on Zulip):

Unless I missed a change somewhere.

davidtwco (Jun 12 2019 at 10:35, on Zulip):

Oh, you're right, it did.

davidtwco (Jun 12 2019 at 10:35, on Zulip):
+   = help: the following implementations were found:
+            <Foo<T> as std::marker::Copy>
-   = note: required because of the requirements on the impl of `std::marker::Copy` for `Foo<std::string::String>`
davidtwco (Jun 12 2019 at 10:37, on Zulip):

:face_palm:

eddyb (Jun 12 2019 at 10:37, on Zulip):

yeah, getting the right error with the "trace" is what I don't know how to do

lqd (Jun 12 2019 at 11:47, on Zulip):

@davidtwco in your tests I believe you meant ignore-compare-mode-nll instead of ignore-compile-mode-nll :)

davidtwco (Jun 12 2019 at 11:47, on Zulip):

@lqd oops, I suspect I did. Will fix in a bit. Thanks.

lqd (Jun 12 2019 at 11:47, on Zulip):

np

Last update: Nov 20 2019 at 01:25UTC