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.

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

Jump to bottom

Update expression span when transcribing macro args #31089

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 all commits
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
90 changes: 60 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,7 +233,6 @@ macro_rules! maybe_whole {
)
}


fn maybe_append(mut lhs: Vec<Attribute>, rhs: Option<Vec<Attribute>>)
-> Vec<Attribute> {
if let Some(ref attrs) = rhs {
Expand All @@ -255,6 +254,7 @@ pub struct Parser<'a> {
pub cfg: CrateConfig,
/// the previous token or None (only stashed sometimes).
pub last_token: Option<Box<token::Token>>,
last_token_interpolated: bool,
pub buffer: [TokenAndSpan; 4],
pub buffer_start: isize,
pub buffer_end: isize,
Expand Down Expand Up @@ -362,6 +362,7 @@ impl<'a> Parser<'a> {
span: span,
last_span: span,
last_token: None,
last_token_interpolated: false,
buffer: [
placeholder.clone(),
placeholder.clone(),
Expand Down Expand Up @@ -542,6 +543,19 @@ impl<'a> Parser<'a> {
self.commit_stmt(&[edible], &[])
}

/// returns the span of expr, if it was not interpolated or the span of the interpolated token
fn interpolated_or_expr_span(&self,
expr: PResult<'a, P<Expr>>)
-> PResult<'a, (Span, P<Expr>)> {
expr.map(|e| {
if self.last_token_interpolated {
(self.last_span, e)
} else {
(e.span, e)
}
})
}

pub fn parse_ident(&mut self) -> PResult<'a, ast::Ident> {
self.check_strict_keywords();
self.check_reserved_keywords();
Expand Down Expand Up @@ -933,6 +947,7 @@ impl<'a> Parser<'a> {
} else {
None
};
self.last_token_interpolated = self.token.is_interpolated();
let next = if self.buffer_start == self.buffer_end {
self.reader.real_token()
} else {
Expand Down Expand Up @@ -2322,18 +2337,20 @@ impl<'a> Parser<'a> {
-> PResult<'a, P<Expr>> {
let attrs = try!(self.parse_or_use_outer_attributes(already_parsed_attrs));

let b = try!(self.parse_bottom_expr());
self.parse_dot_or_call_expr_with(b, attrs)
let b = self.parse_bottom_expr();
let (span, b) = try!(self.interpolated_or_expr_span(b));
self.parse_dot_or_call_expr_with(b, span.lo, attrs)
}

pub fn parse_dot_or_call_expr_with(&mut self,
e0: P<Expr>,
lo: BytePos,
attrs: ThinAttributes)
-> PResult<'a, P<Expr>> {
// Stitch the list of outer attributes onto the return value.
// A little bit ugly, but the best way given the current code
// structure
self.parse_dot_or_call_expr_with_(e0)
self.parse_dot_or_call_expr_with_(e0, lo)
.map(|expr|
expr.map(|mut expr| {
expr.attrs.update(|a| a.prepend(attrs));
Expand All @@ -2360,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 @@ -2374,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 @@ -2408,9 +2424,8 @@ impl<'a> Parser<'a> {
})
}

fn parse_dot_or_call_expr_with_(&mut self, e0: P<Expr>) -> PResult<'a, P<Expr>> {
fn parse_dot_or_call_expr_with_(&mut self, e0: P<Expr>, lo: BytePos) -> PResult<'a, P<Expr>> {
let mut e = e0;
let lo = e.span.lo;
let mut hi;
loop {
// expr.f
Expand All @@ -2421,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 @@ -2474,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 @@ -2709,27 +2724,31 @@ impl<'a> Parser<'a> {
let ex = match self.token {
token::Not => {
self.bump();
let e = try!(self.parse_prefix_expr(None));
hi = e.span.hi;
let e = self.parse_prefix_expr(None);
let (span, e) = try!(self.interpolated_or_expr_span(e));
hi = span.hi;
self.mk_unary(UnNot, e)
}
token::BinOp(token::Minus) => {
self.bump();
let e = try!(self.parse_prefix_expr(None));
hi = e.span.hi;
let e = self.parse_prefix_expr(None);
let (span, e) = try!(self.interpolated_or_expr_span(e));
hi = span.hi;
self.mk_unary(UnNeg, e)
}
token::BinOp(token::Star) => {
self.bump();
let e = try!(self.parse_prefix_expr(None));
hi = e.span.hi;
let e = self.parse_prefix_expr(None);
let (span, e) = try!(self.interpolated_or_expr_span(e));
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 e = try!(self.parse_prefix_expr(None));
hi = e.span.hi;
let e = self.parse_prefix_expr(None);
let (span, e) = try!(self.interpolated_or_expr_span(e));
hi = span.hi;
ExprAddrOf(m, e)
}
token::Ident(..) if self.token.is_keyword(keywords::In) => {
Expand All @@ -2747,9 +2766,10 @@ impl<'a> Parser<'a> {
}
token::Ident(..) if self.token.is_keyword(keywords::Box) => {
self.bump();
let subexpression = try!(self.parse_prefix_expr(None));
hi = subexpression.span.hi;
ExprBox(subexpression)
let e = self.parse_prefix_expr(None);
let (span, e) = try!(self.interpolated_or_expr_span(e));
hi = span.hi;
ExprBox(e)
}
_ => return self.parse_dot_or_call_expr(Some(attrs))
};
Expand Down Expand Up @@ -2784,12 +2804,21 @@ impl<'a> Parser<'a> {
try!(self.parse_prefix_expr(attrs))
}
};


if self.expr_is_complete(&*lhs) {
// Semi-statement forms are odd. See https://github.com/rust-lang/rust/issues/29071
return Ok(lhs);
}
self.expected_tokens.push(TokenType::Operator);
while let Some(op) = AssocOp::from_token(&self.token) {

let lhs_span = if self.last_token_interpolated {
self.last_span
} else {
lhs.span
};

let cur_op_span = self.span;
let restrictions = if op.is_assign_like() {
self.restrictions & Restrictions::RESTRICTION_NO_STRUCT_LITERAL
Expand All @@ -2806,12 +2835,12 @@ 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,
lhs = self.mk_expr(lhs_span.lo, rhs.span.hi,
ExprCast(lhs, rhs), None);
continue
} else if op == AssocOp::Colon {
let rhs = try!(self.parse_ty());
lhs = self.mk_expr(lhs.span.lo, rhs.span.hi,
lhs = self.mk_expr(lhs_span.lo, rhs.span.hi,
ExprType(lhs, rhs), None);
continue
} else if op == AssocOp::DotDot {
Expand All @@ -2833,7 +2862,7 @@ impl<'a> Parser<'a> {
} else {
None
};
let (lhs_span, rhs_span) = (lhs.span, if let Some(ref x) = rhs {
let (lhs_span, rhs_span) = (lhs_span, if let Some(ref x) = rhs {
x.span
} else {
cur_op_span
Expand Down Expand Up @@ -2873,14 +2902,14 @@ impl<'a> Parser<'a> {
AssocOp::Equal | AssocOp::Less | AssocOp::LessEqual | AssocOp::NotEqual |
AssocOp::Greater | AssocOp::GreaterEqual => {
let ast_op = op.to_ast_binop().unwrap();
let (lhs_span, rhs_span) = (lhs.span, rhs.span);
let (lhs_span, rhs_span) = (lhs_span, rhs.span);
let binary = self.mk_binary(codemap::respan(cur_op_span, ast_op), lhs, rhs);
self.mk_expr(lhs_span.lo, rhs_span.hi, binary, None)
}
AssocOp::Assign =>
self.mk_expr(lhs.span.lo, rhs.span.hi, ExprAssign(lhs, rhs), None),
self.mk_expr(lhs_span.lo, rhs.span.hi, ExprAssign(lhs, rhs), None),
AssocOp::Inplace =>
self.mk_expr(lhs.span.lo, rhs.span.hi, ExprInPlace(lhs, rhs), None),
self.mk_expr(lhs_span.lo, rhs.span.hi, ExprInPlace(lhs, rhs), None),
AssocOp::AssignOp(k) => {
let aop = match k {
token::Plus => BiAdd,
Expand All @@ -2894,7 +2923,7 @@ impl<'a> Parser<'a> {
token::Shl => BiShl,
token::Shr => BiShr
};
let (lhs_span, rhs_span) = (lhs.span, rhs.span);
let (lhs_span, rhs_span) = (lhs_span, rhs.span);
let aopexpr = self.mk_assign_op(codemap::respan(cur_op_span, aop), lhs, rhs);
self.mk_expr(lhs_span.lo, rhs_span.hi, aopexpr, None)
}
Expand Down Expand Up @@ -3828,7 +3857,8 @@ impl<'a> Parser<'a> {
let e = self.mk_mac_expr(span.lo, span.hi,
mac.and_then(|m| m.node),
None);
let e = try!(self.parse_dot_or_call_expr_with(e, attrs));
let lo = e.span.lo;
let e = try!(self.parse_dot_or_call_expr_with(e, lo, attrs));
let e = try!(self.parse_assoc_expr_with(0, LhsExpr::AlreadyParsed(e)));
try!(self.handle_expression_like_statement(
e,
Expand Down
8 changes: 8 additions & 0 deletions src/libsyntax/parse/token.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,14 @@ impl Token {
}
}

/// Returns `true` if the token is interpolated.
pub fn is_interpolated(&self) -> bool {
match *self {
Interpolated(..) => true,
_ => false,
}
}

/// Returns `true` if the token is an interpolated path.
pub fn is_path(&self) -> bool {
match *self {
Expand Down
24 changes: 24 additions & 0 deletions src/test/compile-fail/issue-25385.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// Copyright 2016 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.


macro_rules! foo {
($e:expr) => { $e.foo() }
//~^ ERROR no method named `foo` found for type `i32` in the current scope
}

fn main() {
let a = 1i32;
foo!(a);
Copy link
Member

Choose a reason for hiding this comment

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

Looking at this test I worry whether your approach is not ideal; in practice wouldn't it be a strict improvement to point the user at the macro invocation in a case like this? otherwise important context is missing, no?

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 agree, even with this change, the error messages are not as good as they could be. In most cases it would be really helpful to have the information to what a macro variable has been expanded to get the complete picture, e.g. by adding a note: "$e" was expanded to "a", which highlights the parameter in the macro invocation.

I think it still makes sense to highlight the error inside the macro, because that's what causes the error. At the invocation site, we just pass what the macro rule expects (an arbitrary expression).

I also think the ideal place to highlight the error depends on whether the underlying problem is in the macro defintion (there's a bug in the macro) or at with the invocation (an unsupported expression was passed). If we would know that the problem is with the invocation, then we could just display at the invocation site, like:

 error involving expression 'a' passed to macro 'foo': no method named `foo` found for type `i32` in the current scope`

But I think there's no way to know that for sure.

So in my opinion including more detailed information about expanded macro arguments would be the most flexible option. This would probably require substantial changes to the error reporting however.

Copy link
Member

Choose a reason for hiding this comment

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

But I think there's no way to know that for sure.

Sure, but ... we display a macro-backtrace, and one can walk up that to inspect the macro definition, right?

Maybe I need to try out the patch myself. If you are printing out the macro backtraces in the error messages here, and those backtraces do start at the macro invocation site, then perhaps I have nothing to worry about: the compiler will guess at which site to report in its span, but if the backtrace includes everything starting at the invocation site, then the user can always use the backtrace to get the invocation site.

(But this is assuming that the backtrace is that clever, which I suspect it is not, since I think it just uses the provided span and the links therein, which means we won't have the invocation site here, right?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As it is now, the invocation site is displayed with a note, e.g. for the test case the relevant output would be:

src/test/compile-fail/issue-25385.rs:13:23: 13:28 error: no method named `foo` found for type `i32` in the current scope
src/test/compile-fail/issue-25385.rs:13     ($e:expr) => { $e.foo() }
src/test/compile-fail/issue-25385.rs:19:5: 19:13 note: in this expansion of foo! (defined in src/test/compile-fail/issue-25385.rs)

So it includes a backtrace to the invocation site. But in order to find out which parameter $e is expanded to one would have to look at the macro definition.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm okay then.

Copy link
Member

Choose a reason for hiding this comment

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

One last thing then: can you add the expected //~ NOTE annotations to the macro invocation sites in these test cases?

That will make it easier for other people reviewing the tests to see the overall picture of the error output we expect, without having to run the compiler on the test by hand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, done.

//~^ NOTE in this expansion of foo!

foo!(1i32.foo());
//~^ ERROR no method named `foo` found for type `i32` in the current scope
}
40 changes: 40 additions & 0 deletions src/test/compile-fail/issue-25386.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
// Copyright 2016 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

mod stuff {
pub struct Item {
c_object: Box<CObj>,
}
pub struct CObj {
name: Option<String>,
}
impl Item {
pub fn new() -> Item {
Item {
c_object: Box::new(CObj { name: None }),
}
}
}
}

macro_rules! check_ptr_exist {
($var:expr, $member:ident) => (
(*$var.c_object).$member.is_some()
//~^ ERROR field `name` of struct `stuff::CObj` is private
//~^^ ERROR field `c_object` of struct `stuff::Item` is private
);
}

fn main() {
let item = stuff::Item::new();
println!("{}", check_ptr_exist!(item, name));
//~^ NOTE in this expansion of check_ptr_exist!
//~^^ NOTE in this expansion of check_ptr_exist!
}
33 changes: 33 additions & 0 deletions src/test/compile-fail/issue-25793.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
// Copyright 2016 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

macro_rules! width(
($this:expr) => {
$this.width.unwrap()
//~^ ERROR cannot use `self.width` because it was mutably borrowed
}
);

struct HasInfo {
width: Option<usize>
}

impl HasInfo {
fn get_size(&mut self, n: usize) -> usize {
n
}

fn get_other(&mut self) -> usize {
self.get_size(width!(self))
//~^ NOTE in this expansion of width!
}
}

fn main() {}
21 changes: 21 additions & 0 deletions src/test/compile-fail/issue-26093.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// Copyright 2016 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

macro_rules! not_an_lvalue {
($thing:expr) => {
$thing = 42;
//~^ ERROR invalid left-hand side expression
}
}

fn main() {
not_an_lvalue!(99);
//~^ NOTE in this expansion of not_an_lvalue!
}
Loading