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

Hide host effect params from docs #116670

Merged
merged 6 commits into from
Oct 13, 2023
Merged

Hide host effect params from docs #116670

merged 6 commits into from
Oct 13, 2023

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Oct 12, 2023

addresses (only on nightly, needs backport) #116629

r? @compiler-errors

cc @GuillaumeGomez @fee1-dead

@oli-obk oli-obk added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 12, 2023
@rustbot rustbot added A-rustdoc-json Area: Rustdoc JSON backend 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 Oct 12, 2023
@rustbot
Copy link
Collaborator

rustbot commented Oct 12, 2023

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

cc @camelid

@@ -597,6 +598,7 @@ fn clean_generic_param<'tcx>(
ty: Box::new(clean_ty(ty, cx)),
default: default
.map(|ct| Box::new(ty::Const::from_anon_const(cx.tcx, ct.def_id).to_string())),
is_host_effect: cx.tcx.has_attr(param.def_id, sym::rustc_host),
Copy link
Member

Choose a reason for hiding this comment

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

Why are we re-encoding this here? Why not just call has_attr when needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this works if it comes from the HIR, but that's not always true, there is also a code path where the is_host_effect comes from a ty::GenericParamDef due to coming from a different crate. I don't have tests for this, but that's the reason why we encode this field in ty::GenericParamDef instead of just loading the information lazily.

Copy link
Member

@compiler-errors compiler-errors Oct 12, 2023

Choose a reason for hiding this comment

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

You can call tcx.has_attr on a foreign param, though, and we should be encoding attrs for foreign params too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought we were filtering out most of them.

@fee1-dead do you remember why we put the is_host_effect field on GenericParamDef? did I ask for it just to call has_attr less?

Copy link
Member

Choose a reason for hiding this comment

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

#[rustc_host] used to be encoded in an earlier version of #115727 (84a4907) but since #115727 (comment) it no longer is.

Copy link
Member

Choose a reason for hiding this comment

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

You can call tcx.has_attr on a foreign param, though, and we should be encoding attrs for foreign params too.

We currently don't encode attributes on const generics params, and IIRC I added the field for that purpose

@fmease
Copy link
Member

fmease commented Oct 12, 2023

I could be mistaken but I think this doesn't hide host arguments (I can't check out the patch rn, short on time). Consider:

#![feature(effects, const_trait_impl)]

#[const_trait]
pub trait Tr {
    fn f();
}

pub const fn g<T: ~const Tr>() { }

It's rendered as pub const fn g<T: Tr<host>, const host: bool = true>() on master.
If it's already fixed, then my bad but could you still add a test for it? Appreciate it!

@fmease
Copy link
Member

fmease commented Oct 12, 2023

I could be mistaken but I think this doesn't hide host arguments

I don't know if there're actually any ~const bounds left in core or std due to the const refactoring (again, can't check rn, commuting), so this might not be as pressing and therefore might not need to be addressed in this PR.

If you do want to handle them in this PR, then it's fine to just drop such host args outright without attempting to render ~const in rustdoc since we don't want to show that in the docs at the moment.

@oli-obk
Copy link
Contributor Author

oli-obk commented Oct 12, 2023

If you do want to handle them in this PR, then it's fine to just drop such host args outright without attempting to render ~const in rustdoc since we don't want to show them in the docs at the moment.

already done, just took a bit of time to find all the pieces that were needed.

@compiler-errors
Copy link
Member

r? fmease since you've given the most detailed review

@rustbot rustbot assigned fmease and unassigned compiler-errors Oct 13, 2023
Copy link
Member

@fmease fmease left a comment

Choose a reason for hiding this comment

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

r=me with FIXMEs added

@@ -597,6 +598,7 @@ fn clean_generic_param<'tcx>(
ty: Box::new(clean_ty(ty, cx)),
default: default
.map(|ct| Box::new(ty::Const::from_anon_const(cx.tcx, ct.def_id).to_string())),
is_host_effect: cx.tcx.has_attr(param.def_id, sym::rustc_host),
Copy link
Member

Choose a reason for hiding this comment

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

#[rustc_host] used to be encoded in an earlier version of #115727 (84a4907) but since #115727 (comment) it no longer is.

@@ -104,6 +104,7 @@ pub(crate) fn ty_args_to_args<'tcx>(
arg: index,
}),
))),
GenericArgKind::Const(ct) if let ty::ConstKind::Param(p) = ct.kind() && p.name == sym::host => None,
Copy link
Member

@fmease fmease Oct 13, 2023

Choose a reason for hiding this comment

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

Contrary to the HIR code path which filters out #[rustc_host] params, this filters out params named host which isn't quite correct, right? Could you leave a FIXME(effects) if it's too annoying to fix rn?

Copy link
Member

@fmease fmease Oct 13, 2023

Choose a reason for hiding this comment

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

Since we're just looking for ConstKind::Param, we obviously don't hide host args like false, true or more complex const exprs (we'd need to check the corresp. ty::Generics first I think). Could you add a FIXME(effects) for this as well / extend the previous one.

I only know of one case where this currently surfaces (without using #![feature(rustc_attrs)] + #[rustc_host]). Namely, documenting the cross-crate re-export of pub fn h<T: Tr>() {} where Tr is a const trait results in pub fn h<T>() where T: Tr<true> since we currently don't elide defaulted args in cross-crate scenarios (#80379). My PR #112463 would fix this specific case but it's blocked on performance.

(This would also surface with always-const bounds if they were introduced like T: const TraitT: Trait<false>, etc.)

hir::GenericArg::Lifetime(_) => GenericArg::Lifetime(Lifetime::elided()),
hir::GenericArg::Type(ty) => GenericArg::Type(clean_ty(ty, cx)),
hir::GenericArg::Const(ct)
if cx.tcx.has_attr(ct.value.def_id, sym::rustc_host) =>
Copy link
Member

@fmease fmease Oct 13, 2023

Choose a reason for hiding this comment

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

This doesn't hide host args like false, true in impls. Consider:

#[const_trait] pub trait Tr {}
impl Tr for () {}

It gets rendered as impl Tr<true> for () (local/HIR & cross-crate/metadata). Doesn't look super easy to fix, we'd need to look at the corresp. ty::Generics I guess? Please add a FIXME(effects).

@oli-obk
Copy link
Contributor Author

oli-obk commented Oct 13, 2023

@bors r=fmease

@bors
Copy link
Contributor

bors commented Oct 13, 2023

📌 Commit 16f8396 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 Oct 13, 2023
@bors
Copy link
Contributor

bors commented Oct 13, 2023

⌛ Testing commit 16f8396 with merge 193e8a1...

@bors
Copy link
Contributor

bors commented Oct 13, 2023

☀️ Test successful - checks-actions
Approved by: fmease
Pushing 193e8a1 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 13, 2023
@bors bors merged commit 193e8a1 into rust-lang:master Oct 13, 2023
12 checks passed
@rustbot rustbot added this to the 1.75.0 milestone Oct 13, 2023
@oli-obk oli-obk deleted the host_docs branch October 17, 2023 07:03
@a1phyr
Copy link
Contributor

a1phyr commented Oct 17, 2023

only on nightly

The issue also exists in beta, so this needs backporting: https://doc.rust-lang.org/beta/core/ptr/struct.NonNull.html#method.dangling

@GuillaumeGomez
Copy link
Member

It was already nominated to be backported to beta.

bors-ferrocene bot added a commit to ferrocene/ferrocene that referenced this pull request Oct 17, 2023
54: Pull upstream master 2023 10 17 r=pietroalbini a=Veykril

* rust-lang/rust#116196
* rust-lang/rust#116824
* rust-lang/rust#116822
* rust-lang/rust#116477
* rust-lang/rust#116826
* rust-lang/rust#116820
  * rust-lang/rust#116811
  * rust-lang/rust#116808
  * rust-lang/rust#116805
  * rust-lang/rust#116800
  * rust-lang/rust#116798
  * rust-lang/rust#116754
* rust-lang/rust#114370
* rust-lang/rust#116804
  * rust-lang/rust#116802
  * rust-lang/rust#116790
  * rust-lang/rust#116786
  * rust-lang/rust#116709
  * rust-lang/rust#116430
  * rust-lang/rust#116257
  * rust-lang/rust#114157
* rust-lang/rust#116731
* rust-lang/rust#116550
* rust-lang/rust#114330
* rust-lang/rust#116724
* rust-lang/rust#116782
  * rust-lang/rust#116776
  * rust-lang/rust#115955
  * rust-lang/rust#115196
* rust-lang/rust#116775
* rust-lang/rust#114589
* rust-lang/rust#113747
* rust-lang/rust#116772
  * rust-lang/rust#116771
  * rust-lang/rust#116760
  * rust-lang/rust#116755
  * rust-lang/rust#116732
  * rust-lang/rust#116522
  * rust-lang/rust#116341
  * rust-lang/rust#116172
* rust-lang/rust#110604
* rust-lang/rust#110729
* rust-lang/rust#116527
* rust-lang/rust#116688
* rust-lang/rust#116757
  * rust-lang/rust#116753
  * rust-lang/rust#116748
  * rust-lang/rust#116741
  * rust-lang/rust#116594
* rust-lang/rust#116691
* rust-lang/rust#116643
* rust-lang/rust#116683
* rust-lang/rust#116635
* rust-lang/rust#115515
* rust-lang/rust#116742
  * rust-lang/rust#116661
  * rust-lang/rust#116576
  * rust-lang/rust#116540
* rust-lang/rust#116352
* rust-lang/rust#116737
  * rust-lang/rust#116730
  * rust-lang/rust#116723
  * rust-lang/rust#116715
  * rust-lang/rust#116603
  * rust-lang/rust#116591
  * rust-lang/rust#115439
* rust-lang/rust#116264
* rust-lang/rust#116727
  * rust-lang/rust#116704
  * rust-lang/rust#116696
  * rust-lang/rust#116695
  * rust-lang/rust#116644
  * rust-lang/rust#116630
* rust-lang/rust#116728
  * rust-lang/rust#116689
  * rust-lang/rust#116679
  * rust-lang/rust#116618
  * rust-lang/rust#116577
  * rust-lang/rust#115653
* rust-lang/rust#116702
* rust-lang/rust#116015
* rust-lang/rust#115822
* rust-lang/rust#116407
* rust-lang/rust#115719
* rust-lang/rust#115524
* rust-lang/rust#116705
* rust-lang/rust#116645
* rust-lang/rust#116233
* rust-lang/rust#115108
* rust-lang/rust#116670
* rust-lang/rust#116676
* rust-lang/rust#116666



Co-authored-by: Benoît du Garreau <[email protected]>
Co-authored-by: Colin Finck <[email protected]>
Co-authored-by: Ian Jackson <[email protected]>
Co-authored-by: Joshua Liebow-Feeser <[email protected]>
Co-authored-by: León Orell Valerian Liehr <[email protected]>
Co-authored-by: Trevor Gross <[email protected]>
Co-authored-by: Evan Merlock <[email protected]>
Co-authored-by: joboet <[email protected]>
Co-authored-by: Ralf Jung <[email protected]>
Co-authored-by: DaniPopes <[email protected]>
Co-authored-by: Mark Rousskov <[email protected]>
Co-authored-by: onur-ozkan <[email protected]>
Co-authored-by: Nicholas Nethercote <[email protected]>
Co-authored-by: The 8472 <[email protected]>
Co-authored-by: Samuel Thibault <[email protected]>
Co-authored-by: reez12g <[email protected]>
Co-authored-by: Jakub Beránek <[email protected]>
@fmease
Copy link
Member

fmease commented Oct 19, 2023

I don't know how beta-nominated rustdoc PRs get typically approved (since they're usually not showcased in the weekly t-compiler meetings afaict), typically @apiraino does it by themself looking at past PRs.

Anyway, it's no longer super important to backport this PR due to the beta-accepted #116856, right?

@apiraino
Copy link
Contributor

correct, t-rustdoc backports are now handled by the team itself (Zulip announcement, sort of)

@apiraino apiraino added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Oct 19, 2023
@fmease
Copy link
Member

fmease commented Oct 19, 2023

Oh wow, I even reacted to your Zulip message back then but I've totally forgotten about it :'D

@cuviper cuviper modified the milestones: 1.75.0, 1.74.0 Oct 21, 2023
@cuviper cuviper removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 21, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 21, 2023
[beta] backports and stage0 bump

- Bump stage0 to released stable compiler
- Hide host effect params from docs rust-lang#116670
- Fix a performance regression in obligation deduplication. rust-lang#116826
- Make `#[repr(Rust)]` and `#[repr(C)]` incompatible with one another rust-lang#116829
- Update to LLVM 17.0.3 rust-lang#116840
- Disable effects in libcore again rust-lang#116856
- revert rust-lang#114586 rust-lang#116879

r? cuviper
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 8, 2023
…ide-x-crate-host-args, r=GuillaumeGomez

rustdoc: properly elide cross-crate host effect args

Fixes FIXMEs introduced in rust-lang#116670.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Nov 8, 2023
Rollup merge of rust-lang#117531 - fmease:rustdoc-effects-properly-elide-x-crate-host-args, r=GuillaumeGomez

rustdoc: properly elide cross-crate host effect args

Fixes FIXMEs introduced in rust-lang#116670.
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 18, 2023
…nkov

Reenable effects in libcore

With rust-lang#116670, rust-lang#117531, and rust-lang#117171, I think we would be comfortable with re-enabling the effects feature for more testing in libcore.

r? `@oli-obk`
cc `@fmease`
cc rust-lang#110395
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rustdoc-json Area: Rustdoc JSON backend beta-accepted Accepted for backporting to the compiler in the beta channel. merged-by-bors This PR was explicitly merged by bors. 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.

10 participants