Skip to content

Remove incorrect assert #79976

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

Merged
merged 1 commit into from
Dec 13, 2020
Merged

Remove incorrect assert #79976

merged 1 commit into from
Dec 13, 2020

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Dec 12, 2020

Fixes an assertion failure when resolving ::std (or any other crate that uses the :: style, see https://github.com/rust-lang/rust/pull/79809/files#r541776478, https://rust-lang.zulipchat.com/#narrow/stream/266220-rustdoc/topic/Perf.20failing).

Unblocks rust-lang/rustc-perf#806.

r? @ghost - this is a trivial fix and breaking rustc-perf so I'm going to approve it unilaterally. cc @Mark-Simulacrum @Eric-Arellano

@jyn514 jyn514 added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name labels Dec 12, 2020
@jyn514
Copy link
Member Author

jyn514 commented Dec 12, 2020

@bors r+ p=1

This is breaking rustc-perf.

@bors
Copy link
Collaborator

bors commented Dec 12, 2020

📌 Commit 130dbe4 has been approved by jyn514

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Dec 12, 2020
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Thanks for the hotfix!

@@ -436,8 +436,8 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
// Try looking for methods and associated items.
let mut split = path_str.rsplitn(2, "::");
// NB: `split`'s first element is always defined, even if the delimiter was not present.
// NB: `item_str` could be empty when resolving in the root namespace (e.g. `::std`).
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh wait, I'm confused. Note that rsplit returns a reverse iterator. In this example, I think the first element would be std, right? It would not be empty.

Copy link
Member Author

Choose a reason for hiding this comment

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

Here's the debug output from the assertion failure on hyper:

TRACE rustdoc::passes::collect_intra_doc_links considering link '::client'
DEBUG rustdoc::passes::collect_intra_doc_links resolving ::client as a macro in the module DefId(0:248 ~ hyper[8787]::body)
DEBUG rustdoc::passes::collect_intra_doc_links ::client resolved to Err(()) in namespace TypeNS
DEBUG rustdoc::passes::collect_intra_doc_links  resolved to Err(()) in namespace TypeNS
DEBUG rustdoc::passes::collect_intra_doc_links ::client resolved to Err(()) in namespace ValueNS
DEBUG rustdoc::passes::collect_intra_doc_links  resolved to Err(()) in namespace TypeNS
DEBUG rustdoc::passes::collect_intra_doc_links looking for enum variant ::client
DEBUG rustdoc::passes::collect_intra_doc_links  resolved to Err(()) in namespace TypeNS
thread 'rustc' panicked at 'assertion failed: !item_str.is_empty()', src/librustdoc/passes/collect_intra_doc_links.rs:439:9

So I think what's going wrong is that resolve_variant calls resolve recursively, and on the second call the string is empty (and only has one item in the iterator). This could probably be improved but it would require a redesign of the algorithm.

Copy link
Member Author

@jyn514 jyn514 Dec 12, 2020

Choose a reason for hiding this comment

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

Yup, this is the backtrace:

   0: std::panicking::begin_panic
             at /home/joshua/rustc/library/std/src/panicking.rs:519:12
   1: rustdoc::passes::collect_intra_doc_links::LinkCollector::resolve
             at /home/joshua/rustc/src/librustdoc/passes/collect_intra_doc_links.rs:439:9
   2: rustdoc::passes::collect_intra_doc_links::LinkCollector::check_full_res
             at /home/joshua/rustc/src/librustdoc/passes/collect_intra_doc_links.rs:637:17
   3: rustdoc::passes::collect_intra_doc_links::resolution_failure::{{closure}}
             at /home/joshua/rustc/src/librustdoc/passes/collect_intra_doc_links.rs:1680:33
   4: rustdoc::passes::collect_intra_doc_links::report_diagnostic::{{closure}}
             at /home/joshua/rustc/src/librustdoc/passes/collect_intra_doc_links.rs:1597:9
   5: core::ops::function::FnOnce::call_once{{vtable.shim}}
             at /home/joshua/rustc/library/core/src/ops/function.rs:227:5
   6: <alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once
             at /home/joshua/rustc/library/alloc/src/boxed.rs:1328:9
   7: rustc_middle::lint::struct_lint_level::struct_lint_level_impl
             at /home/joshua/rustc/compiler/rustc_middle/src/lint.rs:362:9
   8: rustc_middle::lint::struct_lint_level
             at /home/joshua/rustc/compiler/rustc_middle/src/lint.rs:364:5
   9: rustc_middle::ty::context::TyCtxt::struct_span_lint_hir
             at /home/joshua/rustc/compiler/rustc_middle/src/ty/context.rs:2562:9
  10: rustdoc::passes::collect_intra_doc_links::report_diagnostic
             at /home/joshua/rustc/src/librustdoc/passes/collect_intra_doc_links.rs:1567:5
  11: rustdoc::passes::collect_intra_doc_links::resolution_failure
             at /home/joshua/rustc/src/librustdoc/passes/collect_intra_doc_links.rs:1617:5
  12: rustdoc::passes::collect_intra_doc_links::LinkCollector::resolve_with_disambiguator
             at /home/joshua/rustc/src/librustdoc/passes/collect_intra_doc_links.rs:1299:21
  13: rustdoc::passes::collect_intra_doc_links::LinkCollector::resolve_link
             at /home/joshua/rustc/src/librustdoc/passes/collect_intra_doc_links.rs:1082:39
  14: <rustdoc::passes::collect_intra_doc_links::LinkCollector as rustdoc::fold::DocFolder>::fold_item
             at /home/joshua/rustc/src/librustdoc/passes/collect_intra_doc_links.rs:909:28

Notice this is in diagnostics, not the main resole code.

@bors
Copy link
Collaborator

bors commented Dec 12, 2020

⌛ Testing commit 130dbe4 with merge f61e5ca...

@bors
Copy link
Collaborator

bors commented Dec 13, 2020

☀️ Test successful - checks-actions
Approved by: jyn514
Pushing f61e5ca to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 13, 2020
@bors bors merged commit f61e5ca into rust-lang:master Dec 13, 2020
@rustbot rustbot added this to the 1.50.0 milestone Dec 13, 2020
@jyn514 jyn514 deleted the assertion-failure branch December 13, 2020 03:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants