make `clean::Item::span` return `Option` instead of dummy span #100299

compiler-errors · 2022-08-09T02:49:27Z

Fixes #100283

rustbot · 2022-08-09T02:49:30Z

Some changes occurred in src/librustdoc/clean/types.rs

cc @camelid

rust-highfive · 2022-08-09T02:49:30Z

r? @notriddle

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

compiler-errors · 2022-08-09T02:51:13Z

This PR makes a half-spirited attempt to handle the other places where we were using clean::Span to now gracefully use Option<clean::Span>, but I'm open to feedback for any of those other usages.

Alternative approach to this would be to handle this fully in the rustdoc-json side, and not touch clean::Span

camelid · 2022-08-09T03:52:57Z

src/librustdoc/html/render/context.rs

-        if span.is_dummy() {
-            return None;
-        }


I wonder if there's any points at which we could get dummy spans from rustc itself?

Not sure -- maybe from a really strange macro expansion?

My guess was that this was originally placed to deal with the auto-trait issue I am fixing here more systematically, but maybe it also addresses other sources of DUMMY_SP that I didn't consider... 🤔

Proc macros can generate items without spans, I think.

There's no such thing as "without span" I think? -- proc macros have a few choices of spans to associate with tokens, but I can't tell how you'd get a DUMMY_SP out of a proc macro except during error recovery?

TokenStream doesn't actually require a span though, right? https://doc.rust-lang.org/proc_macro/struct.TokenStream.html#method.new
Only Ident requires a span. https://doc.rust-lang.org/proc_macro/enum.TokenTree.html

@jyn514 sure, but TokenTrees have spans (and presumably need them to be created), and anyways you can't create a rustdoc item with an empty TokenStream, right?

Hmm, I guess so. I'm just really wary of changes here - if you're confident dummy spans are impossible I'd like to make that an assert instead of completely removing the code.

Yeah I can do that

Actually, this failed in rollup because we tried to call rustdoc on an empty file in a cargo unit test.

Specifically, if the file contents are empty, then the span of the root module of the crate is gonna be a span that is exactly equal to DUMMY_SP (because it goes from 0 to 0). So when we try to generate an href to the module sources, we trigger an assertion failure.

I think we should just remove this assertion I added. That is, if a DUMMY_SP (or, in this case, a span that looks just like one) comes from rustc, we should be not doing anything special to it. I think this will generate a src link that points to the empty file, but I feel like that's the simpler and easier to understand behavior.

cc @camelid @jyn514 thoughts?

I really wish we had a way to collect metrics for rustdoc :(

but yeah removing the assertion makes sense to me, if someone reports a bug we can fix it

compiler-errors · 2022-08-09T08:38:05Z

@rustbot author

aDotInTheVoid · 2022-08-09T17:48:00Z

@rustbot modify labels: +A-rustdoc-json

bors · 2022-08-12T01:03:17Z

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

notriddle · 2022-08-12T03:33:05Z

@bors r+

bors · 2022-08-12T03:33:07Z

📌 Commit f5765ae has been approved by notriddle

It is now in the queue for this repository.

…triddle make `clean::Item::span` return `Option` instead of dummy span Fixes rust-lang#100283

Dylan-DPC · 2022-08-12T15:07:40Z

failed in #100449 (comment)

@bors r-

aDotInTheVoid · 2022-08-12T19:52:29Z

src/test/rustdoc-json/impls/auto.rs

+
+pub auto trait Bar {}
+
+// @has auto.json "$.index[*][?(@.kind=='impl')].span" null


This test isn't that specific, as it only checks that their is an impl with an null span. A better one would check that the synthetic impl has a null span, and a real one has the correct span.

(Also, I need to document how the json testing infra works)

Suggested change

// @has auto.json "$.index[*][?(@.kind=='impl')].span" null

pub struct Foo;

/// hack

impl Foo {

pub fn baz(&self) {}

}

// Testing spans, so all tests below code

// @is auto.json "$.index[*][?(@.kind=='impl' && @.inner.synthetic==true)].span" null

// @is - "$.index[*][?(@.docs=='hack')].span.begin" "[12, 0]"

// @is - "$.index[*][?(@.docs=='hack')].span.end" "[14, 1]"

compiler-errors · 2022-08-13T00:27:10Z

Updated the JSON test. Thanks @aDotInTheVoid.

@rustbot ready

notriddle · 2022-08-13T21:49:24Z

src/librustdoc/html/render/context.rs

-        if span.is_dummy() {
-            return None;
-        }
+        assert!(!span.inner().is_dummy());


We still want to remove this assertion. @compiler-errors

Whooops -- It's me, I am the dummy.

notriddle · 2022-08-13T22:58:17Z

@bors r+ rollup

bors · 2022-08-13T22:58:19Z

📌 Commit 752b0e0 has been approved by notriddle

It is now in the queue for this repository.

…mpiler-errors Rollup of 8 pull requests Successful merges: - rust-lang#99646 (Only point out a single function parameter if we have a single arg incompatibility) - rust-lang#100299 (make `clean::Item::span` return `Option` instead of dummy span) - rust-lang#100335 (Rustdoc-Json: Add `Path` type for traits.) - rust-lang#100367 (Suggest the path separator when a dot is used on a trait) - rust-lang#100431 (Enum variant ctor inherits the stability of the enum variant) - rust-lang#100446 (Suggest removing a semicolon after impl/trait items) - rust-lang#100468 (Use an extensionless `x` script for non-Windows) - rust-lang#100479 (Argument type error improvements) Failed merges: - rust-lang#100483 (Point to generic or arg if it's the self type of unsatisfied projection predicate) r? `@ghost` `@rustbot` modify labels: rollup

rust-highfive assigned notriddle Aug 9, 2022

rustbot added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Aug 9, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 9, 2022

camelid reviewed Aug 9, 2022

View reviewed changes

rustbot added the A-rustdoc-json Area: Rustdoc JSON backend label Aug 9, 2022

compiler-errors force-pushed the issue-100283 branch from 96edb41 to f5765ae Compare August 12, 2022 02:53

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 Aug 12, 2022

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Aug 12, 2022

Rollup merge of rust-lang#100299 - compiler-errors:issue-100283, r=no…

2b745ee

…triddle make `clean::Item::span` return `Option` instead of dummy span Fixes rust-lang#100283

Dylan-DPC mentioned this pull request Aug 12, 2022

Rollup of 6 pull requests #100440

Closed

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Aug 12, 2022

Rollup merge of rust-lang#100299 - compiler-errors:issue-100283, r=no…

f3e083a

…triddle make `clean::Item::span` return `Option` instead of dummy span Fixes rust-lang#100283

Dylan-DPC mentioned this pull request Aug 12, 2022

Rollup of 7 pull requests #100449

Closed

Dylan-DPC mentioned this pull request Aug 12, 2022

'assertion failed: !span.inner().is_dummy()', #100454

Closed

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 12, 2022

aDotInTheVoid reviewed Aug 12, 2022

View reviewed changes

compiler-errors force-pushed the issue-100283 branch from f5765ae to 8b1f0ad Compare August 13, 2022 00:03

rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 13, 2022

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 13, 2022

notriddle reviewed Aug 13, 2022

View reviewed changes

make clean::Item::span return option instead of dummy span

752b0e0

compiler-errors force-pushed the issue-100283 branch from 8b1f0ad to 752b0e0 Compare August 13, 2022 22:04

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 Aug 13, 2022

compiler-errors mentioned this pull request Aug 14, 2022

Rollup of 8 pull requests #100516

Merged

bors merged commit d496c4e into rust-lang:master Aug 14, 2022

rustbot added this to the 1.65.0 milestone Aug 14, 2022

compiler-errors deleted the issue-100283 branch August 11, 2023 20:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make `clean::Item::span` return `Option` instead of dummy span #100299

make `clean::Item::span` return `Option` instead of dummy span #100299

compiler-errors commented Aug 9, 2022

rustbot commented Aug 9, 2022

rust-highfive commented Aug 9, 2022

compiler-errors commented Aug 9, 2022 •

edited

Loading

camelid Aug 9, 2022

compiler-errors Aug 9, 2022

jyn514 Aug 9, 2022

compiler-errors Aug 9, 2022

jyn514 Aug 9, 2022

compiler-errors Aug 9, 2022

jyn514 Aug 9, 2022 •

edited

Loading

compiler-errors Aug 9, 2022

compiler-errors Aug 12, 2022

jyn514 Aug 12, 2022

compiler-errors commented Aug 9, 2022

aDotInTheVoid commented Aug 9, 2022

bors commented Aug 12, 2022

notriddle commented Aug 12, 2022

bors commented Aug 12, 2022

Dylan-DPC commented Aug 12, 2022

aDotInTheVoid Aug 12, 2022

compiler-errors commented Aug 13, 2022

notriddle Aug 13, 2022

compiler-errors Aug 13, 2022

compiler-errors Aug 13, 2022

notriddle commented Aug 13, 2022

bors commented Aug 13, 2022


		pub auto trait Bar {}

		// @has auto.json "$.index[*][?(@.kind=='impl')].span" null

-// @has auto.json "$.index[*][?(@.kind=='impl')].span" null
+pub struct Foo;
+/// hack
+impl Foo {
+    pub fn baz(&self) {}
+}
+// Testing spans, so all tests below code
+// @is auto.json "$.index[*][?(@.kind=='impl' && @.inner.synthetic==true)].span" null
+// @is - "$.index[*][?(@.docs=='hack')].span.begin" "[12, 0]"
+// @is - "$.index[*][?(@.docs=='hack')].span.end" "[14, 1]"

make clean::Item::span return Option instead of dummy span #100299

make clean::Item::span return Option instead of dummy span #100299

Conversation

compiler-errors commented Aug 9, 2022

rustbot commented Aug 9, 2022

rust-highfive commented Aug 9, 2022

compiler-errors commented Aug 9, 2022 • edited Loading

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

jyn514 Aug 9, 2022 • edited Loading

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

compiler-errors commented Aug 9, 2022

aDotInTheVoid commented Aug 9, 2022

bors commented Aug 12, 2022

notriddle commented Aug 12, 2022

bors commented Aug 12, 2022

Dylan-DPC commented Aug 12, 2022

Choose a reason for hiding this comment

compiler-errors commented Aug 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

notriddle commented Aug 13, 2022

bors commented Aug 13, 2022

make `clean::Item::span` return `Option` instead of dummy span #100299

make `clean::Item::span` return `Option` instead of dummy span #100299

compiler-errors commented Aug 9, 2022 •

edited

Loading

jyn514 Aug 9, 2022 •

edited

Loading