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

Everybody loops pprint; -Z unstable-options #19964

Merged
merged 4 commits into from
Dec 23, 2014

Conversation

pnkfelix
Copy link
Member

This PR adds a new pretty printing mode, everybody_loops, which replaces every function body in the input with loop { }. I have found this to be useful for building narrow test cases starting from bugs in static analyses like typeck and borrowck that are only exposed initially from complex input source crates like librustc.

The process to follow is:

  1. You first generate the new input with every function body replaced with loop { }, then
  2. you cut-and-paste the original function body that exposes the error,
  3. re-run the compiler on the modified output, to confirm you still see the bug -- but now you should get results faster and with much narrower debug output, since all the other function bodies are trivial.
    4., optional you iteratively delete all the other items that are now found to be no longer necessary since all of the function bodies are now just loop { }.

(Strictly speaking, there are bugs in the pretty printer and/or parser that make the process above not as seamless as described. For example, it seems that use super::{A, B}; can in some contexts be pretty-printed with whitespace separating super and :: and {A, B};, which the parser then errors on. But that is not an argument against the overall technique, which still appears pretty effective, though perhaps it would be even better if combined with macro-expansion in some way.)


Up front: I do believe this sort of local development hack should not want this option to be part of the stable public interface for rustc. So while I could make a -Z flag just for this, I am worried that our -Z infrastructure is generally too weak to express the kinds of options I want to add (see e.g. #19892 where I use the hack of employing environment variables, when I really would have preferred to leverage libgetopts).

Thus, this PR incorporates what I believe to be a reasonable compromise: Add a single new -Z flag, -Z unstable-options, which expands the set of valid options to rustc. This way, we still leverage all of the getopts infrastructure: for example, rustc -Z unstable-options --help prints out information about both the stable and unstable options (while rustc --help without the -Z unstable-options flag just prints out information about the stable options. The main drawback that I can see is that processing such options is a little bit awkward, since you need to make sure that you check sess.debugging_opt(config::UNSTABLE_OPTIONS) before querying the getopts matches for the option in question. But it really is not that bad; all of the ugliest stuff is paid for up front in this PR (e.g. the way I try to ensure that we do not waste time parsing the options twice in the common case where all the provided options are stable).


With -Z unstable-options in place, it is clear that pretty-print modes like everybody_loops belongs under an unstable option, as does the control flow graph visualizing pretty printer. To deal with that, I added a new unstable-option, --xpretty and moved the latter two pretty print modes there.

@pnkfelix pnkfelix changed the title Everybody loops pprint Everybody loops pprint; -Z unstable-options Dec 17, 2014
@pnkfelix
Copy link
Member Author

Note: This series of commits is not quite complete: I still need to migrate the run-make tests for the flowgraph pretty printer to use --xpretty instead of --pretty. (Alternatively, we could leave flow-graph support under --pretty, but I do not think we actually want to do that.)

@pnkfelix
Copy link
Member Author

cc @alexcrichton @brson @nick29581 @cmr

@pnkfelix
Copy link
Member Author

see also PR #19900, which attempted to stabilize some CLI for rustc, namely by moving a number of unstable options behind -Z (in a manner that I do not think will pass the current flowgraph tests in run-make). I suspect it will be easier to develop something like #19900 while keeping all existing in-tree functionality/tests if we have something like -Z unstable-options in place.

@pnkfelix
Copy link
Member Author

(also, there is a reasonable argument against --xpretty everybody_loops that the strategy of starting with the pretty-printed source for a crate and whittling it down incrementally is likely to take longer than building up a narrowed test case from scratch. I do agree that starting from scratch is often the right first step, but if one is unable to reproduce with the initial results from that effort, then it can still be useful to have a way to automatically whittle down the original input in a understandable fashion, like how --xpretty everybody_loops does.)

@alexcrichton
Copy link
Member

I think I like this for top-level flags like --pretty more than #19900 as well, nice idea!

I'm going to rebase #19900 and avoid the --pretty flag for now (to allow this infrastructure to add it). I'm also fine if you don't add xpretty just yet as I was planning on making --pretty unstable anyway.

@alexcrichton
Copy link
Member

r=me once tests are passing on this and you're ready to merge

@brson
Copy link
Contributor

brson commented Dec 19, 2014

Thanks for addressing this @pnkfelix. I like this idea in principle though I haven't looked at the patch.

extend the `rustc` command line interface with options that we do not
want to commit to making part of the long-term public interface for
`rustc`.
This prints out a transformed version of the input source code where
every function body is replaced with `loop { }`.

All such bodies are (1.) trivial and (2.) guaranteed to pass the
type-checker in a valid compiler; therefore they make very nice input
to start with when narrowing down a bug exposed by a large test input
(such as librustc itself).
and `everybody_loops` options to `--xpretty`.
@pnkfelix pnkfelix force-pushed the everybody-loops-pprint branch from 3657207 to bf2f84b Compare December 22, 2014 15:11
@pnkfelix
Copy link
Member Author

cc #19051

bors added a commit that referenced this pull request Dec 22, 2014
…ichton

NOTE: Not yet ready for merge; see my first comment below.  I will remove this note after I post the appropriate follow-on commit to get `run-make` working too.

This PR adds a new pretty printing mode, `everybody_loops`, which replaces every function body in the input with `loop { }`.  I have found this to be useful for building narrow test cases starting from bugs in static analyses like typeck and borrowck that are only exposed initially from complex input source crates like `librustc`.

The process to follow is:
 1. You first generate the new input with every function body replaced with `loop { }`, then
 2. you cut-and-paste the original function body that exposes the error, 
 3. re-run the compiler on the modified output, to confirm you still see the bug -- but now you should get results faster and with much narrower debug output, since all the other function bodies are trivial.
 4., optional you iteratively delete all the other items that are now found to be no longer necessary since all of the function bodies are now just `loop { }`.

(Strictly speaking, there are bugs in the pretty printer and/or parser that make the process above not as seamless as described.  For example, it seems that `use super::{A, B};` can in some contexts be pretty-printed with whitespace separating `super` and `::` and `{A, B};`, which the parser then errors on.  But that is not an argument against the overall technique, which still appears pretty effective, though perhaps it would be even better if combined with macro-expansion in some way.)

----

Up front: I do believe this sort of local development hack should not want this option to be part of the stable public interface for `rustc`.  So while I could make a `-Z` flag just for this, I am worried that our `-Z` infrastructure is generally too weak to express the kinds of options I want to add (see e.g. #19892 where I use the hack of employing environment variables, when I really would have preferred to leverage `libgetopts`).

Thus, this PR incorporates what I believe to be a reasonable compromise: Add a single new `-Z` flag, `-Z unstable-options`, which expands the set of valid options to `rustc`.  This way, we still leverage all of the `getopts` infrastructure: for example, `rustc -Z unstable-options --help` prints out information about both the stable and unstable options (while `rustc --help` without the `-Z unstable-options` flag just prints out information about the stable options.  The main drawback that I can see is that processing such options is a little bit awkward, since you need to make sure that you check `sess.debugging_opt(config::UNSTABLE_OPTIONS)` before querying the `getopts` matches for the option in question.  But it really is not that bad; all of the ugliest stuff is paid for up front in this PR (e.g. the way I try to ensure that we do not waste time parsing the options twice in the common case where all the provided options are stable).

----

With `-Z unstable-options` in place, it is clear that pretty-print modes like `everybody_loops` belongs under an unstable option, as does the control flow graph visualizing pretty printer.  To deal with that, I added a new unstable-option, `--xpretty` and moved the latter two pretty print modes there.
@pnkfelix
Copy link
Member Author

(heh, I forgot to take care of the exact thing I said I would do, namely fixing the run-make tests.)

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Dec 22, 2014
Conflicts:
	src/librustc/session/config.rs
	src/librustc_driver/lib.rs
	src/librustc_driver/pretty.rs
@bors bors merged commit 34d4378 into rust-lang:master Dec 23, 2014
BurntSushi added a commit to docopt/docopt.rs that referenced this pull request Jan 27, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants