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

pub macro defined in submodule is shown at the wrong path #74355

Closed
RalfJung opened this issue Jul 15, 2020 · 16 comments · Fixed by #77862
Closed

pub macro defined in submodule is shown at the wrong path #74355

RalfJung opened this issue Jul 15, 2020 · 16 comments · Fixed by #77862
Labels
A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) A-macros-2.0 Area: Declarative macros 2.0 (#39412) C-bug Category: This is a bug. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

RalfJung commented Jul 15, 2020

rustdoc is showing the raw_mut macro at the wrong path in libcore: https://doc.rust-lang.org/nightly/core/macro.raw_mut.html says core::raw_mut but really it is core::ptr::raw_mut (and the same goes for core::ptr::raw_const). Looks like pub macro is not handled correctly?

Cc @rust-lang/rustdoc

@petrochenkov
Copy link
Contributor

cc #63268

@GuillaumeGomez
Copy link
Member

When a macro is marked as reexported, you can use it directly from the "top", so in this regard, isn't it the expected behavior?

@GuillaumeGomez GuillaumeGomez added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Jul 15, 2020
@RalfJung
Copy link
Member Author

AFAIK that is not true for macro macros, just for macro_rules! macros.

@RalfJung
Copy link
Member Author

RalfJung commented Jul 15, 2020

Note that there is no #[macro_export] here. This macro is exported via pub, like normal items (functions, structs, ...).

@RalfJung
Copy link
Member Author

Code example:

#![feature(raw_ref_macros)]

fn main() {
    let mut x = 5;
    // This works.
    let xptr = core::ptr::raw_const!(x);
    // This does not.
    let xptr = core::raw_const!(x);
}

So I think there is definitely a rustdoc bug here. The path it shows does not work.

@GuillaumeGomez
Copy link
Member

Oh I see. Indeed, it's a bug.

@yoshuawuyts
Copy link
Member

yoshuawuyts commented Jul 15, 2020

Not sure if helpful, but the way we got public exports to work in rustdoc in async-std was by creating a separate crate (async-macros), and then re-exporting from from the main crate. This enabled us to render async_std::task::ready! in rustdoc.

This issue will also affect the upcoming core::task::ready! PR.

@RalfJung
Copy link
Member Author

@GuillaumeGomez how hard is it to fix this problem, and what does the rustdoc team's prioritization for this look like? We'd like to stabilize #73394, and I am not sure if we want to stabilize this while its docs are rendered incorrectly.

@GuillaumeGomez
Copy link
Member

Sorry but no idea. I don't have much time lately so maybe someone else from the @rust-lang/rustdoc will give it a try? Otherwise I'll try to fix it as soon as I have time.

@jyn514 jyn514 added the C-bug Category: This is a bug. label Jul 27, 2020
@Manishearth
Copy link
Member

Yeah so the problem here is that we have TypeKind::Macro which forces all macros to live at the root.

Given a Def or a Res, how do I tell if a macro is MBE or a newstyle macro?

(There's a possibility that our current code actually handles this correctly already and we need to remove the special case for TypeKind::Macro)

@Manishearth
Copy link
Member

Manishearth commented Jul 28, 2020

Okay so here are the two offending bits of code:

module
.macros
.extend(krate.exported_macros.iter().map(|def| self.visit_local_macro(def, None)));

let fqn = if let clean::TypeKind::Macro = kind {
vec![crate_name, relative.last().expect("relative was empty")]
} else {
once(crate_name).chain(relative).collect()
};

The latter is more about linking to these items rather than rendering them.

So the problem is that by the time stuff gets to rustdoc, we're looking at the HIR and the two kinds of macros are squashed together on hir::Crate::exported_macros. The newstyle macros are not a variant of hir::Item at all, they disappear from the HIR tree at that point and only exist as an array on the crate.

It would be nice if MBEs stayed on krate and newstyle macros were actually Items or at least stored separately on the module. We could handle these as we see their containing module, but that seems prone to issues when there are reexports and private modules in the mix.

I think having newstyle macros live on hir::Crate rather than hir::Mod is a mistake.

@Manishearth
Copy link
Member

An option to do this purely in rustdoc would be to check the def_path and insert it in the appropriate modules after the fact, however this is kinda icky and i don't think this will be the last tooling problem encountered by newstyle macros living at the crate root in HIR.

@RalfJung
Copy link
Member Author

RalfJung commented Jul 28, 2020

@petrochenkov will probably know best how to interpret this information; somehow macro name resolution also has to work with this, after all.

@Manishearth
Copy link
Member

Manishearth commented Jul 28, 2020

I don't think name resolution actually operates on the hir that way. But yeah, worth letting him answer.

@jyn514 jyn514 added A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) A-macros-2.0 Area: Declarative macros 2.0 (#39412) labels Aug 25, 2020
@jyn514
Copy link
Member

jyn514 commented Sep 29, 2020

The newstyle macros are not a variant of hir::Item at all, they disappear from the HIR tree at that point and only exist as an array on the crate.

I think you can recover the module they came from using the HirId and calling parent_module on it.

@jyn514
Copy link
Member

jyn514 commented Sep 29, 2020

You can tell whether it's a macro_rules macro or a pub macro by looking at MacroDef.macro_rules.

bors added a commit to rust-lang-ci/rust that referenced this issue Jan 10, 2021
…s_2_0-paths, r=jyn514,petrochenkov

Rustdoc: Fix macros 2.0 and built-in derives being shown at the wrong path

Fixes rust-lang#74355

  - ~~waiting on author + draft PR since my code ought to be cleaned up _w.r.t._ the way I avoid the `.unwrap()`s:~~

      - ~~dummy items may avoid the first `?`,~~

      - ~~but within the module traversal some tests did fail (hence the second `?`), meaning the crate did not possess the exact path of the containing module (`extern` / `impl` blocks maybe? I'll look into that).~~

r? `@jyn514`
@bors bors closed this as completed in 0bee802 Jan 10, 2021
JohnTitor added a commit to JohnTitor/rust that referenced this issue Jan 30, 2021
…-ou-se

Stabilize raw ref macros

This stabilizes `raw_ref_macros` (rust-lang#73394), which is possible now that rust-lang#74355 is fixed.

However, as I already said in rust-lang#73394 (comment), I am not particularly happy with the current names of the macros. So I propose we also change them, which means I am proposing to stabilize the following in `core::ptr`:
```rust
pub macro const_addr_of($e:expr) {
    &raw const $e
}

pub macro mut_addr_of($e:expr) {
    &raw mut $e
}
```

The macro name change means we need another round of FCP. Cc `````@rust-lang/libs`````
Fixes rust-lang#73394
JohnTitor added a commit to JohnTitor/rust that referenced this issue Jul 28, 2021
…=m-ou-se

Stabilize core::task::ready!

_Tracking issue: https://github.com/rust-lang/rust/issues/70922_

This PR stabilizes the `task::ready!` macro. Similar to rust-lang#80886, this PR was waiting on rust-lang#74355 to be fixed.

The `task::ready!` API has existed in the futures ecosystem for several years, and was added on nightly last year in rust-lang#70817. The motivation for this macro is the same as it was back then: virtually every single manual future implementation makes use of this; so much so that it's one of the few things included in the [futures-core](https://docs.rs/futures-core/0.3.12/futures_core) library.

r? `@tmandry`

cc/ `@rust-lang/wg-async-foundations` `@rust-lang/libs`

## Example
```rust
use core::task::{Context, Poll};
use core::future::Future;
use core::pin::Pin;

async fn get_num() -> usize {
    42
}

pub fn do_poll(cx: &mut Context<'_>) -> Poll<()> {
    let mut f = get_num();
    let f = unsafe { Pin::new_unchecked(&mut f) };

    let num = ready!(f.poll(cx));
    // ... use num

    Poll::Ready(())
}
```
JohnTitor added a commit to JohnTitor/rust that referenced this issue Jul 28, 2021
…=m-ou-se

Stabilize core::task::ready!

_Tracking issue: https://github.com/rust-lang/rust/issues/70922_

This PR stabilizes the `task::ready!` macro. Similar to rust-lang#80886, this PR was waiting on rust-lang#74355 to be fixed.

The `task::ready!` API has existed in the futures ecosystem for several years, and was added on nightly last year in rust-lang#70817. The motivation for this macro is the same as it was back then: virtually every single manual future implementation makes use of this; so much so that it's one of the few things included in the [futures-core](https://docs.rs/futures-core/0.3.12/futures_core) library.

r? ``@tmandry``

cc/ ``@rust-lang/wg-async-foundations`` ``@rust-lang/libs``

## Example
```rust
use core::task::{Context, Poll};
use core::future::Future;
use core::pin::Pin;

async fn get_num() -> usize {
    42
}

pub fn do_poll(cx: &mut Context<'_>) -> Poll<()> {
    let mut f = get_num();
    let f = unsafe { Pin::new_unchecked(&mut f) };

    let num = ready!(f.poll(cx));
    // ... use num

    Poll::Ready(())
}
```
eggyal pushed a commit to eggyal/copse that referenced this issue Jan 9, 2023
Stabilize raw ref macros

This stabilizes `raw_ref_macros` (rust-lang/rust#73394), which is possible now that rust-lang/rust#74355 is fixed.

However, as I already said in rust-lang/rust#73394 (comment), I am not particularly happy with the current names of the macros. So I propose we also change them, which means I am proposing to stabilize the following in `core::ptr`:
```rust
pub macro const_addr_of($e:expr) {
    &raw const $e
}

pub macro mut_addr_of($e:expr) {
    &raw mut $e
}
```

The macro name change means we need another round of FCP. Cc `````@rust-lang/libs`````
Fixes #73394
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) A-macros-2.0 Area: Declarative macros 2.0 (#39412) C-bug Category: This is a bug. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants