Skip to content

Add f16 and f128 to rustdoc's PrimitiveType#123581

Merged
bors merged 1 commit intorust-lang:masterfrom
tgross35:f16-f128-rustdoc-updates
Apr 7, 2024
Merged

Add f16 and f128 to rustdoc's PrimitiveType#123581
bors merged 1 commit intorust-lang:masterfrom
tgross35:f16-f128-rustdoc-updates

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

notriddle commented Apr 7, 2024

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 tgross35 force-pushed the f16-f128-rustdoc-updates branch from cc049f6 to ebc86e6 Compare April 7, 2024 03:47
@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
Collaborator

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
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