Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
GuillaumeGomez opened this issue Jul 15, 2021 · 8 comments
Closed
Labels
A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@GuillaumeGomez
Copy link
Member

GuillaumeGomez commented Jul 15, 2021

In #87078, we arrived at the following situation:

error: incompatible link kind for `Foo::bar`
  --> $DIR/field-ice.rs:5:6
   |
LL | /// [`Foo::bar()`]
   |      ^^^^^^^^^^^^ help: to link to the field, remove the disambiguator: ``Foo::bar``

I want to underline this part in particular:

``Foo::bar``

Having two backticks is very weird. What I suggest instead is to reduce the "^^^^^" underlining part in order to not include backticks in it so then they can be removed from the error message as well, which would give:

error: incompatible link kind for `Foo::bar`
  --> $DIR/field-ice.rs:5:6
   |
LL | /// [`Foo::bar()`]
   |       ^^^^^^^^^^ help: to link to the field, remove the disambiguator: `Foo::bar`

I'll do it in the next days if no one does until then. :)

cc @jyn514

@GuillaumeGomez GuillaumeGomez added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name labels Jul 15, 2021
@estebank
Copy link
Contributor

estebank commented Jul 16, 2021

You can fix this by using a span_suggestion_verbose instead of span_suggestion:

diag.span_suggestion(sp, &help, msg, Applicability::MaybeIncorrect);

It'll take more vertical space, but it's ok.


If someone picks this up as their first contribution, you need to --bless the .stderr test files by running ./x.py test src/test/rustdoc-ui --stage 1 --bless from the root of the repo directory.

@estebank estebank added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. labels Jul 16, 2021
@b41sh
Copy link

b41sh commented Jul 17, 2021

@rustbot claim

@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented Jul 17, 2021

@estebank It's a bit different here because we create the spans ourselves because they come from inside a doc comment. So I'm not sure if your suggestion will work out.

Also, in here it's src/test/rustdoc-ui, not src/test/ui (but otherwise it's the same). ;)

@estebank
Copy link
Contributor

@GuillaumeGomez thanks for the correction!

As long as the span was synthesized correctly (including what side of a char the boundary falls on), then just changing the method will work as expected.

@GuillaumeGomez
Copy link
Member Author

I had in mind to simply update the span instead in case the string was starting and ending with backticks.

@jyn514
Copy link
Member

jyn514 commented Jul 17, 2021

@GuillaumeGomez look at the code that's already there:

let msg = if dox.bytes().nth(link_range.start) == Some(b'`') {
format!("`{}`", suggestion.as_help(path_str))
} else {
suggestion.as_help(path_str)
};

If you think this shouldn't use verbose, remove that instead. But I think it's better to show backticks if the user had them there originally.

@GuillaumeGomez
Copy link
Member Author

We can take advantage of the span to exclude the backticks from the underlining, so then we can avoid displaying the backticks twice in the suggestion. As for the how, I guess it's fine as long as we get the output we want.

@b41sh b41sh removed their assignment Jul 29, 2021
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Jul 29, 2021
…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. ;)
fee1-dead added a commit to fee1-dead-contrib/rust that referenced this issue Jul 29, 2021
…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. ;)
bors added a commit to rust-lang-ci/rust that referenced this issue Jul 29, 2021
…bank

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. ;)
@GuillaumeGomez
Copy link
Member Author

Fixed in #87285.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants