Update expression span when transcribing macro args #31089
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| // 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); | ||
|
There was a problem hiding this comment. 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? There was a problem hiding this comment. 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: 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: 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. There was a problem hiding this comment.
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?) There was a problem hiding this comment. As it is now, the invocation site is displayed with a note, e.g. for the test case the relevant output would be: So it includes a backtrace to the invocation site. But in order to find out which parameter There was a problem hiding this comment. One last thing then: can you add the expected 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. |
||
|
|
||
| foo!(1i32.foo()); | ||
| //~^ ERROR no method named `foo` found for type `i32` in the current scope | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| // 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)); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| // 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)) | ||
| } | ||
| } | ||
|
|
||
| fn main() { | ||
| println!("hello?"); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| // 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); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| // 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! some_macro { | ||
| ($other: expr) => ({ | ||
| $other(None) | ||
| //~^ this function takes 0 parameters but 1 parameter was supplied | ||
| }) | ||
| } | ||
|
|
||
| fn some_function() { | ||
| } | ||
|
|
||
| fn main() { | ||
| some_macro!(some_function); | ||
| } |
There was a problem hiding this comment.
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
fnthat returns Result, and shift the trys at the call sites.There was a problem hiding this comment.
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 afn, at least with respect to thetry!(I hope). A similarfnwould have to take a closure which is probably slightly less ergomatic. However if you still think afnwould be better, I'd be happy to change that.There was a problem hiding this comment.
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_exprandParser::parse_prefix_exprhave 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 (
$pand$parse_expr). It unconditionally evaluates both expressions at the outset. (There's a second evaluation of$pbut I don't think that was a deliberate choice.)So, why wouldn't this work:
(And then update the macro calls to call this method instead.)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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_spanused 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_tokenit works as expected. I pushed another commit however, which avoids storing (and cloning) interpolated tokens and instead added aParser.last_token_interpolatedflag.