rustdoc: do not include extra stuff in span

[notriddle]

View all comments

This change prevents our lints from returning a span with stuff in it that isn't actually part of the doc comment. When that happens, it returns None instead.

Fixes rust-lang/rust-clippy#16169, since the bug was reported against the clippy::doc_paragraphs_missing_punctuation lint but is actually a bug in source_span_for_markdown_range_inner.

[rustbot]

r? @lolbinarycat

rustbot has assigned @lolbinarycat.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: rustdoc
• rustdoc expanded to 9 candidates
• Random selection from GuillaumeGomez, camelid, lolbinarycat

[notriddle]

p.s. this change is a lot easier to understand if you use the hide whitespace setting on the diff.

[rustbot]

The Clippy subtree was changed

cc @rust-lang/clippy

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[lolbinarycat]

Do we need to be optimizing our diagnostic code (cold path) to the extent of using binary search to find an item in a usually small list? this only speeds up doc generation if a crate has a ton of warnings. I guess I'll see if it shows up at all in a perf run.

@bors try @rust-timer queue profiles=doc

[rust-bors]

☀️ Try build successful (CI)
Build commit: a64b630 (a64b630f04e7c6ae84c88850e738cae44ce235d7, parent: 65407954098ca3c19f0d46092cb374b5d3e9dc3c)

[notriddle]

Do we need to be optimizing our diagnostic code (cold path) to the extent of using binary search to find an item in a usually small list?

I knew it would be O(n²) if just did a linear scan. Even in the cold path, I'd prefer to avoid that.

[lolbinarycat]

I don't think algorimic complexity is really relevant here.

[notriddle]

@bors try @rust-timer queue profiles=doc

[rust-bors]

☀️ Try build successful (CI)
Build commit: 1a5fffe (1a5fffe9955796b4c20810d806e19fca4d8f6e34, parent: e7ef22d23f10e9e05546592aa9c1e3e8bb288969)

[rust-timer]

Finished benchmarking commit (1a5fffe): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This perf run didn't have relevant results for this metric.

Max RSS (memory usage)

This perf run didn't have relevant results for this metric.

Cycles

This perf run didn't have relevant results for this metric.

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 521.088s -> 525.76s (0.90%)
Artifact size: 401.44 MiB -> 401.46 MiB (0.00%)

[notriddle]

Here's the new non-relevant result set:

[lolbinarycat]

Code looks solid, just have some recommendations about how code readablity could potentially be improved. I don't feel particularly strongly about the comment rewrites, so feel free to just ignore those.

View changes since this review

[lolbinarycat]

Shouldn't we produce a warning, but not a suggestion? fine with addressing this in a followup PR.

[lolbinarycat]

Suggested change

	let span = span_of_fragments(fragments)?;
	let span_of_all_fragments = span_of_fragments(fragments)?;

[lolbinarycat]

Suggested change

	let mut line_bytes = 0;
	let mut prev_lines_bytes = 0;

[lolbinarycat]

Suggested change

	let has_fragment = fragments.iter().any(|fragment| {
	fragment.span.hi().0 >= span.lo().0 + line_bytes
	&& fragment.span.lo().0 <= span.lo().0 + line_bytes + source_line_len
	});
	let source_line_span = span_of_all_fragments.split_at(prev_line_bytes).1.split_at(source_line_len).0;
	let has_fragment = fragments.iter().any(|fragment| {
	fragment.span.contains(source_line_span)
	});

I think an intermediate span or at least some intermediate variables would make this more readable (unfortunately the functions for manipulating spans are a bit lacking in utilities that would be more intuitive to read).

[notriddle]

This doesn't work as-is, because span_of_fragment contains the intermediate indentation and fragment.span doesn't.

pub struct Foo {
    /// doc comment for struct property
    /// intermediate line
    /// another intermediate line
    pub property: Bar,
}

Consider that source_line_span and fragment.span for the middle line won't be the same:

    /// intermediate line
-------------------------
|   ^^^^^^^^^^^^^^^^^^^^^
|   | fragment.span
|
| source_line_span

[lolbinarycat]

doesn't that also mean your code is also wrong? unless i messed up and changed behavior in my refactor

it should be source_line_span.contains(fragment.span())

this whole discussion makes me think we need more test coverage.

[notriddle]

My code was actually just really weird and redundant. I thought it was equivalent to fragment.span.contains(source_line_span), but lucked into a version that actually worked instead.

The new version is just fragment.span.contains(source_line_span.shrink_to_hi()), which should be easier to understand.

[lolbinarycat]

that also seems strange. why not Span::overlaps?

[lolbinarycat]

Suggested change

	// Since this is a source line that doesn't include a markdown line,
	// we have to count the newline that we split from earlier.
	// Since this is a source line that doesn't include a markdown line,
	// we have to count it and its newline as non-markdown bytes

[lolbinarycat]

Suggested change

	} else if source_line.chars().any(|c| !c.is_whitespace()) {
	// If we're past the first line, but haven't found the last line,
	// we can only return a contiguous span if every line is either
	// part of the doc comment or blank.
	return None;
	} else if source_line.chars().any(|c| !c.is_whitespace()) {
	// We're past the first line, but haven't found the last line,
	// but we found a non-empty non-markdown line.
	// This could be an attribute, and we don't want a diagnostic
	// suggesting to delete that attribute, so we return None to be safe.
	return None;

[notriddle]

@rustbot review

Okay, I've pushed a bunch of commits that should resolve all of these requests.

[lolbinarycat]

lines with doc comments can have trailing whitespace that isn't part of the doc comment, though.

there's nothing asymmetrical about this problem as far as i can tell, so i don't see why the solution would be asymmetrical

consider:

/** Here's some function docs with an unusual style */    // whoops some trailing whitespace here!
pub fn f()  { ... }

or even:

/** Everything on one line */ pub fn g() { ... }

i think overlaps is the correct thing to use, or if that's incorrect, then we need something more nuanced than shrink_to_hi.

View changes since the review

[notriddle]

lines with doc comments can have trailing whitespace that isn't part of the doc comment, though.

Neither of the snippets you gave are examples of this, because source_line_span is created by splitting up span_of_all_fragments, which doesn't include anything after the last block comment.

1| /** some doc */ fn foo() {}
   ^^^^^^^^^^^^^^^
   | span_of_all_fragments doesn't include the `fn`,
     so neither does `source_line_span`

You can wind up with an attribute that's included in the span_of_all_fragments without it being in the fragment.span, by putting two block comments back-to-back:

/** [Clone]( */ #[inline]
/** std::clone::Clone) */
pub fn split_blockcomment_twoline_attr() {
}

Or do the same thing with an attribute at the start of the second line:

/** [Clone]( */
#[inline] /** std::clone::Clone) */
pub fn split_blockcomment_twoline_attr() {
}

I've pushed a new commit with the change to overlaps, a bug fix for these two cases, and a bunch of new tests.

[rust-bors]

☔ The latest upstream changes (presumably #158403) made this pull request unmergeable. Please resolve the merge conflicts by rebasing.