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: properly elide cross-crate host effect args #117531

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
3 changes: 2 additions & 1 deletion src/librustdoc/clean/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2534,7 +2534,8 @@ fn clean_generic_args<'tcx>(
}
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
Copy link
Member Author

@fmease fmease Nov 3, 2023

Choose a reason for hiding this comment

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

In #116670 (https://github.com/rust-lang/rust/pull/116670/files#r1357977394) I blamed this code path for /*host*/ true getting leaked in impls which isn't correct 😅. Now I fully understand the relevant parts of the effects lowering code and can confirm that this piece of code is correct. #112463 (sic!) fixed the specific impl issue.

Checking for the presence of #[rustc_host] on the AnonConst is correct and works not just for /*host*/ host but also for the hypothetical /*host*/ true (unreachable atm since true args never get synthesized in the first place) /*host*/ false (always const) (unreachable atm, #117530 would change that).

Copy link
Member

Choose a reason for hiding this comment

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

I'm very tempted to ask you to add this as a code comment. So I'll succumb to the temptation: please add this as a code comment. :D

// Checking for `#[rustc_host]` on the `AnonConst` not only accounts for the case
// where the argument is `host` but for all possible cases (e.g., `true`, `false`).
hir::GenericArg::Const(ct)
if cx.tcx.has_attr(ct.value.def_id, sym::rustc_host) =>
{
Expand Down
6 changes: 1 addition & 5 deletions src/librustdoc/clean/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -124,11 +124,7 @@ pub(crate) fn ty_args_to_args<'tcx>(
)))
}
GenericArgKind::Const(ct) => {
// 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.
if let ty::ConstKind::Param(p) = ct.kind()
&& p.name == sym::host
if let ty::GenericParamDefKind::Const { is_host_effect: true, .. } = params[index].kind
Copy link
Member Author

@fmease fmease Nov 3, 2023

Choose a reason for hiding this comment

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

I could add a test for the fixed issue “impl Trait<true>”. However, it was actually fixed by #112463 (sic!). The /*host*/ false case is unreachable atm, #117530 would change that).

Re. testing host vs #[rustc_host], I don't think it's worth it.

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to have a test, even if pointless. If everything is covered, regressions will be easier to track down.

{
return None;
}
Expand Down
2 changes: 2 additions & 0 deletions tests/rustdoc/const-effect-param.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
// Check that we don't render host effect parameters & arguments.

#![crate_name = "foo"]
#![feature(effects, const_trait_impl)]

Expand Down
16 changes: 16 additions & 0 deletions tests/rustdoc/inline_cross/auxiliary/const-effect-param.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
#![feature(effects, const_trait_impl)]
Copy link
Member Author

Choose a reason for hiding this comment

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

I've renamed the test const-fn.rs to const-effect-param.rs since it's more accurate but git / GitHub doesn't display it as such due to the large number of additional changes in this file.


#[const_trait]
pub trait Resource {}

pub const fn load<R: ~const Resource>() -> i32 {
0
}

pub const fn lock<R: Resource>() {}

#[allow(non_upper_case_globals)]
pub trait Clash<const host: u64> {}

#[allow(non_upper_case_globals)]
pub const fn clash<T: Clash<host>, const host: u64>() {}
5 changes: 0 additions & 5 deletions tests/rustdoc/inline_cross/auxiliary/const-fn.rs

This file was deleted.

29 changes: 29 additions & 0 deletions tests/rustdoc/inline_cross/const-effect-param.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
// Regression test for issue #116629.
// Check that we don't render host effect parameters & arguments.

// aux-crate:const_effect_param=const-effect-param.rs
// edition: 2021
#![crate_name = "user"]

// Don't render the host param on `load` and the host arg `host` passed to `Resource`.
Copy link
Member Author

Choose a reason for hiding this comment

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

This one was already fixed by oli's PR but there was no cross-crate test for it yet, only a local crate one.

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

// Don't render the host arg `true` passed to `Resource`.
Copy link
Member Author

@fmease fmease Nov 5, 2023

Choose a reason for hiding this comment

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

This one was fixed by my “elide cross-crate default generic args” PR which ofc predates oli's PR, hence no test until now. Feature effects synthesizes the param #[rustc_host_param] const host: bool = true for const fn etc, therefore since my PR was merged true gets elided.

This PR introduces a more principled approach and not only elides host and true but arbitrary const values like false. I can't add a test for false since effects doesn't synthesize such code yet. My "generic const items: support effects" PR (#117530) would change that and rustdoc would handle it correctly (I've double-checked that).

// @has user/fn.lock.html
// @has - '//pre[@class="rust item-decl"]' "pub const fn lock<R>()\
// where \
// R: Resource"
pub use const_effect_param::lock;

// Regression test for an issue introduced in PR #116670.
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed by this PR.

// Don't hide the const param `host` since it actually isn't the host effect param.
// @has user/fn.clash.html
// @has - '//pre[@class="rust item-decl"]' \
// "pub const fn clash<T, const host: u64>()\
// where \
// T: Clash<host>"
pub use const_effect_param::clash;
10 changes: 0 additions & 10 deletions tests/rustdoc/inline_cross/const-fn.rs

This file was deleted.

Loading