Update E0050 to new error format #36376

GuillaumeGomez · 2016-09-10T13:28:57Z

sophiajt · 2016-09-12T14:58:31Z

src/librustc_typeck/check/compare_method.rs

+                    impl_m_span
+                } else {
+                    mk_sp(impl_m_sig.decl.inputs[0].pat.span.lo,
+                          impl_m_sig.decl.inputs.last().unwrap().pat.span.hi)


Can you put a comment in here about why it's safe to mix spans? ie - how does this avoid the common pitfalls of mixing spans together?

sophiajt · 2016-09-12T15:01:27Z

Just had a couple points - needs a bit of commenting, and would be good, since you're changing spans, to move the unit test to a UI test.

GuillaumeGomez · 2016-09-18T12:16:45Z

Updated (I decided to use multispan in the end, way better).

sophiajt · 2016-09-19T15:59:35Z

From travis:

error: /build/src/test/compile-fail/E0050.rs:12: unexpected "note": 'expected 2 parameters'

error: /build/src/test/compile-fail/E0050.rs:18: unexpected "note": 'found 1 parameter'

error: /build/src/test/compile-fail/E0050.rs:18: expected note not found: expected 2 parameters

error: 2 unexpected errors found, 1 expected errors not found
status: exit code: 101
command: /build/build/x86_64-unknown-linux-gnu/stage2/bin/rustc /build/src/test/compile-fail/E0050.rs -L /build/build/x86_64-unknown-linux-gnu/test/compile-fail --target=x86_64-unknown-linux-gnu --error-format json -L /build/build/x86_64-unknown-linux-gnu/test/compile-fail/E0050.stage2-x86_64-unknown-linux-gnu.compile-fail.libaux -C prefer-dynamic -o /build/build/x86_64-unknown-linux-gnu/test/compile-fail/E0050.stage2-x86_64-unknown-linux-gnu -Crpath -O -Lnative=/build/build/x86_64-unknown-linux-gnu/rust-test-helpers
unexpected errors (from JSON output): [
    Error {
        line_num: 12,
        kind: Some(
            Note
        ),
        msg: "expected 2 parameters"
    },
    Error {
        line_num: 18,
        kind: Some(
            Note
        ),
        msg: "found 1 parameter"
    }
]

not found errors (from test file): [
    Error {
        line_num: 18,
        kind: Some(
            Note
        ),
        msg: "expected 2 parameters"
    }
]

thread '[compile-fail] compile-fail/E0050.rs' panicked at 'explicit panic', src/tools/compiletest/src/runtest.rs:1084
note: Run with `RUST_BACKTRACE=1` for a backtrace.


failures:
    [compile-fail] compile-fail/E0050.rs

test result: FAILED. 2571 passed; 1 failed; 12 ignored; 0 measured

GuillaumeGomez · 2016-09-19T16:37:06Z

Updated.

sophiajt · 2016-09-22T15:30:32Z

@GuillaumeGomez - if you want to merge the spans for the arguments, you should be able to now. I have a new method in CodeMap called merge_spans that you can use.

GuillaumeGomez · 2016-09-23T10:40:13Z

Yeay! \o/

GuillaumeGomez · 2016-09-24T21:32:47Z

Updated.

Also, please remember that "^" is red while "-" is blue. The output is nicer that what you might think.

bstrie · 2016-09-25T16:01:51Z

r? @jonathandturner

sophiajt · 2016-09-25T16:13:13Z

@bstrie yup, working out best path now. Currently potentially blocked by #36703

sophiajt · 2016-09-27T20:22:50Z

src/librustc_typeck/check/compare_method.rs

+        let (diff, text) = if impl_m.fty.sig.0.inputs.len() > trait_m.fty.sig.0.inputs.len() {
+            (impl_m.fty.sig.0.inputs.len() - trait_m.fty.sig.0.inputs.len(), "less")
+        } else {
+            (trait_m.fty.sig.0.inputs.len() - impl_m.fty.sig.0.inputs.len(), "more")


nit: you could put impl_m.fty.sig.0.inputs.len() and trait_m.fty.sig.0.inputs.len() in variables rather than repeating them

sophiajt

I've added some comments. I think the big thing is following something closer to #35211

sophiajt · 2016-09-27T20:24:32Z

src/test/ui/span/E0050.stderr

+   |     ----------------------------- expected 2 parameters
+...
+18 |     fn foo(&self, y: u8, z: u8) -> bool { true }
+   |     -------^^^^^--^------^----------------------


This shouldn't be needed anymore

sophiajt · 2016-09-27T20:27:36Z

src/test/compile-fail/E0050.rs

+    fn bar(&self) { } //~ ERROR E0050
+                      //~| NOTE found 1 parameter
+    fn less(&self, x: u8, y: u8, z: u8) { } //~ ERROR E0050
+                                            //~| NOTE found 4 parameters


Rather than putting "found x parameters" here, I think the design in #35211 would read better. Specifically the primary errors showing specifically what is going wrong.

sophiajt · 2016-09-27T21:37:01Z

src/librustc_typeck/check/compare_method.rs

+                                 } else {
+                                     format!("{} parameter", trait_number_args)
+                                 }))
+            .span_label(span,


It's hard to tell by looking, but I'd expect this .span_label to use the same span as the struct_span_err!. Do you know why they're different?

I mention it because they have to be the same span for the ^^^ primary underline.

Yup, I just tried building it and they aren't the same span, which gives you output like this:

error[E0050]: method `less` has 4 parameters but the declaration in trait `Foo::less` has 1 --> src/test/compile-fail/E0050.rs:24:13 | 14 | fn less(&self); //~ NOTE trait requires 1 parameter | --------------- trait requires 1 parameter ... 24 | fn less(&self, x: u8, y: u8, z: u8) { } //~ ERROR E0050 | --------^^^^^-------------------------- | | | expected 1 parameter, found 4

Yeah looks like you just need:

- .span_label(span, + .span_label(mspan,

And remove the unused span. With that it's at least closer:

error[E0050]: method `bar` has 1 parameter but the declaration in trait `Foo::bar` has 4 --> src/test/compile-fail/E0050.rs:22:5 | 13 | fn bar(&self, x: u8, y: u8, z: u8); //~ NOTE trait requires 4 parameters | ----------------------------------- trait requires 4 parameters ... 22 | fn bar(&self) { } //~ ERROR E0050 | ^^^^^^^^^^^^^^^^^ expected 4 parameters, found 1 error[E0050]: method `less` has 4 parameters but the declaration in trait `Foo::less` has 1 --> src/test/compile-fail/E0050.rs:24:13 | 14 | fn less(&self); //~ NOTE trait requires 1 parameter | --------------- trait requires 1 parameter ... 24 | fn less(&self, x: u8, y: u8, z: u8) { } //~ ERROR E0050 | ^^^^^ expected 1 parameter, found 4

Not sure why it's doing the full underline rather than just the parameter...

Played around a bit more. You can see what I ended up doing here: https://github.com/jonathandturner/rust/tree/e0050

Changes:

Focused the trait secondary span on the parameter rather than underlining everything

Focused the impl primary span on the parameter rather than underlining everything in the case of the trait having more parameters than the impl

Made the displaying of the trait span optional since the trait may have come from outside the current crate

GuillaumeGomez · 2016-09-28T09:41:13Z

@jonathandturner: I like your changes! Now it should be finally good. :D

sophiajt · 2016-09-28T16:59:24Z

@bors r+ rollup

bors · 2016-09-28T16:59:25Z

📌 Commit 61cbf41 has been approved by jonathandturner

…rner Update E0050 to new error format Part of rust-lang#35233. Fixes rust-lang#35211. r? @jonathandturner

Rollup of 11 pull requests - Successful merges: #36376, #36672, #36740, #36757, #36765, #36769, #36782, #36783, #36784, #36795, #36796 - Failed merges:

rust-highfive assigned sophiajt Sep 10, 2016

GuillaumeGomez force-pushed the e0050 branch 2 times, most recently from 9a9b45f to 2260b69 Compare September 10, 2016 15:55

sophiajt reviewed Sep 12, 2016
View reviewed changes

GuillaumeGomez force-pushed the e0050 branch from 2260b69 to 7f9b576 Compare September 18, 2016 12:16

GuillaumeGomez force-pushed the e0050 branch from 7f9b576 to 09dd053 Compare September 19, 2016 16:32

GuillaumeGomez force-pushed the e0050 branch 2 times, most recently from 76abee7 to d3fd646 Compare September 24, 2016 21:31

GuillaumeGomez force-pushed the e0050 branch from d3fd646 to 8757c94 Compare September 27, 2016 20:20

sophiajt reviewed Sep 27, 2016

View reviewed changes

GuillaumeGomez force-pushed the e0050 branch 2 times, most recently from 191f754 to 07598f5 Compare September 27, 2016 21:23

sophiajt reviewed Sep 27, 2016

View reviewed changes

Update E0050 to new error format

61cbf41

GuillaumeGomez force-pushed the e0050 branch from 07598f5 to 61cbf41 Compare September 28, 2016 09:40

sophiajt pushed a commit to sophiajt/rust that referenced this pull request Sep 28, 2016

Rollup merge of rust-lang#36376 - GuillaumeGomez:e0050, r=jonathandtu…

475a41b

…rner Update E0050 to new error format Part of rust-lang#35233. Fixes rust-lang#35211. r? @jonathandturner

sophiajt mentioned this pull request Sep 28, 2016

Rollup of 11 pull requests #36805

Merged

bors added a commit that referenced this pull request Sep 28, 2016

Auto merge of #36805 - jonathandturner:rollup, r=jonathandturner

86affcd

Rollup of 11 pull requests - Successful merges: #36376, #36672, #36740, #36757, #36765, #36769, #36782, #36783, #36784, #36795, #36796 - Failed merges:

bors merged commit 61cbf41 into rust-lang:master Sep 29, 2016

GuillaumeGomez deleted the e0050 branch September 29, 2016 09:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update E0050 to new error format #36376

Update E0050 to new error format #36376

GuillaumeGomez commented Sep 10, 2016

sophiajt Sep 12, 2016 •

edited

Loading

sophiajt commented Sep 12, 2016

GuillaumeGomez commented Sep 18, 2016 •

edited

Loading

sophiajt commented Sep 19, 2016

GuillaumeGomez commented Sep 19, 2016

sophiajt commented Sep 22, 2016

GuillaumeGomez commented Sep 23, 2016

GuillaumeGomez commented Sep 24, 2016

bstrie commented Sep 25, 2016

sophiajt commented Sep 25, 2016

sophiajt Sep 27, 2016

sophiajt left a comment

sophiajt Sep 27, 2016

sophiajt Sep 27, 2016

sophiajt Sep 27, 2016 •

edited

Loading

sophiajt Sep 27, 2016

sophiajt Sep 27, 2016

sophiajt Sep 27, 2016

GuillaumeGomez commented Sep 28, 2016

sophiajt commented Sep 28, 2016

bors commented Sep 28, 2016

Update E0050 to new error format #36376

Update E0050 to new error format #36376

Conversation

GuillaumeGomez commented Sep 10, 2016

sophiajt Sep 12, 2016 • edited Loading

Choose a reason for hiding this comment

sophiajt commented Sep 12, 2016

GuillaumeGomez commented Sep 18, 2016 • edited Loading

sophiajt commented Sep 19, 2016

GuillaumeGomez commented Sep 19, 2016

sophiajt commented Sep 22, 2016

GuillaumeGomez commented Sep 23, 2016

GuillaumeGomez commented Sep 24, 2016

bstrie commented Sep 25, 2016

sophiajt commented Sep 25, 2016

sophiajt Sep 27, 2016

Choose a reason for hiding this comment

sophiajt left a comment

Choose a reason for hiding this comment

sophiajt Sep 27, 2016

Choose a reason for hiding this comment

sophiajt Sep 27, 2016

Choose a reason for hiding this comment

sophiajt Sep 27, 2016 • edited Loading

Choose a reason for hiding this comment

sophiajt Sep 27, 2016

Choose a reason for hiding this comment

sophiajt Sep 27, 2016

Choose a reason for hiding this comment

sophiajt Sep 27, 2016

Choose a reason for hiding this comment

GuillaumeGomez commented Sep 28, 2016

sophiajt commented Sep 28, 2016

bors commented Sep 28, 2016

sophiajt Sep 12, 2016 •

edited

Loading

GuillaumeGomez commented Sep 18, 2016 •

edited

Loading

sophiajt Sep 27, 2016 •

edited

Loading