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.

Sign up for GitHub

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

Jump to bottom

Fix docscrape memoization #10524

Closed
wants to merge 2 commits into from
Closed

Fix docscrape memoization #10524

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
90fc7dc
properly memoize Docscrape units
willcrichton Mar 29, 2022
41b6810
add test for `-Z rustdoc-scrape-examples` issue #10500
lqd Mar 29, 2022
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
3 changes: 2 additions & 1 deletion src/cargo/core/compiler/unit_dependencies.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -717,14 +717,15 @@ fn compute_deps_doc(
unit_for.root_compile_kind(),
);
deps_of(scrape_unit, state, unit_for)?;
ret.push(new_unit_dep(
ret.push(new_unit_dep_with_profile(
Copy link
Contributor

Choose a reason for hiding this comment

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

In trying to better understand when someone chooses to use new_unit_dep instead of new_unit_dep_with_profile, I was lost until I found a comment for the only direct use of new_unit_dep_with_profile.

Maybe this deserves a similar comment
(granted, I know you aren't the one who made the change so unsure if you feel confident in doing so)

Copy link
Member Author

Choose a reason for hiding this comment

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

unsure if you feel confident in doing so

not particularly ^^

so if you, Eric, or @willcrichton have ideas on what you'd like to see here, I could add that -- but I wouldn't be able to come up with an explanation other than what I believe is happening (but I know little about docscraping and cargo, and could be wrong): recording the profile of these "fake" unit dependencies is important to memoization here, otherwise checking later if the actual units were built will not be found in that map.

Copy link
Contributor

Choose a reason for hiding this comment

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

Everything should use new_unit_dep except for the one place it is done for build scripts because build scripts need different profiles for build-overrides.

This change doesn't appear correct to me. The line just above (let unit_for = …) that shadows unit_for does not seem like the right approach. It is generating an incorrect unit_for with the wrong settings, and that is why the rest of the units end up with the wrong profile. Commenting out that line seems to get all tests to pass.

Perhaps @willcrichton can say why it is creating a new unit_for instead of inheriting the one from the doc unit.

Copy link
Contributor

@willcrichton willcrichton Apr 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Ok after some inspection / experimentation, I believe that @ehuss is right and my code was incorrect. The original intention was to fix an issue that I just realized had a deeper root cause I was able to identify and fix (that proc macros should never actually be scraped). I will put up my own PR so @lqd doesn't have to do more work on my behalf.

See #10533.

state,
scrape_unit,
&scrape_unit.pkg,
&scrape_unit.target,
unit_for,
scrape_unit.kind,
scrape_unit.mode,
scrape_unit.profile.clone(),
IS_NO_ARTIFACT_DEP,
)?);
}
Expand Down
54 changes: 54 additions & 0 deletions tests/testsuite/doc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2560,6 +2560,60 @@ fn scrape_examples_missing_flag() {
.run();
}

#[cargo_test]
fn scrape_examples_with_overriden_profiles() {
if !is_nightly() {
// -Z rustdoc-scrape-examples is unstable
return;
}

// #10500 is an issue where different profile overrides would make `cargo -Z
// rustdoc-scrape-examples` panic due to incomplete memoization of some
// units to scrape.

let p = project()
.file(
"Cargo.toml",
r#"
[package]
name = "foo"
version = "0.0.1"
authors = []

[profile.dev]
panic = "abort"
"#,
)
.file("examples/ex.rs", "fn main() { foo::foo(); }")
.file("src/lib.rs", "pub fn foo() {}\npub fn bar() { foo(); }")
.build();

p.cargo("doc --all -Zunstable-options -Z rustdoc-scrape-examples=all")
.masquerade_as_nightly_cargo()
.run();

let p = project()
.file(
"Cargo.toml",
r#"
[package]
name = "foo"
version = "0.0.1"
authors = []

[profile.dev.build-override]
debug = 0
"#,
)
.file("examples/ex.rs", "fn main() { foo::foo(); }")
.file("src/lib.rs", "pub fn foo() {}\npub fn bar() { foo(); }")
.build();

p.cargo("doc --all -Zunstable-options -Z rustdoc-scrape-examples=all")
.masquerade_as_nightly_cargo()
.run();
}

#[cargo_test]
fn lib_before_bin() {
// Checks that the library is documented before the binary.
Expand Down