Fix docscrape memoization #10524
Fix docscrape memoization #10524
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) soon. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
| @@ -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( | |||
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
I'll close this PR in favor of Will's #10533 then. |
This PR fixes a memoization issue in docscraped units when profile overrides are involved.
Split off from #10493 at @ehuss' request.
Fixes #10500.