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

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

Merged
merged 1 commit into from
Aug 14, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 7 additions & 16 deletions src/librustdoc/clean/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -415,29 +415,28 @@ impl Item {
.unwrap_or(false)
}

pub(crate) fn span(&self, tcx: TyCtxt<'_>) -> Span {
pub(crate) fn span(&self, tcx: TyCtxt<'_>) -> Option<Span> {
let kind = match &*self.kind {
ItemKind::StrippedItem(k) => k,
_ => &*self.kind,
};
match kind {
ItemKind::ModuleItem(Module { span, .. }) => *span,
ItemKind::ImplItem(box Impl { kind: ImplKind::Auto, .. }) => Span::dummy(),
ItemKind::ModuleItem(Module { span, .. }) => Some(*span),
ItemKind::ImplItem(box Impl { kind: ImplKind::Auto, .. }) => None,
ItemKind::ImplItem(box Impl { kind: ImplKind::Blanket(_), .. }) => {
if let ItemId::Blanket { impl_id, .. } = self.item_id {
rustc_span(impl_id, tcx)
Some(rustc_span(impl_id, tcx))
} else {
panic!("blanket impl item has non-blanket ID")
}
}
_ => {
self.item_id.as_def_id().map(|did| rustc_span(did, tcx)).unwrap_or_else(Span::dummy)
}
_ => self.item_id.as_def_id().map(|did| rustc_span(did, tcx)),
}
}

pub(crate) fn attr_span(&self, tcx: TyCtxt<'_>) -> rustc_span::Span {
crate::passes::span_of_attrs(&self.attrs).unwrap_or_else(|| self.span(tcx).inner())
crate::passes::span_of_attrs(&self.attrs)
.unwrap_or_else(|| self.span(tcx).map_or(rustc_span::DUMMY_SP, |span| span.inner()))
}

/// Finds the `doc` attribute as a NameValue and returns the corresponding
Expand Down Expand Up @@ -2109,14 +2108,6 @@ impl Span {
self.0
}

pub(crate) fn dummy() -> Self {
Self(rustc_span::DUMMY_SP)
}

pub(crate) fn is_dummy(&self) -> bool {
self.0.is_dummy()
}

pub(crate) fn filename(&self, sess: &Session) -> FileName {
sess.source_map().span_to_filename(self.0)
}
Expand Down
5 changes: 1 addition & 4 deletions src/librustdoc/html/render/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -301,13 +301,10 @@ impl<'tcx> Context<'tcx> {
/// may happen, for example, with externally inlined items where the source
/// of their crate documentation isn't known.
pub(super) fn src_href(&self, item: &clean::Item) -> Option<String> {
self.href_from_span(item.span(self.tcx()), true)
self.href_from_span(item.span(self.tcx())?, true)
}

pub(crate) fn href_from_span(&self, span: clean::Span, with_lines: bool) -> Option<String> {
if span.is_dummy() {
return None;
}
Comment on lines -308 to -310
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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... 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Proc macros can generate items without spans, I think.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Member

@jyn514 jyn514 Aug 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I can do that

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

let mut root = self.root_path();
let mut path = String::new();
let cnum = span.cnum(self.sess());
Expand Down
2 changes: 1 addition & 1 deletion src/librustdoc/html/render/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2677,7 +2677,7 @@ fn render_call_locations(w: &mut Buffer, cx: &mut Context<'_>, item: &clean::Ite
let contents = match fs::read_to_string(&path) {
Ok(contents) => contents,
Err(err) => {
let span = item.span(tcx).inner();
let span = item.span(tcx).map_or(rustc_span::DUMMY_SP, |span| span.inner());
tcx.sess
.span_err(span, &format!("failed to read file {}: {}", path.display(), err));
return false;
Expand Down
2 changes: 2 additions & 0 deletions src/librustdoc/html/sources.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ impl LocalSourcesCollector<'_, '_> {
fn add_local_source(&mut self, item: &clean::Item) {
let sess = self.tcx.sess;
let span = item.span(self.tcx);
let Some(span) = span else { return };
// skip all synthetic "files"
if !is_real_and_local(span, sess) {
return;
Expand Down Expand Up @@ -109,6 +110,7 @@ impl DocVisitor for SourceCollector<'_, '_> {

let tcx = self.cx.tcx();
let span = item.span(tcx);
let Some(span) = span else { return };
let sess = tcx.sess;

// If we're not rendering sources, there's nothing to do.
Expand Down
2 changes: 1 addition & 1 deletion src/librustdoc/json/conversions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ impl JsonRenderer<'_> {
id: from_item_id_with_name(item_id, self.tcx, name),
crate_id: item_id.krate().as_u32(),
name: name.map(|sym| sym.to_string()),
span: self.convert_span(span),
span: span.and_then(|span| self.convert_span(span)),
visibility: self.convert_visibility(visibility),
docs,
attrs,
Expand Down
18 changes: 10 additions & 8 deletions src/librustdoc/passes/calculate_doc_coverage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,6 @@ impl<'a, 'b> DocVisitor for CoverageCalculator<'a, 'b> {
None,
);

let filename = i.span(self.ctx.tcx).filename(self.ctx.sess());
let has_doc_example = tests.found_tests != 0;
// The `expect_def_id()` should be okay because `local_def_id_to_hir_id`
// would presumably panic if a fake `DefIndex` were passed.
Expand Down Expand Up @@ -261,13 +260,16 @@ impl<'a, 'b> DocVisitor for CoverageCalculator<'a, 'b> {
let should_have_docs = !should_be_ignored
&& (level != lint::Level::Allow || matches!(source, LintLevelSource::Default));

debug!("counting {:?} {:?} in {:?}", i.type_(), i.name, filename);
self.items.entry(filename).or_default().count_item(
has_docs,
has_doc_example,
should_have_doc_example(self.ctx, i),
should_have_docs,
);
if let Some(span) = i.span(self.ctx.tcx) {
let filename = span.filename(self.ctx.sess());
debug!("counting {:?} {:?} in {:?}", i.type_(), i.name, filename);
self.items.entry(filename).or_default().count_item(
has_docs,
has_doc_example,
should_have_doc_example(self.ctx, i),
should_have_docs,
);
}
}
}

Expand Down
18 changes: 18 additions & 0 deletions src/test/rustdoc-json/impls/auto.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
#![feature(no_core, auto_traits, lang_items)]
#![no_core]

#[lang = "sized"]
trait Sized {}

pub auto trait Bar {}

/// has span
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=='has span')].span.begin" "[10, 0]"
// @is - "$.index[*][?(@.docs=='has span')].span.end" "[12, 1]"
pub struct Foo;