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 renders re-exported `async fn`s incorrectly #63710

Open
carllerche opened this issue Aug 19, 2019 · 6 comments · May be fixed by #64599

Comments

@carllerche
Copy link
Member

commented Aug 19, 2019

Relates to #58027, but this is the case where an async fn is re-exported by another crate. The docs for the original function shows async fn but the re-export shows fn -> impl Future.

Example:

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2019

Check-in from the triage meeting:

I'm going to mark this as "blocking" although it isn't really. It does seem like a good "polish" item to try and nail down before async-await hits beta, though it's ultimately a "nice to have".

@estebank

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2019

Should we also consider always documenting Fn() -> impl Future<Output=_> as async Fn() -> _?

@hirschenberger

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2019

After some codereview I found that code.

There seems to be a reset of the asyncness when the function gets duplicated. Unfortunately I don't know from where I can get the asyncness information for hir::FnHeader there. There does not seem to be any information on that in sty::PolyFnSig.

@csmoe

This comment has been minimized.

Copy link
Member

commented Sep 17, 2019

@rustbot claim

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2019

Mentoring instructions

Thanks, @hirschenberger, for the tip, I think that's exactly the right spot in the code. We basically want to copy the pathways in the code that we use for tracking (e.g.) whether a function is a const fn.

For that case, you can see in the code that we invoke the tcx query is_min_const_fn to check:

let constness = if cx.tcx.is_min_const_fn(did) {
hir::Constness::Const
} else {
hir::Constness::NotConst
};

so we're going to want something similar, like let asyncness = tcx.fn_asyncness(...). Here I changed the query to return a hir::Asyncness type instead of a bool, but it's easy enough either way.

But first we need to add that is_async_fn query. We can look at is_min_const_fn as a model, although it has a few complications. In particular, is_min_const_fn turns out to be a regular tcx method that invokes the is_const_fn_raw helper (and then does some other stuff). We're more interested in the is_const_fn_raw model, since we don't need that other stuff.

is_const_fn_raw is defined in the query/mod.rs file:

/// Returns `true` if this is a const fn, use the `is_const_fn` to know whether your crate
/// actually sees it as const fn (e.g., the const-fn-ness might be unstable and you might
/// not have the feature gate active).
///
/// **Do not call this function manually.** It is only meant to cache the base data for the
/// `is_const_fn` function.
query is_const_fn_raw(key: DefId) -> bool {
desc { |tcx| "checking if item is const fn: `{}`", tcx.def_path_str(key) }
}

The "provider" (function that gets called) for local functions is given in ty/constness.rs:

/// only checks whether the function has a `const` modifier
fn is_const_fn_raw(tcx: TyCtxt<'_>, def_id: DefId) -> bool {
let hir_id = tcx.hir().as_local_hir_id(def_id)
.expect("Non-local call to local provider is_const_fn");
let node = tcx.hir().get(hir_id);
if let Some(fn_like) = FnLikeNode::from_node(node) {
fn_like.constness() == hir::Constness::Const
} else if let hir::Node::Ctor(_) = node {
true
} else {
false
}
}

and installed into the provider struct down below:

*providers = Providers {
is_const_fn_raw,
is_promotable_const_fn,
const_fn_is_allowed_fn_ptr,
..*providers
};

The tricky part comes when the function is loaded from metadata from another crate. That requires us to edit the librustc_metadata modules. The first step would be to extend the schema:

#[derive(RustcEncodable, RustcDecodable)]
pub struct FnData<'tcx> {
pub constness: hir::Constness,
pub param_names: Lazy<[ast::Name]>,
pub sig: Lazy<ty::PolyFnSig<'tcx>>,
}

Here, we would add a field like asyncness: hir::Asyncness. Then we'll have to modify the encoder to provide a value for that field, e.g. here:

FnData {
constness: hir::Constness::NotConst,
param_names,
sig: self.lazy(&tcx.fn_sig(def_id)),
}

Here we would just read it from the HIR. Finally, we have to modify the decoder and "crate store impl" files so that when you invoke the fn_asyncness query on an external crate, it gets routed the right way. For that we would add a function like is_const_fn_raw to decoder.rs:

crate fn is_const_fn_raw(&self, id: DefIndex) -> bool {
let constness = match self.entry(id).kind {
EntryKind::Method(data) => data.decode(self).fn_data.constness,
EntryKind::Fn(data) => data.decode(self).constness,
EntryKind::Variant(..) | EntryKind::Struct(..) => hir::Constness::Const,
_ => hir::Constness::NotConst,
};
constness == hir::Constness::Const
}

and then add an entry to the macro which invokes this function when the query is called, kind of like this:

is_const_fn_raw => { cdata.is_const_fn_raw(def_id.index) }

This macro basically says "when this tcx query is called on something from a foreign crate, invoke this method on the cdata variable ('crate data')".

@csmoe csmoe referenced a pull request that will close this issue Sep 19, 2019
@hirschenberger

This comment has been minimized.

Copy link
Contributor

commented Sep 19, 2019

Oh, seems I was too late 😞

@nikomatsakis But thanks for the tried mentoring

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.