make `clean::Item::span` return `Option` instead of dummy span by compiler-errors · Pull Request #100299 · rust-lang/rust

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

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

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


		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]"

Uh oh!

Conversation

compiler-errors commented Aug 9, 2022

Uh oh!

rustbot commented Aug 9, 2022

Uh oh!

rust-highfive commented Aug 9, 2022

Uh oh!

compiler-errors commented Aug 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jyn514 Aug 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

compiler-errors commented Aug 9, 2022

Uh oh!

aDotInTheVoid commented Aug 9, 2022

Uh oh!

bors commented Aug 12, 2022

Uh oh!

notriddle commented Aug 12, 2022

Uh oh!

bors commented Aug 12, 2022

Uh oh!

Dylan-DPC commented Aug 12, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

compiler-errors commented Aug 13, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

notriddle commented Aug 13, 2022

Uh oh!

bors commented Aug 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

compiler-errors commented Aug 9, 2022 •

edited

Loading

jyn514 Aug 9, 2022 •

edited

Loading