Update expression span when transcribing macro args #31089

fhahn · 2016-01-21T21:24:26Z

This is a work in progress PR that potentially should fix #29084, #28308, #25385, #28288, #31011. I think this may also adresse parts of #2887.

The problem in this issues seems to be that when transcribing macro arguments, we just clone the argument Nonterminal, which still has to original spans. This leads to the unprintable spans. One solution would be to update the spans of the inserted argument to match the argument in the macro definition. So for this testcase the error message would be displayed in the macro definition:

src/test/compile-fail/issue-31011.rs:4:12: 4:22 error: attempted access of field `trace` on type `&T`, but no field with that name was found
src/test/compile-fail/issue-31011.rs:4         if $ctx.trace {

Currently I've added a very simple update_span function, which updates the span of the outer-most expression of a NtExpr, but this update_span function should be updated to handle all Nonterminals. But I'm pretty new to the macro system and would like to check if this approach makes sense, before doing that.

rust-highfive · 2016-01-21T21:24:38Z

r? @pnkfelix

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

pnkfelix · 2016-01-22T15:25:50Z

src/libsyntax/ext/tt/transcribe.rs

@@ -181,6 +187,7 @@ pub fn tt_next_token(r: &mut TtReader) -> TokenAndSpan {
        tok: r.cur_tok.clone(),
        sp: r.cur_span.clone(),
    };
+    //println!("RET {:?} {:?}", r.cur_tok, r.cur_span);


please remove these commented out println's... (or if you think they have long term value, then turn them into uncommented debug! invocations.)

fhahn · 2016-01-23T14:25:13Z

@pnkfelix Thanks for the feedback and sorry for opening the PR with those problems. Ran out of time and wanted to start the PR to get some feedback on the general approach.

Now I finally had time to update those tests and think the approach as it is at the moment already gets us pretty far. It fixes all issues with unprintable span I could find (I've created testcases for all issues fixed). Just updating the span of the outermost expression seesm fine, because that way errors in inner expressions are reported at the macro call site (as seen here), which is nice.

But this PR currently only updates the spans of NtExpr arguments. The spans of other non terminals should probably be updated as well, but I am not sure if it should be done in this PR or in a separate one?

fhahn · 2016-01-23T14:31:56Z

I'm pretty sure this PR also closes #25353, but the link to the example is not valid any more.

pnkfelix · 2016-01-24T00:25:23Z

The spans of other non terminals should probably be updated as well, but I am not sure if it should be done in this PR or in a separate one?

I am fine with keeping this PR focused on just the NtExprs,

fhahn · 2016-01-24T22:20:21Z

I've updated the PR again. They way how the spans are handled changed completely from the first version. The idea is as follows:

Transcribed expressions keep their original spans so errors that only involve this expressions are displayed at the macro invocation. When building spans for expressions inside a macro definition that contain transcribed expressions, the parsers uses the span of the Interpolated token (instead of the span of the transcribed expression, which points to the point of the macro invocation). This way, there should be no unprintable spans.

There are already a couple of test cases which illustrate where errors are displayed with this change, but this scheme required small changes to multiple parts of the parser, so I probably should add a couple of more, covering all cases.

This change may have a slight impact on the parser performance. We need to know if the previous token was Interpolated to decide which span to use. For now this PR does this by saving it in last_token, but we do not need to know what was inside this token, so I could add another field to the parsers (like last_token_interpolated: bool).

pnkfelix · 2016-01-25T07:59:37Z

src/libsyntax/parse/parser.rs

@@ -2782,13 +2802,13 @@ impl<'a> Parser<'a> {
            }
            // Special cases:
            if op == AssocOp::As {
-                let rhs = try!(self.parse_ty());
-                lhs = self.mk_expr(lhs.span.lo, rhs.span.hi,
+               let rhs = try!(self.parse_ty());


The indent is weird here, no?

Fixed in my latest commit

bors · 2016-01-26T04:24:29Z

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

closes rust-lang#29084 closes rust-lang#28308 closes rust-lang#25385 closes rust-lang#28288 closes rust-lang#31011 closes rust-lang#26480 closes rust-lang#26093 closes rust-lang#26094 closes rust-lang#25386 closes rust-lang#26237 closes rust-lang#25793

…dot_suffix

fhahn · 2016-01-26T10:53:39Z

@pnkfelix I've pushed a new commit which adds a macro interpolated_or_expr_span which takes an expr to parse an expr as argument and returns the resulting expr and either the span of the expression or the interpolated token.

pnkfelix · 2016-01-26T11:38:06Z

src/libsyntax/parse/parser.rs

@@ -233,6 +233,21 @@ macro_rules! maybe_whole {
    )
 }

+/// Uses $parse_expr to parse an expression and returns the span of the interpolated
+/// token or the span of the parsed expression, if it was not interpolated
+macro_rules! interpolated_or_expr_span {


this makes me feel better about the pattern here. (I still worry people may not know when to call this, but at least now it's easy to see and swap in.)

AFAICT, the reason you made this a macro is due to the use of try! in the given $parse_expr.

I would prefer you make this a fn that returns Result, and shift the trys at the call sites.

It's a macro so it can be used with different methods for parsing expressions. In particular it is once used with Parser::parse_bottom_expr and also with Parser::parse_prefix_expr.

I've update the macro to return Result, which should have the same effect making it a fn, at least with respect to the try! (I hope). A similar fn would have to take a closure which is probably slightly less ergomatic. However if you still think a fn would be better, I'd be happy to change that.

Your argument still doesn't make sense to me.

Both Parser::parse_bottom_expr and Parser::parse_prefix_expr have the same return type, so its not like you are using the macro to inject some kind of duck-typing...

To be clear: You have a macro here that takes in two expressions as input ($p and $parse_expr). It unconditionally evaluates both expressions at the outset. (There's a second evaluation of $p but I don't think that was a deliberate choice.)

So, why wouldn't this work:

impl<'a> Parser<'a> { /// Uses $parse_expr to parse an expression and returns the span of the interpolated /// token or the span of the parsed expression, if it was not interpolated fn interpolated_or_expr_span(&self, expr: PResult<'a, P<Expr>>) -> PResult<'a, (Span, P<Expr>)> { let is_interpolated = self.token.is_interpolated(); expr.map(|e| { if is_interpolated { (self.last_span, e) } else { (e.span, e) } }) } }

(And then update the macro calls to call this method instead.)

Yeah, that makes sense. I've pushed another commit that turns it into a function.

I now remembered to original reason for that being a macro. As it was, interpolated_or_expr_span used the current token to check if it is interpolated, which means the parse functions has to be called after that check.

But by using last_token it works as expected. I pushed another commit however, which avoids storing (and cloning) interpolated tokens and instead added a Parser.last_token_interpolated flag.

pnkfelix · 2016-01-26T11:40:04Z

@fhahn okay hang in there I think we are converging on something I will be very happy with. :)

pnkfelix · 2016-01-27T10:00:42Z

@fhahn r=me after you add the //~ NOTE annotations to the test cases (see #31089 (comment) )

pnkfelix · 2016-01-27T10:01:35Z

@fhahn (also, travis seems to signalled a make tidy failure.)

pnkfelix · 2016-01-27T12:12:24Z

@bors r+ ecb7b01

bors · 2016-01-27T12:12:53Z

⌛ Testing commit ecb7b01 with merge 8256c47...

This is a work in progress PR that potentially should fix #29084, #28308, #25385, #28288, #31011. I think this may also adresse parts of #2887. The problem in this issues seems to be that when transcribing macro arguments, we just clone the argument Nonterminal, which still has to original spans. This leads to the unprintable spans. One solution would be to update the spans of the inserted argument to match the argument in the macro definition. So for [this testcase](https://github.com/rust-lang/rust/compare/master...fhahn:macro-ice?expand=1#diff-f7def7420c51621640707b6337726876R2) the error message would be displayed in the macro definition: src/test/compile-fail/issue-31011.rs:4:12: 4:22 error: attempted access of field `trace` on type `&T`, but no field with that name was found src/test/compile-fail/issue-31011.rs:4 if $ctx.trace { Currently I've added a very simple `update_span` function, which updates the span of the outer-most expression of a `NtExpr`, but this `update_span` function should be updated to handle all Nonterminals. But I'm pretty new to the macro system and would like to check if this approach makes sense, before doing that.

bors · 2016-01-27T16:11:51Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

itaibn · 2016-02-14T16:07:58Z

I found a piece of code that emits a "unprintable span" compiler error. It looks like it's probably a duplicate of this issue, but I don't know for sure. Should I submit a bug report?

fhahn · 2016-02-14T16:51:08Z

This PR fixed a couple of unprintable span errors related to expaneded macro arguments. What's the problamtic code?

fhahn · 2016-02-14T16:53:42Z

Anyways, I think opening an issue would be a good idea, so we do not loose track of the problem.

itaibn · 2016-02-14T17:38:08Z

Alright, I opened a new issue for the error I found.

rust-highfive assigned pnkfelix Jan 21, 2016

pnkfelix reviewed Jan 22, 2016
View reviewed changes

fhahn force-pushed the macro-ice branch from 687fe28 to b8ad7a6 Compare January 23, 2016 14:18

fhahn force-pushed the macro-ice branch from b8ad7a6 to 8c968d9 Compare January 23, 2016 14:34

fhahn force-pushed the macro-ice branch 2 times, most recently from cdf6e38 to 26a9c32 Compare January 24, 2016 23:02

pnkfelix reviewed Jan 25, 2016
View reviewed changes

fhahn added 4 commits January 26, 2016 10:31

Update tests

877ed0d

Set span for interpolated tokens correctly

20edb36

Use interpolated token span when building spans for bigger expressions

1bde18d

fhahn force-pushed the macro-ice branch 2 times, most recently from 95a0b50 to 94ed73c Compare January 26, 2016 10:46

Add interpolated_or_expr_span macro and pass lo to newly added parse_…

2bc8f4f

…dot_suffix

fhahn force-pushed the macro-ice branch from 94ed73c to 2bc8f4f Compare January 26, 2016 10:51

pnkfelix reviewed Jan 26, 2016
View reviewed changes

fhahn added 2 commits January 26, 2016 12:49

Push try! to call site of interpolated_or_expr_span!

9d8c64b

Turn interpolated_or_expr_span into a function

47bfd8c

fhahn added 2 commits January 27, 2016 11:28

Avoid storing interolated token in Parser.last_token

e533ed9

Add NOTE test annotations

ecb7b01

bors mentioned this pull request Jan 27, 2016

Add compile-fail test in rustdoc #30726

Merged

bors merged commit ecb7b01 into rust-lang:master Jan 27, 2016

bors mentioned this pull request Jan 27, 2016

implement RFC 1192 inclusive ranges #30884

Merged

fhahn deleted the macro-ice branch January 27, 2016 17:26

itaibn mentioned this pull request Feb 14, 2016

"unprintable span" compiler error with improper closure in nom macro #31654

Closed

Mark-Simulacrum mentioned this pull request Sep 25, 2016

Use proper spans in ext::tt::transcribe #2887

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update expression span when transcribing macro args #31089

Update expression span when transcribing macro args #31089

fhahn commented Jan 21, 2016

rust-highfive commented Jan 21, 2016

pnkfelix Jan 22, 2016

fhahn commented Jan 23, 2016

fhahn commented Jan 23, 2016

pnkfelix commented Jan 24, 2016

fhahn commented Jan 24, 2016

pnkfelix Jan 25, 2016

fhahn Jan 26, 2016

bors commented Jan 26, 2016

fhahn commented Jan 26, 2016

pnkfelix Jan 26, 2016

fhahn Jan 26, 2016

pnkfelix Jan 27, 2016

fhahn Jan 27, 2016

fhahn Jan 27, 2016

pnkfelix commented Jan 26, 2016

pnkfelix commented Jan 27, 2016

pnkfelix commented Jan 27, 2016

pnkfelix commented Jan 27, 2016

bors commented Jan 27, 2016

bors commented Jan 27, 2016

itaibn commented Feb 14, 2016

fhahn commented Feb 14, 2016

fhahn commented Feb 14, 2016

itaibn commented Feb 14, 2016

Update expression span when transcribing macro args #31089

Update expression span when transcribing macro args #31089

Conversation

fhahn commented Jan 21, 2016

rust-highfive commented Jan 21, 2016

Choose a reason for hiding this comment

fhahn commented Jan 23, 2016

fhahn commented Jan 23, 2016

pnkfelix commented Jan 24, 2016

fhahn commented Jan 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Jan 26, 2016

fhahn commented Jan 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnkfelix commented Jan 26, 2016

pnkfelix commented Jan 27, 2016

pnkfelix commented Jan 27, 2016

pnkfelix commented Jan 27, 2016

bors commented Jan 27, 2016

bors commented Jan 27, 2016

itaibn commented Feb 14, 2016

fhahn commented Feb 14, 2016

fhahn commented Feb 14, 2016

itaibn commented Feb 14, 2016