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

Ergonomic format_args! #33642

Merged
merged 10 commits into from
Jul 13, 2016
Merged

Ergonomic format_args! #33642

merged 10 commits into from
Jul 13, 2016

Conversation

xen0n
Copy link
Contributor

@xen0n xen0n commented May 14, 2016

Fixes #9456 (at last).

Not a ground-up rewrite of the existing machinery, but more like an added intermediary layer between macro arguments and format placeholders. This is now implementing Rust RFC 1618!

@rust-highfive
Copy link
Collaborator

r? @aturon

(rust_highfive has picked a reviewer for you, use r? to override)

@xen0n
Copy link
Contributor Author

xen0n commented May 15, 2016

This caused some small regressions here and there. I'll be slowly fixing them; compilation time is very long.

Non-regressions, just TODO:

  • compile-fail/ifmt-bad-arg.rs
  • run-pass/ifmt.rs

Noticed regressions:

  • compile-fail/macro-backtrace-println.rs
  • doctest fmt_3 ({:.*} didn't eat the precision argument)

@xen0n
Copy link
Contributor Author

xen0n commented May 16, 2016

Implemented correct implicit args handling by resolving all CountIsNextParams and ArgumentNexts into absolute references during parse. Which means, for example, {foo:.*} {} {1:.*} {:.*} {bar:.*} now parses exactly like {foo:.0$} {1} {1:.2$} {4:.3$} {bar:.5$}. Which obviates the need to handle any next-references during expansion and outputting; will remove these later, after I finish up an RFC for this PR later this week.

EDIT: Whoops, CountIsParam(i) should be CountIsNextParam. CountIsParam(i) is also touched but it's not related to resolution (it's expansion).

eddyb added a commit to eddyb/rust that referenced this pull request May 17, 2016
…tsakis

syntax_ext: format: nest_level's are no more

Just noticed this while working on rust-lang#33642 and here's a quick fix, shouldn't touch anything else. It's some historic code indeed...
@aturon
Copy link
Member

aturon commented May 17, 2016

r? @alexcrichton

@rust-highfive rust-highfive assigned alexcrichton and unassigned aturon May 17, 2016
Manishearth added a commit to Manishearth/rust that referenced this pull request May 18, 2016
…tsakis

syntax_ext: format: nest_level's are no more

Just noticed this while working on rust-lang#33642 and here's a quick fix, shouldn't touch anything else. It's some historic code indeed...
@alexcrichton
Copy link
Member

Thanks for the PR! I'll take a closer look at this once rust-lang/rfcs#1618 is handled

@xen0n xen0n force-pushed the ergonomic-format-macro branch 3 times, most recently from 93b7b3f to 9614a17 Compare May 21, 2016 15:13
@xen0n
Copy link
Contributor Author

xen0n commented May 21, 2016

Fixed test cases broken by this PR, and added new cases as well.

Currently make check fails on several unrelated tests:

  • run-pass/issue-28950.rs
  • run-pass/panic-runtime/lto-abort.rs
  • run-pass/panic-runtime/lto-unwind.rs
  • run-pass/sepcomp-lib-lto.rs

I'm not sure why those keep popping up, it wasn't the case several days ago. Testing master branch right now, will edit this once finished.

Update: master fails on those tests too. Pretty sure these aren't my fault. But again it may be my environment... I have to sleep now though, it's 3 am, too tired to launch yet another investigation.

Another update: I recompiled without --enable-debug and the failures were gone. Surely there is some nasty interaction between LTO and debug code generation. But at least I could sanely continue my work. 😞

@xen0n xen0n changed the title [WIP] [DO NOT MERGE] Ergonomic format macro [WIP] Ergonomic format_args! May 21, 2016
@xen0n
Copy link
Contributor Author

xen0n commented May 22, 2016

Travis test completed without the unrelated failures, seems there is a pretty-print round-trip failure:

--- pretty.expected.rs  2016-05-22 13:39:14.080831211 +0800
+++ pretty.actual.rs    2016-05-22 13:38:56.912831929 +0800
@@ -178,7 +178,6 @@
        , b = "hijklmn" , c = 3 ) , "abcd hijk 4\nabc hij 3");
     t!(format ! ( "{a:.*} {0} {:.*}" , 4 , 3 , "efgh" , a = "abcdef" ) ,
        "abcd 4 efg");
-
     // Test that pointers don't get truncated.
     {
         let val = usize::MAX;

Apart from this and deduplication of argument objects, the feature feels almost ready. Waiting for RFC resolution 😄

@alexcrichton
Copy link
Member

Ah yeah unfortunately we've had bad luck with debuginfo and LTO in the past (lots of weird LLVM assertions), but thanks for the progress update!

@xen0n
Copy link
Contributor Author

xen0n commented Jun 1, 2016

I immediately gave up fixing the pretty print issue after trying to produce a minimal test case and got this:

macro_rules! t {
    ($a:expr, $b:expr) => { assert_eq!($a, $b) }
}

pub fn main() {
    t!(format!("{:x}{:X}{:x}{:X}{:x}{a:X}",1,2,3,4,5,a=15), "abcdefghijkl");




    // every time this code gets pretty-printed, a newline is eaten!
}

Might as well reduce the length of affected lines, as the bug seems to only occur when a line wrap is needed in the output. I'll finish the argument object de-duplication and then we can start reviewing.

@xen0n xen0n force-pushed the ergonomic-format-macro branch 2 times, most recently from b96da3d to 68dd135 Compare June 2, 2016 14:01
@xen0n
Copy link
Contributor Author

xen0n commented Jun 2, 2016

Rebased and cleaned up a little, should be ready for review!

PS: The reason I think an RFC is really needed, is that some serious documentation changes and maybe deprecation inside the formatting runtime will have to be done. However as the RFC is already submitted and waiting through its FCP this point is already moot.

@xen0n xen0n changed the title [WIP] Ergonomic format_args! Ergonomic format_args! Jun 2, 2016
@xen0n xen0n force-pushed the ergonomic-format-macro branch 2 times, most recently from dea6bfd to 7037411 Compare June 3, 2016 14:10
// named arg only used as count
t!(format!("{0:.a$}", "aaaaaa", a=2), "aa");


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test here as well with side effects on the evaluated expression to ensure it's only calculated once even though it may be formatted multiple times.

@bors
Copy link
Contributor

bors commented Jul 4, 2016

☔ The latest upstream changes (presumably #34638) made this pull request unmergeable. Please resolve the merge conflicts.

@xen0n
Copy link
Contributor Author

xen0n commented Jul 4, 2016

Rebased again and (finally found time to do so!) added comments to facilitate better understanding. Hope this is not too late for 1.10!

@alexcrichton alexcrichton added the relnotes Marks issues that should be documented in the release notes of the next release. label Jul 5, 2016
@alexcrichton
Copy link
Member

Oops, sorry this fell off the radar! I think this got lost in my inbox by accident...

Ok, everything looks good to me, thanks @xen0n! Could you also squash the commits down? Other than that r=me

xen0n added 10 commits July 14, 2016 02:44
format: beautifully get rid of ArgumentNext and CountIsNextParam

Now that CountIsNextParam and ArgumentNext are resolved during parse,
the need for handling them outside of libfmt_macros is obviated.

Note: *one* instance of implicit reference handling still remains, and
that's for implementing `all_args_simple`. It's trivial enough though,
so in this case it may be tolerable.
Converts named argument references into indices, right after
verification as suggested by @alexcrichton. This drastically simplifies
the whole process!
This commit removed the restriction of only allowing one type per argument.
This is achieved by adding mappings between macro arguments and format
placeholders, then taking the mapping into consideration when emitting
the Arguments expression.

syntax_ext: format: fix implicit positional arguments

syntax_ext: format: don't panic if no args given for implicit positional args

Check the list lengths before use.
Fixes regression of `compile-fail/macro-backtrace-println.rs`.

syntax_ext: format: also map CountIsParam indices to expanded args

syntax_ext: format: fix ICE in case of malformed format args
format: workaround pretty-printer to pass tests
@xen0n
Copy link
Contributor Author

xen0n commented Jul 13, 2016

Squashed all review-generated commits into respective commits and rebased again. Also clarified a few _ => panic! cases for better maintainability (no worries if some enum variants got added someday). Should be good to go now! @alexcrichton

(Edit: As for missing 1.10, I was also taking a break from Rust recently so I don't really mind. 😈 )

@alexcrichton
Copy link
Member

@bors: r+ 51e54e5

Thanks again!

@bors
Copy link
Contributor

bors commented Jul 13, 2016

⌛ Testing commit 51e54e5 with merge db71987...

bors added a commit that referenced this pull request Jul 13, 2016
Ergonomic format_args!

Fixes #9456 (at last).

Not a ground-up rewrite of the existing machinery, but more like an added intermediary layer between macro arguments and format placeholders. This is now implementing Rust RFC 1618!
@bors bors merged commit 51e54e5 into rust-lang:master Jul 13, 2016
@xen0n xen0n deleted the ergonomic-format-macro branch July 14, 2016 04:29
jimblandy added a commit to jimblandy/rust that referenced this pull request Feb 23, 2017
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Feb 24, 2017
…crichton

Update std::fmt module docs for landing of rust-lang#33642.

Since rust-lang#33642, it's no longer true that all references to a given format argument must use the same type. The docs don't seem to have been updated.
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Feb 24, 2017
…crichton

Update std::fmt module docs for landing of rust-lang#33642.

Since rust-lang#33642, it's no longer true that all references to a given format argument must use the same type. The docs don't seem to have been updated.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants