Improve intra doc errors display #87285

GuillaumeGomez · 2021-07-19T15:02:21Z

@jyn514 This is what I had in mind to avoid having duplicated backticks. I also gave a try to simply updating the span for the suggestion/help messages but I think this current one is better because less "noisy". Anyway, that allows you to see the result. ;)

rust-highfive · 2021-07-19T15:02:24Z

Some changes occurred in intra-doc-links.

cc @jyn514

rust-highfive · 2021-07-19T15:02:25Z

r? @ollie27

(rust-highfive has picked a reviewer for you, use r? to override)

jyn514 · 2021-07-19T15:51:38Z

r? @jyn514

jyn514

I still think it would be better to use @estebank's verbose suggestion, the backticks are misleading. Narrowing the span to remove the backticks seems useful, but I don't think it solves the original issue.

jyn514 · 2021-07-19T15:55:01Z

src/librustdoc/passes/collect_intra_doc_links.rs

-        let span = super::source_span_for_markdown_range(tcx, dox, link_range, &item.attrs);
+        let span =
+            super::source_span_for_markdown_range(tcx, dox, link_range, &item.attrs).map(|sp| {
+                if dox.bytes().nth(link_range.start) == Some(b'`') {


It's not actually required that the backticks match - can you also check that the ending backtick is present?

Suggested change

if dox.bytes().nth(link_range.start) == Some(b'`') {

if dox.bytes().nth(link_range.start) == Some(b'`') && dox.bytes().nth(link_range.end) == Some(b'`') {

I was wondering originally why it wasn't checking both the beginning and the end. Well, I guess that answers my question. :)

estebank · 2021-07-19T16:09:18Z

src/test/rustdoc-ui/intra-doc/ambiguity.stderr

 help: to link to the enum, prefix with `enum@`
   |
 LL | /// Ambiguous non-implied shortcut link [`enum@foo::bar`].
-   |                                          ^^^^^^^^^^^^^^^
+   |                                           ^^^^^^^^^^^^^
 help: to link to the function, add parentheses
   |
 LL | /// Ambiguous non-implied shortcut link [`foo::bar()`].
-   |                                          ^^^^^^^^^^^^
+   |                                           ^^^^^^^^^^


Given that these are being shown as verbose already (because there's more than one suggestion), it'd be nice to make the span smaller, only covering the enum@ and ().

This is an interesting take. Although, not sure it would be an improvement in these cases because they ask to add things, not remove them.

This would look like this:

error: `foo::bar` is both an enum and a function --> $DIR/ambiguity.rs:35:43 | LL | /// Ambiguous non-implied shortcut link [`foo::bar`]. | ^^^^^^^^ ambiguous link | help: to link to the enum, prefix with `enum@` | LL | /// Ambiguous non-implied shortcut link [`enum@foo::bar`]. | ^^^^^ help: to link to the function, add parentheses | LL | /// Ambiguous non-implied shortcut link [`foo::bar()`]. | ^^

which to my eyes looks easier to understand at a glance.

GuillaumeGomez · 2021-07-20T11:13:12Z

I still think it would be better to use @estebank's verbose suggestion, the backticks are misleading. Narrowing the span to remove the backticks seems useful, but I don't think it solves the original issue.

@estebank confirmed that it was already using verbose because there are multiple messages, so on this part I think it's already fine?

What is unresolved with the current changes? My complain was that the error message was unclear because of the duplication of the backticks. So instead of including the whole intra-doc element, I only include the part in the backticks. If you have something else in mind, I can do it here as well if you want.

jyn514 · 2021-07-20T13:10:05Z

@estebank confirmed that it was already using verbose because there are multiple messages, so on this part I think it's already fine?

It happened in that specific case because there happened to be multiple help messages. Almost always there will be only 0 or 1.

What is unresolved with the current changes? My complain was that the error message was unclear because of the duplication of the backticks. So instead of including the whole intra-doc element, I only include the part in the backticks. If you have something else in mind, I can do it here as well if you want.

My concern is that showing backticks at all is confusing - showing 1 backtick is actually even more confusing because it's not obviously a bug. Take this diagnostic: https://github.com/rust-lang/rust/blob/7f0cb81c8e4743aacc37167a7299ea0a85f005af/src/test/rustdoc-ui/intra-doc/disambiguator-mismatch.stderr#L1-L5

That makes it look like you should add backticks when they weren't there before, I've seen people running into this in practice: lightningdevkit/rust-lightning#955 (comment)
Actually after reading that, I wonder if we should change this for any markdown suggestion ...

GuillaumeGomez · 2021-07-20T14:23:36Z

My concern is that showing backticks at all is confusing - showing 1 backtick is actually even more confusing because it's not obviously a bug. Take this diagnostic:
...
That makes it look like you should add backticks when they weren't there before, I've seen people running into this in practice: lightningdevkit/rust-lightning#955 (comment)
Actually after reading that, I wonder if we should change this for any markdown suggestion ...

We use backticks literally everywhere:

LL | fn mk_int() -> usize { let i: isize = 3; return i; }
   |                -----                            ^ expected `usize`, found `isize`
   |                |
   |                expected `usize` because of return type

So this is coherent with all other error outputs. But again, I might misunderstand you...

It happened in that specific case because there happened to be multiple help messages. Almost always there will be only 0 or 1.

It's definitely worth adding a new test to ensure the output for an error with just one message then!

jyn514 · 2021-07-20T14:34:22Z

We use backticks literally everywhere:

Yes, I understand that. In most parts of the compiler it's not ambiguous, because adding the backtick will cause the code to fail to compile. But in markdown it's valid syntax, and changes the meaning of the code. So we shouldn't suggest it, because it's not easy for the user to tell it's formatting and not part of the suggestion.

GuillaumeGomez · 2021-07-20T14:47:31Z

I see, it's why I changed the span in order to only underline the parts without backticks so we can use the same logic. But at least I understand your point now.

estebank · 2021-07-20T16:16:25Z

I'm for the change to make the span smaller. I would also prefer it if we could use span_suggestion_verbose more liberally than we currently are, except for situations where we are suggesting a complete substitution. The reason I haven't mass replaced them is because some people will complain about added verbosity, but at the same time a different subset of people does get confused on small changes (like adding() at the end, but pointing at the whole thing). This later group ends up frustrated thinking "But that's what I have!?". We have an opportunity to avoid that frustration.

GuillaumeGomez · 2021-07-20T17:07:02Z

Definitely sounds like an improvement. I'm going to give it a try tomorrow!

GuillaumeGomez · 2021-07-28T19:19:42Z

With the (great!) help of @estebank, I was able to greatly improve the error output. You can now admire! 😃

src/librustdoc/passes/collect_intra_doc_links.rs

src/test/rustdoc-ui/intra-doc/disambiguator-mismatch.stderr

src/test/rustdoc-ui/intra-doc/errors.stderr

estebank · 2021-07-29T09:12:11Z

src/test/rustdoc-ui/intra-doc/errors.stderr

+help: to link to the macro, add an exclamation mark
+   |
+LL | /// [m!()]
+   |       ^


For this case the replacement span was pointing at the point immediately after m, but it needs to start there and end at the current sp.hi().

Somehow, even after all the other cases have been fixed, this one hasn't 🤔

src/librustdoc/passes/collect_intra_doc_links.rs

GuillaumeGomez · 2021-07-29T11:40:23Z

Updated!

estebank · 2021-07-29T12:22:16Z

src/librustdoc/passes/collect_intra_doc_links.rs

+        if spans.len() > 1 {
+            diag.multipart_suggestion(&help, spans, Applicability::MaybeIncorrect);
        } else {
-            suggestion.as_help(path_str)
-        };
-
-        diag.span_suggestion(sp, &help, msg, Applicability::MaybeIncorrect);
+            let (sp, suggestion_text) = spans.pop().unwrap();
+            diag.span_suggestion_verbose(sp, &help, suggestion_text, Applicability::MaybeIncorrect);
+        }


I think that you can just always call multipart_suggestion_verbose to get the same effect. Otherwise, it's fine to keep this as is.

This function doesn't exist: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_errors/diagnostic/struct.Diagnostic.html?search=multipart_suggestion_verbose#method.multipart_suggestion

Ah! I added one, but in a PR that hasn't been merged yet. Disregard for now then.

estebank · 2021-07-29T12:27:13Z

src/librustdoc/passes/collect_intra_doc_links.rs

+                sugg
+            }
+            Self::Macro => {
+                let mut sugg = vec![(inner_sp.shrink_to_hi(), "!".to_string())];


This is needed to account for the foo() -> foo! change, so the span covers the () and gets replaced by !:

Suggested change

let mut sugg = vec![(inner_sp.shrink_to_hi(), "!".to_string())];

let mut sugg = vec![(inner_sp.shrink_to_hi().with_hi(sp.hi()), "!".to_string())];

Good point, I'll add a specific check for that.

Actually, it is a valid link to have macro!(), so I'll keep as is. :)

estebank

r=me after addressing the outstanding incorrect suggestion.

Love the results!

GuillaumeGomez · 2021-07-29T16:42:19Z

@bors: r=estebank

bors · 2021-07-29T16:42:20Z

📌 Commit 0f7f85e has been approved by estebank

…tebank Improve intra doc errors display rust-lang#87169 `@jyn514` This is what I had in mind to avoid having duplicated backticks. I also gave a try to simply updating the span for the suggestion/help messages but I think this current one is better because less "noisy". Anyway, that allows you to see the result. ;)

…tebank Improve intra doc errors display rust-lang#87169 ``@jyn514`` This is what I had in mind to avoid having duplicated backticks. I also gave a try to simply updating the span for the suggestion/help messages but I think this current one is better because less "noisy". Anyway, that allows you to see the result. ;)

jyn514 · 2021-07-29T16:55:40Z

I see that this has already been put in a rollup, but I'd really prefer to see a test for multiple overlaps in a namespace: #87285 (comment)

GuillaumeGomez · 2021-07-29T17:01:06Z

I answered on the discussion: I added a function with the same name. ;)

bors · 2021-07-29T23:33:23Z

⌛ Testing commit 0f7f85e with merge e66a8c2...

bors · 2021-07-30T02:14:01Z

☀️ Test successful - checks-actions
Approved by: estebank
Pushing e66a8c2 to master...

GuillaumeGomez added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. A-rustdoc-ui Area: Rustdoc UI (generated HTML) labels Jul 19, 2021

rust-highfive assigned ollie27 Jul 19, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 19, 2021

rust-highfive assigned jyn514 and unassigned ollie27 Jul 19, 2021

jyn514 added A-diagnostics Area: Messages for errors, warnings, and lints A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name and removed A-rustdoc-ui Area: Rustdoc UI (generated HTML) labels Jul 19, 2021

jyn514 suggested changes Jul 19, 2021

View reviewed changes

estebank reviewed Jul 19, 2021

View reviewed changes

GuillaumeGomez force-pushed the intra-doc-span branch from 8d91e49 to 28f0a97 Compare July 20, 2021 11:11

GuillaumeGomez marked this pull request as ready for review July 20, 2021 11:13

This comment has been minimized.

Sign in to view

GuillaumeGomez force-pushed the intra-doc-span branch from 28f0a97 to 7f0cb81 Compare July 20, 2021 11:47

GuillaumeGomez force-pushed the intra-doc-span branch 2 times, most recently from 3829068 to d39a544 Compare July 28, 2021 19:18

GuillaumeGomez changed the title ~~Intra doc error span~~ Improve intra doc errors display Jul 28, 2021

estebank reviewed Jul 28, 2021

View reviewed changes

src/librustdoc/passes/collect_intra_doc_links.rs Outdated Show resolved Hide resolved

estebank reviewed Jul 28, 2021

View reviewed changes

src/test/rustdoc-ui/intra-doc/disambiguator-mismatch.stderr Outdated Show resolved Hide resolved

GuillaumeGomez force-pushed the intra-doc-span branch from d39a544 to 9dc0fbb Compare July 29, 2021 09:03

estebank reviewed Jul 29, 2021

View reviewed changes

Change span for intra-doc links errors

a2f3e4a

GuillaumeGomez force-pushed the intra-doc-span branch from 9dc0fbb to bcf057a Compare July 29, 2021 11:37

estebank reviewed Jul 29, 2021

View reviewed changes

estebank approved these changes Jul 29, 2021

View reviewed changes

jyn514 assigned estebank and unassigned jyn514 Jul 29, 2021

Update rustdoc-ui tests for intra-doc links errors

0f7f85e

GuillaumeGomez force-pushed the intra-doc-span branch from bcf057a to 0f7f85e Compare July 29, 2021 15:36

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 29, 2021

GuillaumeGomez mentioned this pull request Jul 29, 2021

Rollup of 10 pull requests #87593

Closed

fee1-dead mentioned this pull request Jul 29, 2021

Rollup of 9 pull requests #87595

Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 30, 2021

bors merged commit e66a8c2 into rust-lang:master Jul 30, 2021

rustbot added this to the 1.56.0 milestone Jul 30, 2021

GuillaumeGomez deleted the intra-doc-span branch July 30, 2021 07:55

GuillaumeGomez mentioned this pull request Aug 2, 2021

Improve error messages for intra-doc links when there are backticks #87169

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve intra doc errors display #87285

Improve intra doc errors display #87285

GuillaumeGomez commented Jul 19, 2021 •

edited

Loading

rust-highfive commented Jul 19, 2021

rust-highfive commented Jul 19, 2021

jyn514 commented Jul 19, 2021

jyn514 left a comment

jyn514 Jul 19, 2021

GuillaumeGomez Jul 20, 2021

estebank Jul 19, 2021

GuillaumeGomez Jul 20, 2021

estebank Jul 20, 2021

GuillaumeGomez commented Jul 20, 2021

This comment has been minimized.

jyn514 commented Jul 20, 2021

GuillaumeGomez commented Jul 20, 2021

jyn514 commented Jul 20, 2021

GuillaumeGomez commented Jul 20, 2021

estebank commented Jul 20, 2021

GuillaumeGomez commented Jul 20, 2021

GuillaumeGomez commented Jul 28, 2021 •

edited

Loading

estebank Jul 29, 2021

estebank Jul 29, 2021

GuillaumeGomez commented Jul 29, 2021

estebank Jul 29, 2021

GuillaumeGomez Jul 29, 2021

estebank Jul 29, 2021

estebank Jul 29, 2021 •

edited

Loading

GuillaumeGomez Jul 29, 2021

GuillaumeGomez Jul 29, 2021

estebank left a comment

GuillaumeGomez commented Jul 29, 2021

bors commented Jul 29, 2021

jyn514 commented Jul 29, 2021

GuillaumeGomez commented Jul 29, 2021

bors commented Jul 29, 2021

bors commented Jul 30, 2021

	if dox.bytes().nth(link_range.start) == Some(b'`') {
	if dox.bytes().nth(link_range.start) == Some(b'`') && dox.bytes().nth(link_range.end) == Some(b'`') {

	let mut sugg = vec![(inner_sp.shrink_to_hi(), "!".to_string())];
	let mut sugg = vec![(inner_sp.shrink_to_hi().with_hi(sp.hi()), "!".to_string())];

Improve intra doc errors display #87285

Improve intra doc errors display #87285

Conversation

GuillaumeGomez commented Jul 19, 2021 • edited Loading

rust-highfive commented Jul 19, 2021

rust-highfive commented Jul 19, 2021

jyn514 commented Jul 19, 2021

jyn514 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Jul 20, 2021

This comment has been minimized.

jyn514 commented Jul 20, 2021

GuillaumeGomez commented Jul 20, 2021

jyn514 commented Jul 20, 2021

GuillaumeGomez commented Jul 20, 2021

estebank commented Jul 20, 2021

GuillaumeGomez commented Jul 20, 2021

GuillaumeGomez commented Jul 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Jul 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estebank Jul 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estebank left a comment

Choose a reason for hiding this comment

GuillaumeGomez commented Jul 29, 2021

bors commented Jul 29, 2021

jyn514 commented Jul 29, 2021

GuillaumeGomez commented Jul 29, 2021

bors commented Jul 29, 2021

bors commented Jul 30, 2021

GuillaumeGomez commented Jul 19, 2021 •

edited

Loading

GuillaumeGomez commented Jul 28, 2021 •

edited

Loading

estebank Jul 29, 2021 •

edited

Loading