Skip to content

Cleanup markdown span handling - #80244

Merged
bors merged 3 commits into
rust-lang:masterfrom
jyn514:spans
Dec 21, 2020
Merged

Cleanup markdown span handling#80244
bors merged 3 commits into
rust-lang:masterfrom
jyn514:spans

Conversation

@jyn514

@jyn514jyn514 commented Dec 20, 2020•
edited
Loading

Copy link
Copy Markdown
Member
  1. Get rid of locate() in markdown handling

This function was unfortunate for several reasons:

  • It used unsafe because it wanted to tell whether a string came from
    the same allocation as another, not just whether it was a textual match.
  • It recalculated spans even though they were already available from pulldown
  • It sometimes failed to calculate the span, which meant it was always possible for the span to be None, even though in practice that should never happen.

This has several cleanups:

  • Make the span required
  • Pass through the span from pulldown in the HeadingLinks and Footnotes iterators
  • Only add iterator bounds on the impl Iterator, not on new and the struct itself.
  1. Remove unnecessary scope in markdown_links

I recommend reading a single commit at a time.

cc @bugadani - this will conflict with #77859, I'll try to make sure that gets merged first.

@jyn514jyn514 added C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Dec 20, 2020
@rust-highfive

This comment has been minimized.

Sign in to view
@rust-highfive

rust-highfive commented Dec 20, 2020

Copy link
Copy Markdown
Contributor

r? @ollie27

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

@rust-highfiverust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 20, 2020
@bugadani

bugadani commented Dec 20, 2020

Copy link
Copy Markdown
Contributor

I'd be fine with merging this first; the MarkdownLink refactor in #77859 could probably be done on top of this.

@jyn514jyn514 mentioned this pull request Dec 20, 2020
Merged
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@jyn514
Get rid of locate() in markdown handling
65f4f39
This function was unfortunate for several reasons:
- It used `unsafe` because it wanted to tell whether a string came from
the same *allocation* as another, not just whether it was a textual
match.
- It recalculated spans even though they were already available from
pulldown
- It sometimes *failed* to calculate the span, which meant it was always
possible for the span to be `None`, even though in practice that
should never happen.
This commit has several cleanups:
- Make the span required
- Pass through the span from pulldown in the `HeadingLinks` and
`Footnotes` iterators
- Only add iterator bounds on the `impl Iterator`, not on `new` and the
struct itself.
@jyn514
Remove unnecessary scope
1e15c2f
@jyn514

jyn514 commented Dec 20, 2020

Copy link
Copy Markdown
MemberAuthor

r? @bugadani

bugadani
bugadani reviewed Dec 20, 2020
View reviewed changes
Comment threadsrc/librustdoc/html/markdown.rs
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bugadani

bugadani commented Dec 20, 2020

Copy link
Copy Markdown
Contributor

@bors r+

Looks neat :)

@bors

bors commented Dec 20, 2020

Copy link
Copy Markdown
Collaborator

@bugadani: 🔑 Insufficient privileges: Not in reviewers

@bugadani

bugadani commented Dec 20, 2020

Copy link
Copy Markdown
Contributor

Hmm I expected bors to respect the review request. I guess I assumed too much :)

@jyn514

jyn514 commented Dec 20, 2020

Copy link
Copy Markdown
MemberAuthor

@bors r=bugadani

Thanks for the review!

@bors

bors commented Dec 20, 2020

Copy link
Copy Markdown
Collaborator

📌 Commit 60d5567 has been approved by bugadani

@borsbors 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 20, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Dec 21, 2020
@Dylan-DPC
Rollup merge ofrust-lang#80244- jyn514:spans, r=bugadani
d0bc5e4
Cleanup markdown span handling
1. Get rid of `locate()` in markdown handling
This function was unfortunate for several reasons:
- It used `unsafe` because it wanted to tell whether a string came from
the same *allocation* as another, not just whether it was a textual match.
- It recalculated spans even though they were already available from pulldown
- It sometimes *failed* to calculate the span, which meant it was always possible for the span to be `None`, even though in practice that should never happen.
This has several cleanups:
- Make the span required
- Pass through the span from pulldown in the `HeadingLinks` and `Footnotes` iterators
- Only add iterator bounds on the `impl Iterator`, not on `new` and the struct itself.
2. Remove unnecessary scope in `markdown_links`
I recommend reading a single commit at a time.
cc `@bugadani` - this will conflict with rust-lang#77859, I'll try to make sure that gets merged first.
This was referenced Dec 21, 2020
Closed
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 21, 2020
@bors
Auto merge ofrust-lang#80253- Dylan-DPC:rollup-bkmn74z, r=Dylan-DPC
15d1f81
Rollup of 11 pull requests
Successful merges:
- rust-lang#80159 (Add array search aliases)
- rust-lang#80166 (Edit rustc_middle docs)
- rust-lang#80170 (Fix ICE when lookup method in trait for type that have bound vars)
- rust-lang#80171 (Edit rustc_middle::ty::TyKind docs)
- rust-lang#80199 (also const-check FakeRead)
- rust-lang#80211 (Handle desugaring in impl trait bound suggestion)
- rust-lang#80236 (Use pointer type in AtomicPtr::swap implementation)
- rust-lang#80239 (Update Clippy)
- rust-lang#80240 (make sure installer only creates directories in DESTDIR)
- rust-lang#80244 (Cleanup markdown span handling)
- rust-lang#80250 (Minor cleanups in LateResolver)
Failed merges:
r? `@ghost`
`@rustbot` modify labels: rollup
@bors
bors merged commit 8232109 into rust-lang:masterDec 21, 2020
@rustbotrustbot added this to the 1.50.0 milestone Dec 21, 2020
@jyn514
jyn514 deleted the spans branch December 21, 2020 13:38
@jyn514jyn514 mentioned this pull request Dec 26, 2020
Merged
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Dec 30, 2020
@Dylan-DPC
Rollup merge ofrust-lang#80381- rust-lang:revert-80244-spans, r=Gui…
9b5cdd8
…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`
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Dec 30, 2020
@JohnTitor
Rollup merge ofrust-lang#80381- rust-lang:revert-80244-spans, r=Gui…
7494aef
…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`
@bugadanibugadani mentioned this pull request Dec 31, 2020
Merged
2 tasks
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 2, 2021
@bors
Auto merge ofrust-lang#80550- bugadani:markdown-refactor, r=jyn514
fd85ca0
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.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@bugadanibugadanibugadani approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@bugadanibugadani

Labels

C-cleanupCategory: PRs that clean code up or issues documenting cleanup.S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.T-rustdocRelevant to the rustdoc team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

1.50.0

Development

Successfully merging this pull request may close these issues.

7 participants

@jyn514@rust-highfive@bugadani@rust-log-analyzer@bors@ollie27@rustbot