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

Revert "Cleanup markdown span handling" #80381

Merged
merged 1 commit into from
Dec 30, 2020
Merged

Revert "Cleanup markdown span handling" #80381

merged 1 commit into from
Dec 30, 2020

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Dec 26, 2020

Reverts #80244. This caused a diagnostic regression, originally it was:

warning: unresolved link to `std::process::Comman`
 --> link.rs:3:10
  |
3 | //! [a]: std::process::Comman
  |          ^^^^^^^^^^^^^^^^^^^^ no item named `Comman` in module `process`
  |
  = note: `#[warn(broken_intra_doc_links)]` on by default

but after that PR rustdoc now displays

warning: unresolved link to `std::process::Comman`
 --> link.rs:1:14
  |
1 | //! Links to [a] [link][a]
  |              ^^^ no item named `Comman` in module `process`
  |
  = note: `#[warn(broken_intra_doc_links)]` on by default

which IMO is much less clear.

cc @bugadani, thanks for catching this in #77859.
r? @GuillaumeGomez

@jyn514 jyn514 added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. A-diagnostics Area: Messages for errors, warnings, and lints labels Dec 26, 2020
@rust-highfive
Copy link
Collaborator

Some changes occurred in intra-doc-links.

cc @jyn514

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 26, 2020
@GuillaumeGomez
Copy link
Member

Could you also add a UI test to prevent a future regression too please?

This caused a diagnostic regression, originally it was:

```
warning: unresolved link to `std::process::Comman`
 --> link.rs:3:10
  |
3 | //! [a]: std::process::Comman
  |          ^^^^^^^^^^^^^^^^^^^^ no item named `Comman` in module `process`
  |
  = note: `#[warn(broken_intra_doc_links)]` on by default
```
but after that PR rustdoc now displays
```
warning: unresolved link to `std::process::Comman`
 --> link.rs:1:14
  |
1 | //! Links to [a] [link][a]
  |              ^^^ no item named `Comman` in module `process`
  |
  = note: `#[warn(broken_intra_doc_links)]` on by default
```
which IMO is much less clear.
@jyn514 jyn514 force-pushed the revert-80244-spans branch from 0403b28 to 0f25712 Compare December 26, 2020 17:12
@jyn514
Copy link
Member Author

jyn514 commented Dec 26, 2020

@GuillaumeGomez done, good idea.

@GuillaumeGomez
Copy link
Member

Thanks! A bit sad for this change to be reverted but I guess it's going back to better jump. 😉 r=me once CI pass

@jyn514
Copy link
Member Author

jyn514 commented Dec 30, 2020

@bors r=GuillaumeGomez

@bors
Copy link
Contributor

bors commented Dec 30, 2020

📌 Commit 0f25712 has been approved by GuillaumeGomez

@bors 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 Dec 30, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Dec 30, 2020
…llaumeGomez

Revert "Cleanup markdown span handling"

Reverts rust-lang#80244. This caused a diagnostic regression, originally it was:

```
warning: unresolved link to `std::process::Comman`
 --> link.rs:3:10
  |
3 | //! [a]: std::process::Comman
  |          ^^^^^^^^^^^^^^^^^^^^ no item named `Comman` in module `process`
  |
  = note: `#[warn(broken_intra_doc_links)]` on by default
```
but after that PR rustdoc now displays
```
warning: unresolved link to `std::process::Comman`
 --> link.rs:1:14
  |
1 | //! Links to [a] [link][a]
  |              ^^^ no item named `Comman` in module `process`
  |
  = note: `#[warn(broken_intra_doc_links)]` on by default
```
which IMO is much less clear.

cc `@bugadani,` thanks for catching this in rust-lang#77859.
r? `@GuillaumeGomez`
@bugadani
Copy link
Contributor

bugadani commented Dec 30, 2020

Some notes:

  • CowStr is Borrowed only as long as the original link destination doesn't have to be transformed. This covers most of the intra-doc links, except for anchors that contain e.g. html entities.
  • the original locate can handle the Borrowed case well enough, even though it's unsafe and ugly. But, since the borrowed string is part of the original source, it can be simplified. I suspect we can devise test cases where even the original code provides error messages pointing to the place of use instead of the definition. I'm not sure we can work around that, though, short of manually extracting the link label and looking up the definition in the md source.
  • pulldown_cmark doesn't provide any information about reference-style link definitions. This is unfortunate, especially since Footnote defs are otherwise emitted as events.
  • The only case where we have to care about all this (meaning the reason for reverting the original PR, and the points in this comment) is shortcut links and reference-style links, e.g. where the link definition is separate from the place of use.

bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 30, 2020
Rollup of 7 pull requests

Successful merges:

 - rust-lang#80185 (Fix ICE when pointing at multi bytes character)
 - rust-lang#80260 (slightly more typed interface to panic implementation)
 - rust-lang#80311 (Improvements to NatVis support)
 - rust-lang#80337 (Use `desc` as a doc-comment for queries if there are no doc comments)
 - rust-lang#80381 (Revert "Cleanup markdown span handling")
 - rust-lang#80492 (remove empty wraps, don't return Results from from infallible functions)
 - rust-lang#80509 (where possible, pass slices instead of &Vec or &String (clippy::ptr_arg))

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 7494aef into master Dec 30, 2020
@bors bors deleted the revert-80244-spans branch December 30, 2020 18:24
@rustbot rustbot added this to the 1.51.0 milestone Dec 30, 2020
@jyn514 jyn514 added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Dec 31, 2020
@jyn514
Copy link
Member Author

jyn514 commented Dec 31, 2020

This fixes a diagnostic regression that was introduced in 1.50, but the fix was only merged in 1.51.

@rust-lang/rustdoc how do you feel about backporting this? The change is pretty small and only reverts an earlier change.

@GuillaumeGomez
Copy link
Member

I agree with the backport.

bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 2, 2021
Cleanup markdown span handling, take 2

This PR includes the cleanups made in rust-lang#80244 except for the removal of `locate()`.

While the biggest conceptual part in rust-lang#80244 was the removal of `locate()`, it introduced a diagnostic regression.

Additional cleanup:
 - Use `RefCell` to avoid building two separate vectors for the links

Work to do:
- [ ] Decide if `locate()` can be simplified by assuming `s` is always in `md`
- [ ] Should probably add some tests that still provide the undesired diagnostics causing rust-lang#80381

cc `@jyn514` This is the best I can do without patching Pulldown to provide multiple ranges for reference-style links. Also, since `locate` is probably more efficient than `rfind` (at least it's constant time), I decided to not check the link type and just cover every &str as it was before.
@apiraino
Copy link
Contributor

apiraino commented Jan 7, 2021

beta backport discussed and approved during today's meeting

@apiraino apiraino added beta-accepted Accepted for backporting to the compiler in the beta channel. and removed beta-nominated Nominated for backporting to the compiler in the beta channel. labels Jan 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints beta-accepted Accepted for backporting to the compiler in the beta channel. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants