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

rustdoc only: duplicate lang item panic_halt #68427

Closed
TimNN opened this issue Jan 21, 2020 · 19 comments · Fixed by #83738 or #88215
Closed

rustdoc only: duplicate lang item panic_halt #68427

TimNN opened this issue Jan 21, 2020 · 19 comments · Fixed by #83738 or #88215
Labels
A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name A-lang-item Area: Language items A-resolve Area: Name resolution C-bug Category: This is a bug. regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@TimNN
Copy link
Contributor

TimNN commented Jan 21, 2020

This is a regression from rustdoc 1.40.0 (73528e339 2019-12-16) to rustdoc 1.41.0-beta.3 (fb3056216 2020-01-19).

If rustdoc attempts to document a crate that has at least two dependencies which implement a panic_handler, it fails with an error similar to

error: duplicate lang item in crate `panic_halt` (which `panic2` depends on): `panic_impl`.
  |
  = note: first defined in crate `std` (which `panic2` depends on).

Note that the crate builds fine, it's just documenting that is broken.


Steps to reproduce

  1. Create a new project, e.g. cargo new panic2
  2. Add a dependency on panic-halt = "0.2.0", e.g. cargo add panic-halt
  3. Attempt to document the crate, e.g. cargo doc
An example of the different behavior between `stable`/`beta` and `build`/`doc`.
$ cargo +stable clean

$ cargo +beta clean

$ cargo +stable build
   Compiling panic-halt v0.2.0
   Compiling panic2 v0.1.0 (/Users/logic/Projects/tmp/panic2)
    Finished dev [unoptimized + debuginfo] target(s) in 0.35s

$ cargo +stable doc
 Documenting panic-halt v0.2.0
    Checking panic-halt v0.2.0
 Documenting panic2 v0.1.0 (/Users/logic/Projects/tmp/panic2)
    Finished dev [unoptimized + debuginfo] target(s) in 1.40s

$ cargo +beta build
   Compiling panic-halt v0.2.0
   Compiling panic2 v0.1.0 (/Users/logic/Projects/tmp/panic2)
    Finished dev [unoptimized + debuginfo] target(s) in 0.27s

$ cargo +beta doc
 Documenting panic-halt v0.2.0
    Checking panic-halt v0.2.0
 Documenting panic2 v0.1.0 (/Users/logic/Projects/tmp/panic2)
error: duplicate lang item in crate `panic_halt` (which `panic2` depends on): `panic_impl`.
  |
  = note: first defined in crate `std` (which `panic2` depends on).

error: aborting due to previous error

error: Could not document `panic2`.

Caused by:
  process didn't exit successfully: `rustdoc --edition=2018 --crate-type bin --crate-name panic2 src/main.rs -o /Users/logic/Projects/tmp/panic2/target/doc --error-format=json --json=diagnostic-rendered-ansi --document-private-items -L dependency=/Users/logic/Projects/tmp/panic2/target/debug/deps --extern panic_halt=/Users/logic/Projects/tmp/panic2/target/debug/deps/libpanic_halt-6d0ca720c3c355fb.rmeta` (exit code: 1)
@Mark-Simulacrum Mark-Simulacrum added regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jan 21, 2020
@Mark-Simulacrum
Copy link
Member

cc @rust-lang/rustdoc

@GuillaumeGomez GuillaumeGomez self-assigned this Jan 21, 2020
@GuillaumeGomez
Copy link
Member

I'll try to take a look in the next days.

@pietroalbini
Copy link
Member

Note that the stable release will be prepared on monday, so the patch should be merged by then.

@ollie27
Copy link
Member

ollie27 commented Jan 24, 2020

It looks like this was triggered by #66211 (cc. @kinnison). rustdoc now seems to load all crates passed by --extern even if they're unused in the code whereas rustc just ignores them. Adding extern crate panic_halt; or use panic_halt; to panic2 causes stable rustdoc and rustc to produce the same error message.

Is this issue affecting any real world code? It seems weird to list a dependency on panic-halt but not actually use it.

#66211 was to fix an ICE using the intra-doc-links feature but that's only available on nightly so it should be safe to revert #66211 on beta for now if needed.

@TimNN
Copy link
Contributor Author

TimNN commented Jan 24, 2020

I guess it is unlikely that this will be triggered in many real world scenarios. I came across this problem because I was experimenting with embedded programming: I had multiple panic implementations in the dependencies and then multiple binaries which were using different panic implementations (to see their different behaviors).

@kinnison
Copy link
Contributor

kinnison commented Jan 25, 2020

If there's a way to get the metadata into rustdoc without fully loading the --extern crates then that'd satisfy rustdoc's usecase without causing this. I do not object to backing this out of beta so long as there's some idea of how we'd fix it without entirely backing it out and reintroducing the ICE. @GuillaumeGomez how do you feel about that?

@GuillaumeGomez
Copy link
Member

We can revert it on the beta to prevent the crash but we definitely need a way to get the metadata from the crates.

@GuillaumeGomez
Copy link
Member

GuillaumeGomez commented Jan 25, 2020

So after a discussion with @eddyb, it appears that @petrochenkov is one of the last person working on this part.

cc @petrochenkov (at @eddyb's suggestion)

@JohnTitor JohnTitor added A-lang-item Area: Language items C-bug Category: This is a bug. labels Jan 26, 2020
@petrochenkov
Copy link
Contributor

If you don't want to resolve all crates passed with --extern (with which I agree), then resolve paths in intra-doc links, the crates will be used on demand then.

All the paths need to be resolved before the Resolver data is frozen (to_resolver_outputs is called), so rustdoc should organize its passes accordingly.

@petrochenkov
Copy link
Contributor

In other words, if you remove Resolver::clone_outputs while keeping rustdoc working, it will fix this issue and some other issues as well.

@GuillaumeGomez
Copy link
Member

I'll need more information because none of the Resolver methods you listed are used currently in rustdoc (or at least not directly). Also, how do we resolve those paths if resolving them triggers a panic?

@petrochenkov
Copy link
Contributor

petrochenkov commented Jan 26, 2020

The methods are not in rustdoc, but rustdoc is their only user and they exist due to it.
Path resolution won't panic if it is used before the Resolver data is frozen, as I said in a previous comment.

So, rustdoc should work similarly to rustc and organize its passes in such a way that they perform name resolution early.
I don't know how exactly this needs to be done, since I'm not a rustdoc maintainer.

@eddyb
Copy link
Member

eddyb commented Jan 26, 2020

@petrochenkov My understanding is that rustdoc works on the HIR, collects doc comments, and only then attempts to resolve paths in the markdown syntax of doc comments.

Maybe it should have a (much simpler) AST pass that only looks for paths in markdown links in #[doc = ...] attributes, and forces rustc_resolve to load the relevant crates.

I don't think it's possible to fully push rustdoc to before AST->HIR lowering.

@GuillaumeGomez
Copy link
Member

@petrochenkov My question was more: "how do we do that?". I know very little about the rustc internals generally.

@pietroalbini pietroalbini added regression-from-stable-to-stable Performance or correctness regression from one stable version to another. and removed regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels Jan 27, 2020
@jyn514
Copy link
Member

jyn514 commented Aug 14, 2020

This is now blocking #75176.

@jyn514
Copy link
Member

jyn514 commented Aug 14, 2020

Instead of Resolver having its own copy of cstore, is it possible to pass one in and have Resolver use that instead? I think TyCtxt::cstore() might come from the resolver in the first place so maybe it should be a public field or something so that having a cstore on hand is optional.

@jyn514
Copy link
Member

jyn514 commented Aug 14, 2020

Another idea is to store the resolver in the TyCtxt itself so that it never has to be cloned in the first place: #75176 (comment)

@petrochenkov
Copy link
Contributor

I already mentioned my preferred solution:

So, rustdoc should work similarly to rustc and organize its passes in such a way that they perform name resolution early.

+ eddyb's comment - #68427 (comment)

@jyn514 jyn514 added the A-resolve Area: Name resolution label Dec 12, 2020
JohnTitor added a commit to JohnTitor/rust that referenced this issue Apr 2, 2021
…rochenkov

rustdoc: Don't load all extern crates unconditionally

Instead, only load the crates that are linked to with intra-doc links.

This doesn't help very much with any of rustdoc's fundamental issues
with freezing the resolver, but it at least fixes a stable-to-stable
regression, and makes the crate loading model somewhat more consistent
with rustc's. I tested and it unfortunately does not help at all with rust-lang#82496.

Closes rust-lang#68427. Let me know if you want me to open a separate issue for not freezing the resolver.
r? `@petrochenkov` cc `@eddyb` `@ollie27`
JohnTitor added a commit to JohnTitor/rust that referenced this issue Apr 2, 2021
…rochenkov

rustdoc: Don't load all extern crates unconditionally

Instead, only load the crates that are linked to with intra-doc links.

This doesn't help very much with any of rustdoc's fundamental issues
with freezing the resolver, but it at least fixes a stable-to-stable
regression, and makes the crate loading model somewhat more consistent
with rustc's. I tested and it unfortunately does not help at all with rust-lang#82496.

Closes rust-lang#68427. Let me know if you want me to open a separate issue for not freezing the resolver.
r? ``@petrochenkov`` cc ``@eddyb`` ``@ollie27``
@bors bors closed this as completed in 640ce99 Apr 3, 2021
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Jul 2, 2021
…crate-load, r=jyn514

Revert "Don't load all extern crates unconditionally"

Fixes rust-lang#84738.

This reverts rust-lang#83738.

For the "smart" load of external crates, we need to be able to access their items in order to check their doc comments, which seems, if not impossible, quite complicated using only the AST.

For some context, I first tried to extend the `IntraLinkCrateLoader` visitor by adding `visit_foreign_item`. Unfortunately, it never enters into this call, so definitely not the right place...

I then added `visit_use_tree` to then check all the imports outside with something like this:

```rust
let mut loader = crate::passes::collect_intra_doc_links::IntraLinkCrateLoader::new(resolver);
    ast::visit::walk_crate(&mut loader, krate);

    let mut items = Vec::new();
    for import in &loader.imports_to_check {
        if let Some(item) = krate.items.iter().find(|i| i.id == *import) {
            items.push(item);
        }
    }
    for item in items {
        ast::visit::walk_item(&mut item);
        for attr in &item.attrs {
            loader.check_attribute(attr);
        }
    }
```

This was, of course, a failure. We find the items without problems, but we still can't go into the external crate to check its items' attributes.

Finally, `@jyn514` suggested to look into the [`CrateLoader`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_metadata/creader/struct.CrateLoader.html), but it only seems to provide metadata (I went through [`CStore`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_metadata/creader/struct.CStore.html) and [`CrateMetadata`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_metadata/rmeta/decoder/struct.CrateMetadata.html)).

I think we are too limited here (with AST only) to be able to determine the crates we actually need to import, but it's very likely that I missed something. Maybe `@petrochenkov` or `@Aaron1011` have an idea?

So until we find a way to make it work completely, we need to revert it to fix the ICE. Once merged, we'll need to re-open rust-lang#68427.

r? `@jyn514`
@jyn514
Copy link
Member

jyn514 commented Jul 4, 2021

#83738 was reverted in #85749.

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 A-lang-item Area: Language items A-resolve Area: Name resolution C-bug Category: This is a bug. regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
10 participants