Teach diagnostics to correct margin of multiline messages #38916

estebank · 2017-01-08T05:16:20Z

Make the suggestion list have a correct padding:

error[E0308]: mismatched types
 --> file.rs:3:20
  |
3 |     let x: usize = "";
  |                    ^^ expected usize, found reference
  |
  = note: expected type `usize`
  = note:    found type `&'static str`
  = help: here are some functions which might fulfill your needs:
          - .len()
          - .foo()
          - .bar()

rust-highfive · 2017-01-08T05:16:32Z

r? @nikomatsakis

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

Make the suggestion list have a correct padding: ``` error[E0308]: mismatched types --> file.rs:3:20 | 3 | let x: usize = ""; | ^^ expected usize, found reference | = note: expected type `usize` = note: found type `&'static str` = help: here are some functions which might fulfill your needs: - .len() - .foo() - .bar() ```

KalitaAlexey · 2017-01-08T10:08:54Z

src/librustc_errors/emitter.rs

+                                format!("{}\n{}",
+                                        &child.message,
+                                        &child.list.iter().map(|item| {
+                                            format!("{}         - {}",


It is hard for to understand the inner loop.
Can you describe it?

Added comments.

KalitaAlexey · 2017-01-08T10:09:37Z

src/librustc_errors/emitter.rs

+                                                          .collect::<String>(),
+                                                    item)
+                                        }).collect::<Vec<String>>()
+                                        .join("\n"))


Are you sure that collect and join are more efficient than fold?

EDIT:
I think that it is more efficient.

KalitaAlexey · 2017-01-08T10:12:23Z

src/librustc_errors/emitter.rs

                    .to_string(),
                span: MultiSpan::new(),
                render_span: None,
+                list: vec![],


@nikomatsakis,
Is vec![] idiomatic way to create an empty vector?
I mean that vec! is a macro.
It requires expansion.
I think that Vec::new is more appropriate.

KalitaAlexey · 2017-01-08T10:19:13Z

src/librustc_errors/diagnostic.rs

           message: &str,
           span: MultiSpan,
           render_span: Option<RenderSpan>) {
+        self.sub_with_list(level, message, span, render_span, vec![]);


@nikomatsakis,
Is vec![] idiomatic way to create an empty vector?
I mean that vec! is a macro.
It requires expansion.
I think that Vec::new is more appropriate.

estebank · 2017-01-08T22:10:24Z

I realized that this could be generalized in a better way by making the diagnostic renderer aware of multiline strings, and make it provide the appropriate margin instead. By doing it that way

the diagnostics do not need to care about lists at all
multiline strings now should look much better (not that there're many at this time)
the json output remains untouched, showing the unpadded multiline string

KalitaAlexey · 2017-01-08T22:26:35Z

src/librustc_errors/emitter.rs

If I get it right, it will allocate every time the method is called.
What do you think about adding a function

fn add_padding(msg: &mut String, padding: usize) { for _ in 0..padding { msg.push(' '); } }

and using it instead of pushing padding.

KalitaAlexey · 2017-01-08T22:28:22Z

src/librustc_errors/emitter.rs

I need other opinions, but I think that before 711 lines you call measure a new string size and reserve it to prevent allocations in 713, 715, 716 lines.

durka · 2017-01-08T22:50:00Z

src/librustc_errors/emitter.rs

How about "note".len() + 5 (and "suggestion".len() + 5 further down) to have one magic number instead of two?

Make any diagnostic line to have the correct margin to align with the first line: ``` error: message --> file.rs:3:20 | 3 | <CODE> | ^^^^ | = note: this is a multiline note with a correct margin = note: this is a single line note = help: here are some functions which might fulfill your needs: - .len() - .foo() - .bar() = suggestion: this is a multiline suggestion with a correct margin ```

KalitaAlexey · 2017-01-09T07:52:17Z

src/librustc_errors/emitter.rs

-            buffer.append(0, msg, Style::NoStyle);
+
+            // The extra 3 ` ` is the padding that's always needed to align to the `note: `.
+            let message = self.msg_with_padding(msg, max_line_num_len + "note: ".len() + 3);


What does 3 stand for?

Added this documentation to the code.

nikomatsakis · 2017-01-10T18:18:33Z

@bors r+

This is very nice.

bors · 2017-01-10T18:18:34Z

📌 Commit 04e4a60 has been approved by nikomatsakis

nikomatsakis · 2017-01-10T18:17:22Z

src/librustc_errors/emitter.rs


+    /// Add a left margin to every line but the first, given a padding length and the label being
+    /// displayed.
+    fn msg_with_padding(&self, msg: &str, padding: usize, label: &str) -> String {


It seems like the labels attached to a span might also need wrapping, not just notes, right?

But I guess this code is not about "autowrap" but specifically targeting \n that are embedded in a message. I see, carry on.

bors · 2017-01-11T05:26:46Z

⌛ Testing commit 04e4a60 with merge e57f061...

Teach diagnostics to correct margin of multiline messages Make the suggestion list have a correct padding: ``` error[E0308]: mismatched types --> file.rs:3:20 | 3 | let x: usize = ""; | ^^ expected usize, found reference | = note: expected type `usize` = note: found type `&'static str` = help: here are some functions which might fulfill your needs: - .len() - .foo() - .bar() ```

bors · 2017-01-11T07:29:04Z

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

rust-highfive assigned nikomatsakis Jan 8, 2017

estebank mentioned this pull request Jan 8, 2017

Remove note: prefix from type mismatch errors #38902

Closed

estebank force-pushed the pad-suggestion-list branch 2 times, most recently from db5ec8b to 7f7a0dc Compare January 8, 2017 07:24

estebank force-pushed the pad-suggestion-list branch from 7f7a0dc to 43b10fa Compare January 8, 2017 07:34

KalitaAlexey reviewed Jan 8, 2017

View reviewed changes

estebank force-pushed the pad-suggestion-list branch 2 times, most recently from 3d4a19d to a54d700 Compare January 8, 2017 21:11

Use fold instead of collect/join and add comments

f65a907

estebank force-pushed the pad-suggestion-list branch 2 times, most recently from a934320 to ba7503c Compare January 8, 2017 22:10

estebank changed the title ~~Teach diagnostics to have correctly padded suggestion lists~~ Teach diagnostics to correct the margin of multiline messages Jan 8, 2017

estebank changed the title ~~Teach diagnostics to correct the margin of multiline messages~~ Teach diagnostics to correct margin of multiline messages Jan 8, 2017

KalitaAlexey reviewed Jan 8, 2017

View reviewed changes

durka reviewed Jan 8, 2017

View reviewed changes

estebank force-pushed the pad-suggestion-list branch from ba7503c to b206064 Compare January 9, 2017 00:07

Remove magic number

6904761

KalitaAlexey reviewed Jan 9, 2017

View reviewed changes

Deduplicate and document logic

04e4a60

estebank force-pushed the pad-suggestion-list branch from e424c0e to 04e4a60 Compare January 9, 2017 17:11

estebank mentioned this pull request Jan 9, 2017

Make the expected/found notes a first class comment #38901

Closed

nikomatsakis approved these changes Jan 10, 2017

View reviewed changes

estebank mentioned this pull request Jan 10, 2017

Teach Diagnostics to highlight text #38955

Merged

bors merged commit 04e4a60 into rust-lang:master Jan 11, 2017

bors mentioned this pull request Jan 11, 2017

Ref suggestion #37658

Merged

estebank deleted the pad-suggestion-list branch November 9, 2023 05:27

Uh oh!

Teach diagnostics to correct margin of multiline messages #38916

Teach diagnostics to correct margin of multiline messages #38916

Uh oh!

Conversation

estebank commented Jan 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Jan 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KalitaAlexey Jan 8, 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!

KalitaAlexey Jan 8, 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!

Choose a reason for hiding this comment

Uh oh!

estebank commented Jan 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikomatsakis commented Jan 10, 2017

Uh oh!

bors commented Jan 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bors commented Jan 11, 2017

Uh oh!

bors commented Jan 11, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

estebank commented Jan 8, 2017 •

edited

Loading

KalitaAlexey Jan 8, 2017 •

edited

Loading

KalitaAlexey Jan 8, 2017 •

edited

Loading