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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 19 additions & 9 deletions src/librustdoc/clean/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -521,7 +521,7 @@ fn clean_generic_param_def<'tcx>(
},
)
}
ty::GenericParamDefKind::Const { has_default, .. } => (
ty::GenericParamDefKind::Const { has_default, is_host_effect } => (
def.name,
GenericParamDefKind::Const {
ty: Box::new(clean_middle_ty(
Expand All @@ -541,6 +541,7 @@ fn clean_generic_param_def<'tcx>(
)),
false => None,
},
is_host_effect,
},
),
};
Expand Down Expand Up @@ -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

},
),
};
Expand Down Expand Up @@ -2508,14 +2510,22 @@ fn clean_generic_args<'tcx>(
let args = generic_args
.args
.iter()
.map(|arg| match arg {
hir::GenericArg::Lifetime(lt) if !lt.is_anonymous() => {
GenericArg::Lifetime(clean_lifetime(*lt, cx))
}
hir::GenericArg::Lifetime(_) => GenericArg::Lifetime(Lifetime::elided()),
hir::GenericArg::Type(ty) => GenericArg::Type(clean_ty(ty, cx)),
hir::GenericArg::Const(ct) => GenericArg::Const(Box::new(clean_const(ct, cx))),
hir::GenericArg::Infer(_inf) => GenericArg::Infer,
.filter_map(|arg| {
Some(match arg {
hir::GenericArg::Lifetime(lt) if !lt.is_anonymous() => {
GenericArg::Lifetime(clean_lifetime(*lt, cx))
}
hir::GenericArg::Lifetime(_) => GenericArg::Lifetime(Lifetime::elided()),
hir::GenericArg::Type(ty) => GenericArg::Type(clean_ty(ty, cx)),
// FIXME(effects): This will still emit `<true>` for non-const impls of const traits
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).

{
return None;
}
hir::GenericArg::Const(ct) => GenericArg::Const(Box::new(clean_const(ct, cx))),
hir::GenericArg::Infer(_inf) => GenericArg::Infer,
})
})
.collect::<Vec<_>>()
.into();
Expand Down
7 changes: 4 additions & 3 deletions src/librustdoc/clean/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1306,7 +1306,7 @@ impl WherePredicate {
pub(crate) enum GenericParamDefKind {
Lifetime { outlives: Vec<Lifetime> },
Type { did: DefId, bounds: Vec<GenericBound>, default: Option<Box<Type>>, synthetic: bool },
Const { ty: Box<Type>, default: Option<Box<String>> },
Const { ty: Box<Type>, default: Option<Box<String>>, is_host_effect: bool },
}

impl GenericParamDefKind {
Expand All @@ -1326,9 +1326,10 @@ impl GenericParamDef {
Self { name, kind: GenericParamDefKind::Lifetime { outlives: Vec::new() } }
}

pub(crate) fn is_synthetic_type_param(&self) -> bool {
pub(crate) fn is_synthetic_param(&self) -> bool {
match self.kind {
GenericParamDefKind::Lifetime { .. } | GenericParamDefKind::Const { .. } => false,
GenericParamDefKind::Lifetime { .. } => false,
GenericParamDefKind::Const { is_host_effect, .. } => is_host_effect,
GenericParamDefKind::Type { synthetic, .. } => synthetic,
}
}
Expand Down
4 changes: 4 additions & 0 deletions src/librustdoc/clean/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,10 @@ pub(crate) fn ty_args_to_args<'tcx>(
arg: index,
}),
))),
// FIXME(effects): this relies on the host effect being called `host`, which users could also name
// their const generics.
// FIXME(effects): this causes `host = true` and `host = false` generics to also be emitted.
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.)

GenericArgKind::Const(ct) => {
Some(GenericArg::Const(Box::new(clean_middle_const(kind.rebind(ct), cx))))
}
Expand Down
3 changes: 1 addition & 2 deletions src/librustdoc/html/format.rs
Original file line number Diff line number Diff line change
Expand Up @@ -250,8 +250,7 @@ impl clean::Generics {
cx: &'a Context<'tcx>,
) -> impl fmt::Display + 'a + Captures<'tcx> {
display_fn(move |f| {
let mut real_params =
self.params.iter().filter(|p| !p.is_synthetic_type_param()).peekable();
let mut real_params = self.params.iter().filter(|p| !p.is_synthetic_param()).peekable();
if real_params.peek().is_none() {
return Ok(());
}
Expand Down
16 changes: 9 additions & 7 deletions src/librustdoc/json/conversions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -453,7 +453,7 @@ impl FromWithTcx<clean::GenericParamDefKind> for GenericParamDefKind {
default: default.map(|x| (*x).into_tcx(tcx)),
synthetic,
},
Const { ty, default } => GenericParamDefKind::Const {
Const { ty, default, is_host_effect: _ } => GenericParamDefKind::Const {
type_: (*ty).into_tcx(tcx),
default: default.map(|x| *x),
},
Expand Down Expand Up @@ -491,12 +491,14 @@ impl FromWithTcx<clean::WherePredicate> for WherePredicate {
default: default.map(|ty| (*ty).into_tcx(tcx)),
synthetic,
},
clean::GenericParamDefKind::Const { ty, default } => {
GenericParamDefKind::Const {
type_: (*ty).into_tcx(tcx),
default: default.map(|d| *d),
}
}
clean::GenericParamDefKind::Const {
ty,
default,
is_host_effect: _,
} => GenericParamDefKind::Const {
type_: (*ty).into_tcx(tcx),
default: default.map(|d| *d),
},
};
GenericParamDef { name, kind }
})
Expand Down
1 change: 1 addition & 0 deletions src/librustdoc/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#![feature(array_methods)]
#![feature(assert_matches)]
#![feature(box_patterns)]
#![feature(if_let_guard)]
#![feature(impl_trait_in_assoc_type)]
#![feature(iter_intersperse)]
#![feature(lazy_cell)]
Expand Down
12 changes: 12 additions & 0 deletions tests/rustdoc/const-effect-param.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
#![crate_name = "foo"]
#![feature(effects, const_trait_impl)]

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

// @has foo/fn.g.html
// @has - '//pre[@class="rust item-decl"]' 'pub const fn g<T: Tr>()'
/// foo
pub const fn g<T: ~const Tr>() {}
19 changes: 19 additions & 0 deletions tests/rustdoc/const-fn-effects.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
#![crate_name = "foo"]
#![feature(effects)]

// @has foo/fn.bar.html
// @has - '//pre[@class="rust item-decl"]' 'pub const fn bar() -> '
/// foo
pub const fn bar() -> usize {
2
}

// @has foo/struct.Foo.html
// @has - '//*[@class="method"]' 'const fn new()'
pub struct Foo(usize);

impl Foo {
pub const fn new() -> Foo {
Foo(0)
}
}
5 changes: 5 additions & 0 deletions tests/rustdoc/inline_cross/auxiliary/const-fn.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
#![feature(effects)]

pub const fn load() -> i32 {
0
}
10 changes: 10 additions & 0 deletions tests/rustdoc/inline_cross/const-fn.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// Regression test for issue #116629.
// Check that we render the correct generic params of const fn

// aux-crate:const_fn=const-fn.rs
// edition: 2021
#![crate_name = "user"]

// @has user/fn.load.html
// @has - '//pre[@class="rust item-decl"]' "pub const fn load() -> i32"
pub use const_fn::load;