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

Add f16 and f128 to rustdoc's PrimitiveType #123581

Merged
merged 1 commit into from
Apr 7, 2024
Merged

Add f16 and f128 to rustdoc's PrimitiveType #123581

merged 1 commit into from
Apr 7, 2024

Conversation

tgross35
Copy link
Contributor

@tgross35 tgross35 commented Apr 7, 2024 •
edited
Loading

Fix a few places where these primitives were missing from librustdoc. This should fix the CI failures from doc links in #122470.

@rustbot
Copy link
Collaborator

rustbot commented Apr 7, 2024

r? @fmease

rustbot has assigned @fmease.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Apr 7, 2024
@rustbot
Copy link
Collaborator

rustbot commented Apr 7, 2024

Some changes occurred in src/librustdoc/clean/types.rs

cc @camelid

@tgross35
Copy link
Contributor Author

tgross35 commented Apr 7, 2024

@rustbot label F-f16_and_f128

@rustbot rustbot added the F-f16_and_f128 `#![feature(f16)]`, `#![feature(f128)]` label Apr 7, 2024
This was referenced Apr 7, 2024
Open
Merged
@fmease
Copy link
Member

fmease commented Apr 7, 2024

rustbot has assigned @fmease.

How fitting :P

Jokes aside, does this fix the issues you were seeing?

@fmease
Copy link
Member

fmease commented Apr 7, 2024 •
edited
Loading

I don't exactly know anymore how far along #![feature(f16|f128)] is but it would be great if you could add some rustdoc tests for them in this PR demonstrating what you fixed. There are plenty of areas to choose from: Basic rendering, intra-doc links, synthetic auto trait impls, etc.

@tgross35
Copy link
Contributor Author

tgross35 commented Apr 7, 2024

rustbot has assigned @fmease.

How fitting :P

I thought the same :D

Jokes aside, does this fix the issues you were seeing?

Yes it did, the changes from #122470 show up now.

image

I don't exactly know anymore how far along #![feature(f16|f128)] is but it would be great if you could add some rustdoc tests for them in this PR demonstrating what you fixed. There are plenty of areas to choose from: Basic rendering, intra-doc links, synthetic auto trait impls, etc.

Are there any tests for the other float types that I could use as a reference? I am not seeing anything but might not be looking in the right place.

@tgross35 tgross35 closed this Apr 7, 2024
@tgross35 tgross35 reopened this Apr 7, 2024
@tgross35
Copy link
Contributor Author

tgross35 commented Apr 7, 2024

HIt ctrl enter at the wrong time, whoops.

Anyway #122470 does not pass CI currently (broken links error) but with this patch applied it does. Is that sufficient for a test or did you mean something else?

@notriddle
Copy link
Contributor

https://github.com/rust-lang/rust/blob/master/tests/rustdoc/primitive/primitive.rs

@fmease
Copy link
Member

fmease commented Apr 7, 2024 •
edited
Loading

I thought of something like tests/rustdoc/primitive-link.rs. I would suggest to add an intra-doc link test tests/rustdoc/intra-doc/f16-f128.rs:

#![deny(rustdoc::broken_intra_doc_links)]
#![features(f16, f128)]
//! [`f16`]. [`f128`].

// @has f16_f128/index.html
// @has - '//a/@href' '{{channel}}/std/primitive.f16.html'
// @has - '//a/@href' '{{channel}}/std/primitive.f128.html'

If I'm not mistaken, this test currently fails on master. I know that this test is probably not super important but we can always remove it, extend it or do whatever.

@fmease
Copy link
Member

fmease commented Apr 7, 2024

Ah, but notriddle's suggestion might be better since the necessary library/ changes are part of the other PR so my test will probably still fail even with your patch applied.

@tgross35
Add f16 and f128 to rustdoc's PrimitiveType
ebc86e6 
Fix a few places where these primitives were missing from librustdoc.
@tgross35
Copy link
Contributor Author

tgross35 commented Apr 7, 2024

I added notriddle's test and verified it fails on master, but passes with this change. Would you still want the intra-doc link test? If so, I can add that after #122470 lands.

Is there a good place to throw an error if rustc_doc_primitive has an unexpected type, to make this a bit more noticeable in the future? I am not sure if there is a reasonable way to do this.

@fmease
Copy link
Member

fmease commented Apr 7, 2024 •
edited
Loading

Is there a good place to throw an error if rustc_doc_primitive has an unexpected type, to make this a bit more noticeable in the future? I am not sure if there is a reasonable way to do this.

Yes, we should definitely do that to improve the contributor UX. We have a FIXME here:

rust/src/librustdoc/clean/types.rs

Lines 266 to 272 in 5dbaafd

let as_primitive = |res: Res<!>| {
let Res::Def(DefKind::Mod, def_id) = res else { return None };
tcx.get_attrs(def_id, sym::rustc_doc_primitive).find_map(|attr| {
// FIXME: should warn on unknown primitives?
Some((def_id, PrimitiveType::from_symbol(attr.value_str()?)?))
})
};

I haven't checked if it would make sense to add a check in this specific location but feel free to experiment and/or look for other places mentioning rustc_doc_primitive. If you have the time and motivation to implement that check, that'd be amazing. Feel free to r? fmease the future PR.

@fmease
Copy link
Member

fmease commented Apr 7, 2024

@bors r rollup

@bors
Copy link
Contributor

bors commented Apr 7, 2024

📌 Commit ebc86e6 has been approved by fmease

It is now in the queue for this repository.

@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 7, 2024
@matthiaskrgr matthiaskrgr mentioned this pull request Apr 7, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 7, 2024
@bors
Auto merge of rust-lang#123585 - matthiaskrgr:rollup-dexaj7a, r=matth…
8f2c255 
…iaskrgr

Rollup of 4 pull requests

Successful merges:

 - rust-lang#123410 (Relax framework linking test)
 - rust-lang#123446 (Fix incorrect 'llvm_target' value used on watchOS target)
 - rust-lang#123579 (add some more tests)
 - rust-lang#123581 (Add `f16` and `f128` to rustdoc's `PrimitiveType`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 549d85d into rust-lang:master Apr 7, 2024
11 checks passed
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 7, 2024
@rust-timer
Unrolled build for rust-lang#123581
0f16779 
Rollup merge of rust-lang#123581 - tgross35:f16-f128-rustdoc-updates, r=fmease

Add `f16` and `f128` to rustdoc's `PrimitiveType`

Fix a few places where these primitives were missing from librustdoc. This should fix the CI failures from doc links in rust-lang#122470.
@rustbot rustbot added this to the 1.79.0 milestone Apr 7, 2024
@tgross35 tgross35 deleted the f16-f128-rustdoc-updates branch April 12, 2024 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@fmease fmease

Labels
F-f16_and_f128 `#![feature(f16)]`, `#![feature(f128)]` 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
Milestone
1.79.0
Development

Successfully merging this pull request may close these issues.
5 participants
@tgross35 @rustbot @fmease @notriddle @bors