Skip to content

Update expression span when transcribing macro args #31089

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.

Sign up for GitHub

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

Merged
merged 9 commits into from
Jan 27, 2016
Merged

Update expression span when transcribing macro args #31089

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b285ebc
Update expression span when transcribing macro args
fhahn Jan 21, 2016
877ed0d
Update tests
fhahn Jan 23, 2016
20edb36
Set span for interpolated tokens correctly
fhahn Jan 23, 2016
1bde18d
Use interpolated token span when building spans for bigger expressions
fhahn Jan 24, 2016
2bc8f4f
Add interpolated_or_expr_span macro and pass lo to newly added parse_…
fhahn Jan 26, 2016
9d8c64b
Push try! to call site of interpolated_or_expr_span!
fhahn Jan 26, 2016
47bfd8c
Turn interpolated_or_expr_span into a function
fhahn Jan 27, 2016
e533ed9
Avoid storing interolated token in Parser.last_token
fhahn Jan 27, 2016
ecb7b01
Add NOTE test annotations
fhahn Jan 27, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Add interpolated_or_expr_span macro and pass lo to newly added parse_… 
…dot_suffix
  • Loading branch information
@fhahn
fhahn committed Jan 26, 2016
commit 2bc8f4ff80a4343bacfcab9629eb681e576dee48
68 changes: 38 additions & 30 deletions src/libsyntax/parse/parser.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

($p:expr, $parse_expr:expr) => {
{
let is_interpolated = $p.token.is_interpolated();
let e = $parse_expr;
if is_interpolated {
($p.last_span, e)
} else {
(e.span, e)
}
}
}
}

fn maybe_append(mut lhs: Vec<Attribute>, rhs: Option<Vec<Attribute>>)
-> Vec<Attribute> {
Expand Down Expand Up @@ -2323,14 +2338,8 @@ impl<'a> Parser<'a> {
-> PResult<'a, P<Expr>> {
let attrs = try!(self.parse_or_use_outer_attributes(already_parsed_attrs));

let is_interpolated = self.token.is_interpolated();
let b = try!(self.parse_bottom_expr());
let lo = if is_interpolated {
self.last_span.lo
} else {
b.span.lo
};
self.parse_dot_or_call_expr_with(b, lo, attrs)
let (span, b) = interpolated_or_expr_span!(self, try!(self.parse_bottom_expr()));
self.parse_dot_or_call_expr_with(b, span.lo, attrs)
}

pub fn parse_dot_or_call_expr_with(&mut self,
Expand Down Expand Up @@ -2368,7 +2377,8 @@ impl<'a> Parser<'a> {
fn parse_dot_suffix(&mut self,
ident: Ident,
ident_span: Span,
self_value: P<Expr>)
self_value: P<Expr>,
lo: BytePos)
-> PResult<'a, P<Expr>> {
let (_, tys, bindings) = if self.eat(&token::ModSep) {
try!(self.expect_lt());
Expand All @@ -2382,8 +2392,6 @@ impl<'a> Parser<'a> {
self.span_err(last_span, "type bindings are only permitted on trait paths");
}

let lo = self_value.span.lo;

Ok(match self.token {
// expr.f() method call.
token::OpenDelim(token::Paren) => {
Expand Down Expand Up @@ -2428,7 +2436,7 @@ impl<'a> Parser<'a> {
hi = self.span.hi;
self.bump();

e = try!(self.parse_dot_suffix(i, mk_sp(dot_pos, hi), e));
e = try!(self.parse_dot_suffix(i, mk_sp(dot_pos, hi), e, lo));
}
token::Literal(token::Integer(n), suf) => {
let sp = self.span;
Expand Down Expand Up @@ -2481,7 +2489,7 @@ impl<'a> Parser<'a> {
let dot_pos = self.last_span.hi;
e = try!(self.parse_dot_suffix(special_idents::invalid,
mk_sp(dot_pos, dot_pos),
e));
e, lo));
}
}
continue;
Expand Down Expand Up @@ -2716,31 +2724,31 @@ impl<'a> Parser<'a> {
let ex = match self.token {
token::Not => {
self.bump();
let (interpolated, prev_span) = (self.token.is_interpolated(), self.span);
let e = try!(self.parse_prefix_expr(None));
hi = if interpolated { prev_span.hi } else { e.span.hi };
let (span, e) = interpolated_or_expr_span!(self,
try!(self.parse_prefix_expr(None)));
hi = span.hi;
self.mk_unary(UnNot, e)
}
token::BinOp(token::Minus) => {
self.bump();
let (interpolated, prev_span) = (self.token.is_interpolated(), self.span);
let e = try!(self.parse_prefix_expr(None));
hi = if interpolated { prev_span.hi } else { e.span.hi };
let (span, e) = interpolated_or_expr_span!(self,
try!(self.parse_prefix_expr(None)));
hi = span.hi;
self.mk_unary(UnNeg, e)
}
token::BinOp(token::Star) => {
self.bump();
let (interpolated, prev_span) = (self.token.is_interpolated(), self.span);
let e = try!(self.parse_prefix_expr(None));
hi = if interpolated { prev_span.hi } else { e.span.hi };
let (span, e) = interpolated_or_expr_span!(self,
try!(self.parse_prefix_expr(None)));
hi = span.hi;
self.mk_unary(UnDeref, e)
}
token::BinOp(token::And) | token::AndAnd => {
try!(self.expect_and());
let m = try!(self.parse_mutability());
let (interpolated, prev_span) = (self.token.is_interpolated(), self.span);
let e = try!(self.parse_prefix_expr(None));
hi = if interpolated { prev_span.hi } else { e.span.hi };
let (span, e) = interpolated_or_expr_span!(self,
try!(self.parse_prefix_expr(None)));
hi = span.hi;
ExprAddrOf(m, e)
}
token::Ident(..) if self.token.is_keyword(keywords::In) => {
Expand All @@ -2758,10 +2766,10 @@ impl<'a> Parser<'a> {
}
token::Ident(..) if self.token.is_keyword(keywords::Box) => {
self.bump();
let (interpolated, prev_span) = (self.token.is_interpolated(), self.span);
let subexpression = try!(self.parse_prefix_expr(None));
hi = if interpolated { prev_span.hi } else { subexpression.span.hi };
ExprBox(subexpression)
let (span, e) = interpolated_or_expr_span!(self,
try!(self.parse_prefix_expr(None)));
hi = span.hi;
ExprBox(e)
}
_ => return self.parse_dot_or_call_expr(Some(attrs))
};
Expand Down Expand Up @@ -2825,7 +2833,7 @@ impl<'a> Parser<'a> {
}
// Special cases:
if op == AssocOp::As {
let rhs = try!(self.parse_ty());
let rhs = try!(self.parse_ty());
lhs = self.mk_expr(lhs_span.lo, rhs.span.hi,
ExprCast(lhs, rhs), None);
continue
Expand Down
2 changes: 1 addition & 1 deletion src/test/compile-fail/issue-31011.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
macro_rules! log {
( $ctx:expr, $( $args:expr),* ) => {
if $ctx.trace {
//~^ attempted access of field `trace` on type `&T`, but no field with that name was found
//~^ ERROR attempted access of field `trace` on type `&T`, but no field with that name
println!( $( $args, )* );
}
}
Expand Down