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

Tracking issue for accepting unstable flags in stable compilers #31847

Closed
alexcrichton opened this issue Feb 23, 2016 · 46 comments · Fixed by #41751
Closed

Tracking issue for accepting unstable flags in stable compilers #31847

alexcrichton opened this issue Feb 23, 2016 · 46 comments · Fixed by #41751
Labels
B-unstable Blocker: Implemented in the nightly compiler and unstable. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@alexcrichton
Copy link
Member

This issue is intended to track the process of fixing rustc to not accept unstable flags on stable compilers. It's somewhat of a historical mistake that this was allowed, and we're attempting now to repair the logic to disallow unstable flags on the compiler. Today the compiler has a few unstable flags:

  • -Z - all debugging options in the compiler are intended to be unstable
  • --pretty - the pretty printer is quite broken in various ways, and it's output is also generally quite unstable
  • --unpretty - the flag and formats emitted here were not intended to be stable
  • --error-format - this support is currently experimental in the compiler and may not be ready for prime-time

For backwards compatibility the compiler will continue to accept these flags on the stable and beta channels of Rust. In #31793, however, the compiler will start to issue a warning on these channels whenever these flags are passed. The warning message points at this issue.

The next steps on this issue are likely:

  • Wait for the warning to propagate to the stable channel
  • Get feedback about which unstable flags are widely used today
  • Resolve how to stabilize widely used unstable flags
  • Wait for these flags to be stable
  • Turn the warning of unstable flags on the stable channel into a hard error

If you're coming to this issue via the compiler warning, please feel free to comment with your flag and use case! We're eager to learn which unstable flags are most widely used to prioritize what to stabilize.

@alexcrichton alexcrichton added T-tools T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. B-unstable Blocker: Implemented in the nightly compiler and unstable. labels Feb 23, 2016
@Zoxc
Copy link
Contributor

Zoxc commented Feb 23, 2016

Can -C link-args, -C llvm-args join this list? Both need a replacement which acceps spaces (#30947)

@alexcrichton
Copy link
Member Author

@Zoxc for now, no, those are both stable flags and this is just targeted at unstable flags.

@durka
Copy link
Contributor

durka commented Feb 23, 2016

For myself I use -Z trace-macros and --pretty=expanded for debugging macros, though both need improvements in their output.

I also use -Z no-trans for a faster compile-edit cycle, so I can fix all the errors before even trying the slow translation/linking steps. And I use cargo clippy for extra lints, which uses this flag internally (I think cargo check does too). Something like it should probably be stabilized with a different name.

cargo clippy also uses -Z extra-plugins.

@jrvidal
Copy link
Contributor

jrvidal commented Mar 4, 2016

I also use -Z no-trans for a faster compile-edit cycle

+1 on this.

(I'm guessing "+1" comments are fine in this thread...?)

@nagisa
Copy link
Member

nagisa commented Mar 4, 2016

--pretty and --unpretty should probably be -Z pretty/unpretty instead. cc @nikomatsakis

@mkpankov
Copy link
Contributor

Note that passing -Z no-trans disables some errors. In particular, type size comparison done when checking if std::mem::transmute should be allowed is not performed.

See this example: https://is.gd/cnjaFI . When run locally with -Z no-trans, no errors are reported.

Also, I'd like to point out that many linters are using -Z no-trans. This itself doesn't justify its stabilization, especially considering the example above that isn't checked correctly with the option. But, there probably should be another, stable, option, then, which would serve the purpose of linting. It should check for all possible errors and drop the translation as soon as all possible. Or should we wait for linting support in IDE oracle?

@psFried
Copy link

psFried commented Jun 10, 2016

Regardless of whether -Z no-trans disables some checks, I think it's still very nice to have in the stable compiler. Ideally, the long term solution would be for linters and IDE's to use the oracle once it's ready, but I think it's important in the mean time to have a fast check that catches most compile errors. This is especially important for those who are new to the language and will ofter require many revisions in order to get their code compiling.

@nikomatsakis
Copy link
Contributor

@psFried I believe the plan was to make -Z no-trans more official, something like -C check. We should actually do this, however.

@brson
Copy link
Contributor

brson commented Jul 14, 2016

Nominating to consider converting to an error.

@sanxiyn
Copy link
Member

sanxiyn commented Jul 17, 2016

This will break using -Z time-passes with stable compiler.

@durka
Copy link
Contributor

durka commented Jul 19, 2016

Have any of the useful flags people mentioned (no-trans, time-passes,
pretty-expanded, trace-macros) been moved to stable
equivalents/alternatives? (I think the answer is no.) If not, I think we
shouldn't make this an error yet, since (supposedly) the intent was to keep
useful functionality available, while being mindful about what gets
stabilized. Without that it's kind of a bait-and-switch.

On Jul 17, 2016 08:27, "Seo Sanghyeon" [email protected] wrote:

This will break using -Z time-passes with stable compiler.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#31847 (comment),
or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAC3nxuPYQNuceqHf7r-fBIcz7KhkvNLks5qWh--gaJpZM4Hg8-w
.

@alexcrichton
Copy link
Member Author

@sanxiyn yes that's the intention, if it's a desirable flag to be on stable it should be stable.

@durka no, nothing has change, but that's because there doesn't seem to be that much desire to move these flags to stable.

@sanxiyn
Copy link
Member

sanxiyn commented Jul 19, 2016

#34858 is an example of using -Z time-passes with stable, in this case to diagnose hang.

@durka
Copy link
Contributor

durka commented Jul 19, 2016

I mean, why even ask in this issue for people's use cases if the answers
weren't going to matter? Sure, go ahead and error everything out and see
who complains.

On Jul 19, 2016 07:12, "Seo Sanghyeon" [email protected] wrote:

#34858 #34858 is an example of
using -Z time-passes with stable, in this case to diagnose hang.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#31847 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAC3n-21HmArc8LDMrpq5fPpdyLuZ0kUks5qXLEtgaJpZM4Hg8-w
.

@nikomatsakis
Copy link
Contributor

Hmm, I think we should considering making a stable variant of -Z no-trans at least -- and probably the other flags too.

Regarding the other flags that were mentioned (time-passes, pretty-expanded, trace-macros):

All of these have in common that I feel more comfortable with stabilizing the ability to access said functionality than I do with stabilizing anything about the output. For example, I'd expect time-passes to look different over time (obviously, the set of passes will change, but also maybe the data we collect). Similary, pretty-expanded relies on our pretty printer, which I hope we will remove someday: I could imagine a new version looking quite different, or emitting only JSON, or something else.

What this means is that humans using these switches would probably be fine, but tools would break. Do we think we can realistically declare a stability level of that kind? I guess it is the same as (for example) our general error output, which we expect to change. (We probably want to consider explicit scripting commands like git offers.)

@pnkfelix
Copy link
Member

Compiler team's current thinking: Its too soon to switch off all support for all current -Z flags. There is a set of useful flags for debugging that we have no means of expression via -C or other stable interface.

But something we may be able to do in the short term is white list a small subset of -Z flags to be still accepted in nightly (as today), and make all others an error.

What small subset? We suggest starting with any that appear in comments on this thread. Get to it people!!! Add them below if not already above!

@arielb1
Copy link
Contributor

arielb1 commented Jul 21, 2016

The contentious flags are these used for debugging user code. I don't feel like they should have the same stability semantics as normal flags. Maybe have -Z debug-mode or something.

@nikomatsakis
Copy link
Contributor

It does seem like there is room to have flags that, at minimum, have undefined output. Which effectively means they are unstable and can't really be used for scripting. I am thinking of things like gcc's "dump ast" flags (gcc -fdump-tree-vcg -g test.c).

@pwoolcoc
Copy link
Contributor

If there is an answer to how we want to stabilize -Z no-trans I would love to use it as an "entry-level" PR for rustc, if yinz don't mind.

@durka
Copy link
Contributor

durka commented Feb 11, 2017 via email

@shepmaster
Copy link
Member

MIR even if the actual text form completely changes?

I guess we should also consider that there might come a time where MIR no longer even exists (it didn't in Rust 1.0, afterall! 😸). In that case, I even think it would be fine to print the text "MIR was removed in Rust 1.99, nothing to see here!".

a way to stabilize options without stabilizing their output format.

Pedantically, we say we output LLVM IR, but we certainly don't control the format of that. And it's already changed incompatibly as Rust has upgraded LLVM over time. Maybe we are worrying more than we need to be?

@nagisa
Copy link
Member

nagisa commented Feb 12, 2017

The whole point of emiting MIR is for debugging. --unpretty=mir has little use to compiler devs by now, because -Z dump-mir simply provides more information. In that sense its in the same position as -Ztime-passes – useful for debugging the compiler and nothing else. --unpretty=mir is most likely to just get merged into -Z dump-mir (incompatible output format, files instead stdout).

Comparing MIR output to LLVM-IR/LLVM-BC is unfair here IMO and it should rather be compared with the --emit=obj instead. Why? Because in both cases this output, when paired with an appropriate version of a tool, will end up in asm or object. In that sense LLVM-IR/BC output is stable. There are no (and never should be any) tools for parsing the MIR dumps.

@shepmaster
Copy link
Member

--unpretty=mir has little use to compiler devs by now, because -Z dump-mir simply provides more information

I'm totally OK with whichever format...

incompatible output format, files instead stdout)

But for my own selfish reasons, I'd prefer that the MIR for the entire crate be output into a single file (just like LLVM-IR and ASM), as well as being triggered by --emit=.... This is just to make my life easier.

There are no (and never should be any) tools for parsing the MIR dumps.

How picky should we be about this aspect? The official playground already parses MIR to some extent.

@pnkfelix
Copy link
Member

pnkfelix commented Feb 17, 2017

@rfcbot concern bootstrap-rustc-builds

with the change being planned here, will the -Z and --pretty/--unpretty flags continue to be available with the bootstrap rustc builds?

There are occasional times where I need to resort to looking at expanded output annotated with node-ids in order to make sense of ICE's that occur from a bootstrap compiler that I am in the process of constructing.

@eddyb
Copy link
Member

eddyb commented Feb 17, 2017

Nowadays all you have to do is set RUSTC_BOOTSTRAP=1.

@pnkfelix
Copy link
Member

@rfcbot resolved bootstrap-rustc-builds

(I'm assuming that whatever else is happening based on the change proposed here, RUSTC_BOOTSTRAP=1 will remain as a way to bypass the stability checks and allow one to again use -Z whatever.)

@MJDSys
Copy link
Contributor

MJDSys commented Mar 17, 2017

One problem I noticed with this issue is that Cargo doesn't detect what version of rust is running before passing -Z flags.

As a real example, I've enabled CARGO_INCREMENTAL on one of my computers, and it recently ran into a bug on the nightly compiler. I've fallen back to the stable compiler for now, but I get warning about -Zincremental being passed due to this issue. It would be good if Cargo avoided passing -Z flags to rust before it becomes a hard error.

Would it help if I filed an issue about this against cargo?

@SimonSapin
Copy link
Contributor

@MJDSys see rust-lang/cargo#3835

frewsxcv added a commit to frewsxcv/rust that referenced this issue Mar 23, 2017
Teach rustc --emit=mir

I'm opening this PR to discuss:

1. Is this a good idea?
1. Is this a good implementation?

I'm sure people will have opinions on both points!

This spawned from rust-lang#31847 (comment), so I figured a prototype implementation could help provide a seed to talk about.
@est31
Copy link
Member

est31 commented May 4, 2017

See also issue #41743 .

@alexcrichton
Copy link
Member Author

All major stakeholders have checked in with checkboxes above, and this was re-reviewed during tools triage yesterday, and I've now opened a PR: #41751

@durka
Copy link
Contributor

durka commented May 4, 2017

Removing --pretty also breaks cargo expand.

alexcrichton added a commit to alexcrichton/rust that referenced this issue May 4, 2017
First deprecated in rustc 1.8.0 the intention was to never allow `-Z` flags make
their way to the stable channel (or unstable options). After a year of warnings
we've seen one of the main use cases, `-Z no-trans`, stabilized as `cargo
check`. Otherwise while other use cases remain the sentiment is that now's the
time to start forbidding `-Z` by default on stable/beta.

Closes rust-lang#31847
@alexcrichton
Copy link
Member Author

@durka we've spent the last year printing that this is going to become an error. cargo expand will still work on nightly, as is expected.

frewsxcv added a commit to frewsxcv/rust that referenced this issue May 5, 2017
rustc: Forbid `-Z` flags on stable/beta channels

First deprecated in rustc 1.8.0 the intention was to never allow `-Z` flags make
their way to the stable channel (or unstable options). After a year of warnings
we've seen one of the main use cases, `-Z no-trans`, stabilized as `cargo
check`. Otherwise while other use cases remain the sentiment is that now's the
time to start forbidding `-Z` by default on stable/beta.

Closes rust-lang#31847
bors added a commit that referenced this issue May 5, 2017
rustc: Forbid `-Z` flags on stable/beta channels

First deprecated in rustc 1.8.0 the intention was to never allow `-Z` flags make
their way to the stable channel (or unstable options). After a year of warnings
we've seen one of the main use cases, `-Z no-trans`, stabilized as `cargo
check`. Otherwise while other use cases remain the sentiment is that now's the
time to start forbidding `-Z` by default on stable/beta.

Closes #31847
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-unstable Blocker: Implemented in the nightly compiler and unstable. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.