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

Stabilize -Zunpretty=expanded #43364

Open
joshtriplett opened this issue Jul 20, 2017 · 18 comments
Open

Stabilize -Zunpretty=expanded #43364

joshtriplett opened this issue Jul 20, 2017 · 18 comments
Labels
A-pretty Area: Pretty printing. C-feature-request Category: A feature request, i.e: not implemented / a PR. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-dev-tools Relevant to the dev-tools subteam, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@joshtriplett
Copy link
Member

--pretty=expanded is an incredibly useful debugging tool for macros. The change in rustc 1.19 to stop supporting unstable command-line options on the stable/beta compilers makes it all the more important to stabilize unstable options that people use, and this seems like it easily qualifies.

Much like --emit mir, this should not make any commitments about the output, only that the option should continue to exist. In particular, I'd even suggest making it explicitly clear that there's no commitment for the output to compile.

@shepmaster
Copy link
Member

shepmaster commented Jul 20, 2017

For usage in the playground, it would be lovely if it was actually --emit something, perhaps --emit macro-expanded?

@Mark-Simulacrum Mark-Simulacrum added C-feature-request Category: A feature request, i.e: not implemented / a PR. T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Jul 26, 2017
@nrc
Copy link
Member

nrc commented Aug 10, 2017

I strongly don't want to do this. The generated code is not guaranteed to compile (because hygienic names are not reflected in the generated source text), it means a big chunk of compiler code (pretty printing) is now exposed to stable users, and I worry about bugs, etc. in that code.

I worry that this will encourage tools to try and use that output code, which is absolutely the wrong thing to do.

For debugging macros, we should be able to use expansion info in spans, plus some output saved from macro expansion to give a much better tool - look at every stage of expansion, not just the start and end, have def/ref info for each stage, etc.

@joshtriplett
Copy link
Member Author

joshtriplett commented Aug 10, 2017

@nrc I'd love to see an even more capable mechanism, absolutely, and the idea of stepping through macro expansion step-by-step sounds incredible. But in the interim, --pretty=expanded is what we have for macro debugging, which means stable users have approximately nothing.

If you want to discourage tools from using it, stick an intentional line at the top explaining that the output format isn't stable and prevent it from compiling. But I don't think it's reasonable to take away the only mechanism stable users have for macro debugging, offer no recourse, and not yet have another solution available.

Now, that said, that does mean this will still break cbindgen and other tools which apparently process this code. And that seems quite unfortunate as well, but perhaps more justifiable. But right now, I'm a lot more worried about what to tell stable users who ask "how do I debug my macros?".

@kennytm
Copy link
Member

kennytm commented Aug 12, 2017

@nrc Every-stage-expansion is useful, but just like we have both -Zdump-mir and --emit mir, I don't think that precludes the introduction of --emit expanded/--print expanded.

@nrc nrc added the T-dev-tools Relevant to the dev-tools subteam, which will review and decide on the PR/issue. label Aug 21, 2017
@nrc
Copy link
Member

nrc commented Aug 21, 2017

But right now, I'm a lot more worried about what to tell stable users who ask "how do I debug my macros?".

We tell them to use nightly. Switching compiler with rustup is so easy, that I don't think a stable requirement is a reason to stabilise an otherwise unstable feature.

@nrc
Copy link
Member

nrc commented Aug 21, 2017

I feel like expanded AST is different from mir, because no-one parses the MIR code and expects it to compile (at least as Rust, it would compile as its own language).

@kennytm
Copy link
Member

kennytm commented Aug 21, 2017

@nrc using nightly doesn't always work, as the involving macro may change between rust versions (consider the difference of thread_local! between stable and nightly.)

@ehuss
Copy link
Contributor

ehuss commented Jun 25, 2018

I was wondering if the output isn't compilable, could it at least be parseable? Related to #47287 I came across an issue where a macro expansion results in unparseable output. In particular, I was trying to use cargo expand on the cargo source tree, and rustfmt chokes on this idiom. Not a big deal, one can work around it by not using rustfmt, but unformatted macro expansion can be difficult to read.

// Works
fn f1() -> bool {
    macro_rules! try(( $ e : expr ) => (
        match $ e {
            Some ( e ) => e ,
            None => return false ,
        }
    ));

    try!(Some(42)) == 42
}

// Expanded version doesn't work
fn f2() -> bool {
    match Some(42) {
        Some ( e ) => e ,
        None => return false ,
    } == 42
}

// Could be OK with parenthesis (at least in this case)
fn f3() -> bool {
    (match Some(42) {
        Some ( e ) => e ,
        None => return false ,
    }) == 42
}

@fedochet
Copy link

For debugging macros, we should be able to use expansion info in spans, plus some output saved from macro expansion to give a much better tool - look at every stage of expansion, not just the start and end, have def/ref info for each stage, etc.

@nrc can you please provide more details about how we can retrieve such information (if this is possible)?

@kvark
Copy link
Contributor

kvark commented Jan 18, 2019

Note that the situation needs some kind of solution at the moment. --pretty=expanded is used by cbindgen, which helps gluing together WebRender with Gecko as well as powering other crates, like wgpu-bindings.
If stabilizing the pretty output is not an option, what would be the right way to proceed with making cbindgen robust? cc @eqrion

@eqrion
Copy link
Contributor

eqrion commented Jan 18, 2019

Assuming stabilizing the output is not the right path forward, I believe something like my rust-ffi prototype is a good path forward.

I've been too busy to work on getting it to a MVP though.

@nrc
Copy link
Member

nrc commented Jan 18, 2019

There's also cargo-expand. We'll be looking at adding some support to the RLS which might be a better path to stabilisation that --pretty=expanded

@petrochenkov
Copy link
Contributor

I'd like to revive this issue.
cbindgen is an important tool that must work on stable, if it requires pretty-printing, then so be it.

I think providing --emit expanded working on best effort basis should be acceptable, if we document it.
The pretty-printing output should at least parse successfully, pretty-printer can insert parentheses to maintain parsing priorities for code expanded from macros, if there are cases when it doesn't, then we should treat them as bugs.

@est31
Copy link
Member

est31 commented Oct 10, 2020

There is no need to do a full metadata build of non-build and non-proc-macro dependencies of a crate. Yes, expansion is needed, but expensive stuff like type checking or MIR building isn't. rustc still assumes this though.

In terms of real world tools, this has the effect that cargo expand is as slow as cargo check, even though it only needs expansion of most crates in the DAG and thus could be way faster.

Regarding stabilization this opens the question: would stabilization expose any property of the current code (beyond slowness) that would prevent rustc to be refactored in the future to support faster --emit expanded?

@petrochenkov
Copy link
Contributor

@est31
You've seen the update that I've just posted at https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/Stabilizing.20.60--pretty.3Dexpanded.60/near/212910473?
(I don't think metadata was mentioned previously in this thread.)

I'm pretty sure that we can produce that .rmeta earlier than regular .rmeta, perhaps at cost of some code duplication in rustc and inability to import it as a regular crate.

@jryans
Copy link
Contributor

jryans commented Dec 10, 2020

@rustbot label A-pretty

@rustbot rustbot added the A-pretty Area: Pretty printing. label Dec 10, 2020
@joshtriplett joshtriplett added the I-lang-nominated The issue / PR has been nominated for discussion during a lang team meeting. label Feb 13, 2022
@joshtriplett
Copy link
Member Author

We discussed this in today's @rust-lang/lang meeting, and in a previous meeting.

We didn't have any clear consensus on whether we wanted to stabilize an option that produces machine-parseable output. Some team members proposed that we should ideally have a dedicated option for machine parseability, rather than having tools parse Rust code. In general, there was a fair bit of trepidation about supporting machine-parseable output.

At the very least, we had general consensus that even if we stabilize the option, it should continue to have one or more #![feature(...)] directives emitted at the top, and the presence of those directives would mean that any tool parsing the output was effectively parsing nightly Rust, and subject to the (lack of) stability guarantees of any such syntax.

Human-readable and machine-parsable also pull in two different directions; for instance, machine-parseable output should probably do something easily parseable to handle hygiene, whereas human-readable output should either not handle hygiene, or handle it in a way that's easier for humans to parse (e.g. not just disambiguating symbols by adding large indexes).

That said, there was a lot of sympathy for stabilizing human-readable output here. In particular, it sounded like we might have a consensus for stabilizing the option if we added a nightly feature and a large comment at the top.

@joshtriplett joshtriplett removed the I-lang-nominated The issue / PR has been nominated for discussion during a lang team meeting. label Mar 1, 2022
@jyn514
Copy link
Member

jyn514 commented May 10, 2023

Note that --pretty=expanded has since been renamed to -Zunpretty=expanded: #83491

I am not sure why this was tagged T-lang. It seems pretty clearly T-compiler to me; it does not affect the syntax or semantics of the language.

@jyn514 jyn514 added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label May 10, 2023
@dtolnay dtolnay changed the title Stabilize --pretty=expanded Stabilize -Zunpretty=expanded May 29, 2023
UebelAndre added a commit to bazelbuild/rules_rust that referenced this issue Dec 27, 2023
Proposal for #1642
Duplicates #1643 (special
thanks to @freeformstu)

### Summary

Rustc can be used to expand all macros so that you can inspect the
generated source files easier.

This feature is enabled via `-Zunpretty={mode}`. The `-Z` flag is only
available in the nightly
version of `rustc` (rust-lang/rust#43364).


### Unprettying

Build and test your targets normally.

```
bazel build //:ok_binary   
INFO: Analyzed target //:ok_binary (0 packages loaded, 0 targets configured).
INFO: Found 1 target...
Target //:ok_binary up-to-date:
  bazel-bin/ok_binary
INFO: Elapsed time: 0.081s, Critical Path: 0.00s
INFO: 1 process: 1 internal.
INFO: Build completed successfully, 1 total action
```

Use the aspect to generate the expanded files in as a one-off build.

(`.bazelrc`)
```
# Enable unpretty for all targets in the workspace
build:unpretty --aspects=@rules_rust//rust:defs.bzl%rust_unpretty_aspect
build:unpretty --output_groups=+rust_unpretty

# `unpretty` requires the nightly toolchain. See tracking issue:
# rust-lang/rust#43364
build:unpretty --@rules_rust//rust/toolchain/channel=nightly
```

```
bazel build --config=unpretty //:ok_binary
INFO: Analyzed target //:ok_binary (1 packages loaded, 2 targets configured).
INFO: Found 1 target...
Aspect @rules_rust//rust/private:unpretty.bzl%rust_unpretty_aspect of //:ok_binary up-to-date:
  bazel-bin/ok_binary.expand.rs
INFO: Elapsed time: 0.149s, Critical Path: 0.00s
INFO: 1 process: 1 internal.
INFO: Build completed successfully, 1 total action
```

Targeting tests is valid as well.

```
bazel build --config=unpretty //:ok_test  
INFO: Analyzed target //:ok_test (0 packages loaded, 2 targets configured).
INFO: Found 1 target...
Aspect @rules_rust//rust/private:unpretty.bzl%rust_expand_aspect of //:ok_test up-to-date:
  bazel-bin/test-397521499/ok_test.expand.rs
INFO: Elapsed time: 0.113s, Critical Path: 0.00s
INFO: 1 process: 1 internal.
INFO: Build completed successfully, 1 total action
```

Finally, manually wire up a `rust_unpretty` target explicitly if you
want a target to build. This rule is unique compared to the aspect in
that it forces a transition to a nightly toolchain so that `-Zunpretty`
can be used.

```starlark
load("@rules_rust//rust:defs.bzl", "rust_binary", "rust_unpretty")

rust_binary(
    name = "ok_binary",
    srcs = ["src/main.rs"],
    edition = "2021",
)

rust_unpretty(
    name = "ok_binary_expand",
    deps = [":ok_binary"],
)
```

```
bazel build //:ok_binary_expand
INFO: Analyzed target //:ok_binary_expand (0 packages loaded, 1 target configured).
INFO: Found 1 target...
Target //:ok_binary_expand up-to-date:
  bazel-bin/ok_binary.expand.rs
INFO: Elapsed time: 0.090s, Critical Path: 0.00s
INFO: 1 process: 1 internal.
INFO: Build completed successfully, 1 total action
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-pretty Area: Pretty printing. C-feature-request Category: A feature request, i.e: not implemented / a PR. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-dev-tools Relevant to the dev-tools subteam, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests