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: Discriminate required and provided associated constants and types #95316

Merged

Conversation

fmease
Copy link
Member

@fmease fmease commented Mar 25, 2022

Currently, rustdoc merely separates required and provided associated functions (i.e. methods). This PR extends this to constants (fixes #94652) and types. This makes the documentation of all three kinds of associated items more alike and consistent.

As an aside, associated types may actually be provided / have a default when users enable the unstable feature associated_type_defaults.

Before After
image image
image image

clean::types::ItemKind modification

  • ItemKind::TypedefItem(.., true)ItemKind::AssocTypeItem(..)
  • ItemKind::TypedefItem(.., false)ItemKind::TypedefItem(..)

Further, I added ItemKind::TyAssoc{Const,Type}Item, the “required” variant of ItemKind::Assoc{Const,Type}Item, analogous to ItemKind::TyMethodItem with ItemKind::MethodItem. These new variants don't contain new information really, they are just the result of me getting rid of the Option<_> field in AssocConstItem and AssocTypeItem.

Goal: Make associated items more consistent.
Originally I thought modifying ItemKind was necessary to achieve the new functionality of this PR but in retrospect, it does not. If you don't like the changes to ItemKind, I think I can get rid of them.

This change is the root cause of those tiny changes in a lot of different files.

Concerns and Open Questions

  • breaking changes to hyperlinks: Some heading IDs change:
    • associated-const (sic!) -> {provided,required}-associated-consts
    • associated-types -> {provided,required}-associated-types
  • verbosity of the headings {Required,Provided} Associated {Constants,Types}
  • For some files, I am not sure if the changes I made are correct. So please take extra care when reviewing conversions.rs (conversion to JSON), cache.rs/fold_item, stripper.rs/fold_item, check_doc_test_visibility.rs/should_have_doc_example, collect_intra_doc_links.rs/from_assoc_item
  • JSON output: I still map AssocTypeItems to Typedef etc. (FIXME)

@rustbot rustbot added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Mar 25, 2022
@rust-highfive
Copy link
Collaborator

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez,@Folyd,@jsha

Some changes occurred in clean/types.rs.

cc @camelid

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @CraftSpider (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 25, 2022
src/librustdoc/clean/types.rs Outdated Show resolved Hide resolved
@GuillaumeGomez
Copy link
Member

does this require an FCP?

For the URL changes, definitely. For the UI changes, not really unless some someone has concerns.

Otherwise I think it's a nice improvement overall. I like having new sections. Not a big fan of the new spaces you added in the type definition source code block.

  • I am currently printing = ... for provided associated constants mirroring { ... } for provided methods. I am not sure if you like that. I prefer printing something to make required and provided constants look distinct in the code snippet. I do not print the default value itself since that was recently removed and...

It's logical for methods to not write the content since it's way too verbose. But for constants I think it'd make more sense to provide the value.

@GuillaumeGomez
Copy link
Member

Also cc @camelid (for code changes) and @Manishearth (for UI changes).

@rust-log-analyzer

This comment has been minimized.

@Manishearth
Copy link
Member

Manishearth commented Mar 26, 2022

I'm okay with this from the UI POV.

Not a fan of URL fragments breaking but overall as long as it's just the fragments i'm still okay with this

@fmease
Copy link
Member Author

fmease commented Mar 26, 2022

I have to add that the URL change prepending ty to certain fragments is technically not even necessary as far as I understand the code base. I just followed suit looking at the existing distinction drawn between tymethods and methods.

(OTOH, the addition of those new sections force URL fragment changes no matter what obviously)

I went for consistency, though I can go for backward compatibility and performance(?) depending on the concensus.

@fmease

This comment was marked as outdated.

@Manishearth
Copy link
Member

I went for consistency, though I can go for backward compatibility and performance(?) depending on the concensus.

Yeah I'm fine with the consistent solution too.

@GuillaumeGomez
Copy link
Member

I think overall you went a bit too far on this feature: having new sections to differentiate provided and required associated items is a good idea (and even makes the clean enum smaller). However, making the difference in the search is not useful (in my opinion) nor it is to have a new anchor kind (but for this one, I can see some use, so not really against it).

I am not sure if I personally should attempt to fix this or what's going on. Could someone please illuminate me? Thanks :)

You might have discovered a new rustdoc bug. 😄 Before making any fixes in the std docs, better wait we all agree on the changes here to prevent you to do it more than once (which will already be huge...).

@fmease

This comment was marked as resolved.

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 28, 2022
@fmease fmease force-pushed the rustdoc-discr-req-prov-assoc-consts-tys branch from 1081a0e to be3867a Compare March 29, 2022 14:08
@fmease

This comment was marked as outdated.

@rustbot rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Mar 29, 2022
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 29, 2022
@fmease fmease force-pushed the rustdoc-discr-req-prov-assoc-consts-tys branch from 19e1cf1 to 1dabd1b Compare April 12, 2022 13:02
@GuillaumeGomez
Copy link
Member

cc @notriddle @camelid in case you want to make another review round

@fmease fmease force-pushed the rustdoc-discr-req-prov-assoc-consts-tys branch from 1dabd1b to 8de453a Compare April 12, 2022 13:39
@fmease fmease requested a review from camelid April 12, 2022 13:39
@GuillaumeGomez
Copy link
Member

Let's go!

@bors: r=notriddle,GuillaumeGomez

@bors
Copy link
Contributor

bors commented Apr 12, 2022

📌 Commit 8de453a has been approved by notriddle,GuillaumeGomez

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 12, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 13, 2022
Rollup of 7 pull requests

Successful merges:

 - rust-lang#95316 (Rustdoc: Discriminate required and provided associated constants and types)
 - rust-lang#95405 (Move name resolution logic to a dedicated file)
 - rust-lang#95914 (Implement tuples using recursion)
 - rust-lang#95918 (Delay a bug when we see SelfCtor in ref pattern)
 - rust-lang#95970 (Fix suggestions in case of `T:` bounds)
 - rust-lang#95973 (prevent opaque types from appearing in impl headers)
 - rust-lang#95986 (Autolabel library PRs with T-libs)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 0ec00c0 into rust-lang:master Apr 13, 2022
@rustbot rustbot added this to the 1.62.0 milestone Apr 13, 2022
@fmease fmease deleted the rustdoc-discr-req-prov-assoc-consts-tys branch April 13, 2022 05:50
@pnkfelix
Copy link
Member

pnkfelix commented Apr 20, 2022

Hey @fmease and @GuillaumeGomez , it seems like rollup PR #95990 regressed diesel-1.4.8 doc by 4.1% and many-assoc-items doc by 71.71%

Skimming over the contents of rollup PR #95990, it seems plausible that this PR is the root cause of those regressions. (Useful hint: the links for each of the numeric percentages go to specific pages that include the performance collector invocation that downloads the compiler artifacts, runs them on the benchmark atop cachegrind, and diffs the performance data; see e.g. 71.71% link)

Can one of you look into whether this PR is the cause of those regressions?

@GuillaumeGomez
Copy link
Member

I'll try to as soon as possible.

@fmease
Copy link
Member Author

fmease commented Apr 22, 2022

@pnkfelix Apologies for the missing updates. I am going to take a look at the regression in a few hours from now. Since this is my first time profiling rustc, I had to take some time setting everything up etc. It's very likely that my PR is the cause of the heavy regressions. Thus I will prioritize this issue.

@rylev
Copy link
Member

rylev commented May 17, 2022

@fmease @GuillaumeGomez has there been any progress on figuring out a fix for the performance improvement?

@GuillaumeGomez
Copy link
Member

I think @fmease found something but otherwise no idea.

@fmease
Copy link
Member Author

fmease commented May 17, 2022

@rylev I am terribly sorry to report that no, I do not have any clue how to improve the performance since I have next to no expertise with const evaluation, the origin of the regression. rustdoc simply calls tcx.const_eval_poly which in turn calls the query rustc_query_impl::make_query::eval_to_const_value_raw which dominates those benchmarks, e.g. 67% for many-assoc-items where the query is called 9987 times (once per assoc item). Before this PR, the query was called zero times. (It's a feature that we do this evaluation now).

I am not sure if anything can be done on the rustdoc side, maybe the representation of constants in rustdoc (clean::Constant) has to drastically change to store already evaluated constants (@GuillaumeGomez most recently optimized clean-constants, I think, in #82873 and I am not at all familiar with why clean::ConstantKind looks the way it does). However, const evaluation still has to happen approx. 10,000 times in many-assoc-items at some point. In my assessment, const evaluation itself has to be optimized and it's a matter of the const-eval WG. I don't want to sound audacious, I'd love to help but I am a rustc newbie and would have to read through a lot of things. I think the regression necessitates architectural optimizations – which is currently under way with valtrees if I understand correctly.

The only thing I personally can offer right now is a temporary workaround / partial rollback of this PR where we don't do any const eval (but still keep basically 90% of the changes introduced in this PR). @GuillaumeGomez, I know, you once expressed your disapproval in regards to that (printing a placeholder like ... or _ after the = for provided associated constants, edit: printing the unevaluated const instead is also an option).

@GuillaumeGomez
Copy link
Member

Performance regression could be reduced with the help of someone who knows about the const evaluation but the added information is definitely important in my opinion.

Do you know who we could ask for help @rylev by any chance?

@rylev
Copy link
Member

rylev commented May 18, 2022

Perhaps @oli-obk might be of some assistance?

@oli-obk
Copy link
Contributor

oli-obk commented May 18, 2022

Before this PR, the query was called zero times. (It's a feature that we do this evaluation now).

There's nothing to be done about this. If you want an evaluated constant you need to evaluate it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

Documentation of associated constants