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

Syntax extensions (and macros) that start a line want to be a whole statement #5941

Closed
paulstansifer opened this issue Apr 18, 2013 · 14 comments
Labels
A-parser Area: The parsing of Rust source code to an AST A-syntaxext Area: Syntax extensions P-medium Medium priority

Comments

@paulstansifer
Copy link
Contributor

{ stringify!(bees).to_owned() }
 foo.rs:5:27: 5:28 error: unexpected token: `.`
 foo.rs:5          { stringify!(bees).to_owned() }
                                     ^

However, if you wrap the macro invocation in parens...

 { (stringify!(bees)).to_owned() }

...it works.

The tricky thing about this problem is that, when the parser sees stringify!, it doesn't know whether it'll be a statement macro or an expression macro; currently, it commits to the former, which breaks if the invocation has to be an expression.

@pnkfelix
Copy link
Member

@paulstansifer How hard is this to fix? How important is it to fix?

One can see similar paren-wrapping work-arounds elsewhere in such cases where disambiguation is necessary (e.g. to distinguish a method call a.foo() from a field-deref+function invocation (a.foo)()).

So, would it possibly suffice in this case to improve the parser's error message?

@paulstansifer
Copy link
Contributor Author

I think that this is different from that because, in that case, we need parentheses to distinguish between two different legitimate parses, whereas, in this case, there's a legitimate interpretation that's not accessible (and surprises the user who isn't thinking about statements at all).

On the other hand, determining whether stringify!( is going to be a statement is not possible in LL(k). The workaround is to parse a node that could be a statement or an expression, depending on what comes next.

Summary: I think it's important, but I could be persuaded that using parens is the proper Rustic solution.

@huonw
Copy link
Member

huonw commented Nov 24, 2013

Triage: still an issue; personally I think that parsing it as a statement always is ok if we detected when we've hit a parse error due to this choice (detecting it heuristically or otherwise), i.e. something like:

fn main() { stringify!(1).to_owned(); }

currently errors out with

5941.rs:1:25: 1:26 error: unexpected token: `.`
5941.rs:1 fn main() { stringify!(1).to_owned(); }
                                   ^

but could error with

error: unexpected token: `.` (a macro starting a line is parsed as a statement, wrap in parentheses to parse as an expression)

or something like that.

@huonw
Copy link
Member

huonw commented Jul 6, 2014

I think changing this would be backwards incompatible (although it's not clear that we actually want to change it), e.g.

foo!()
(x + 1).bar()

currently parses as two statements foo!() and (x+1).bar(), but if it were to be parsed as an expression, I think it would come out as ((foo!())(x+1)).bar() i.e. a function call with a method call on its result.

(Similar things happen with a prefix unary - becoming binary subtraction and [...] literals becoming an indexing expression; there may be others.)

Nominating.

@pcwalton
Copy link
Contributor

pcwalton commented Jul 7, 2014

How important is it that we have macros that can expand to statements? Couldn't such macros instead expand to the expression { STATEMENT }? (Maybe we could just do that expansion "under the hood"?)

@jbclements
Copy link
Contributor

Actually, I think there's some of that there, already; if you look at the default implementation of make_stmt in ext/tt/base.rs, you'll see that it does exactly that. I'm not looking at the larger conversation, though. There may be good reasons that this would break things.

Ooh, okay, how about this one; a let is a statement, right? So putting it in braces would kill its scope.

@huonw
Copy link
Member

huonw commented Jul 8, 2014

I have found use in an RAII macro like

macro_rules! guard {
    ($thing: expr) => { let _guard = Guarder::new($thing); }
}

which means

{
    guard!(foo);
    // ... all inside the guard above, due to the scoping of the `let`
}

works as you might expect; this was mainly to avoid problems where someone might write { Guarder::new(foo); ... } and thus not be inside the RAII-section for the .... However, we now have the #[must_use] attribute which makes this usecase less relevant.

@huonw
Copy link
Member

huonw commented Jul 8, 2014

(It would also kill macros like this.)

@pcwalton
Copy link
Contributor

pcwalton commented Jul 8, 2014

Oh yeah, it doesn't work for let. Bah. I guess this is hard to fix and maybe we shouldn't do anything.

@pnkfelix
Copy link
Member

Team decided that resolving this does not need to block 1.0; that is, if we to stick with our current behavior, that is acceptable for 1.0, though a bit of a wart.

Leaving issue open in the hopes that a better solution arises, but not a 1.0 milestone bug P-high..

@ftxqxd
Copy link
Contributor

ftxqxd commented Sep 19, 2014

As far as I can tell, there is no ‘nice’ way of fully resolving this, as an expression/statements like foo!() -1 is/are valid semantically under either interpretation. ‘Fixing’ this bug would almost certainly create another one about how make_an_int!() - 1 isn’t valid, although that is probably a less intrusive issue than this one. Removing statement macros altogether would work, but I don’t think we should: they’re actually quite useful for macros that expand to items, which is actually a relatively common use case for macros and shouldn’t require a semicolon. However, the obvious alternative is to parse things like foo!() -1 as (foo!() - 1), which is confusing when foo!() is a statement/item macro. So I see these options (mostly ignoring implementation details):

  1. Special-case the ambiguity resolution only for binary operators that can’t also start an expression. Things like foo!() -1 and foo!() () would be parsed as they are today, while things like foo!() + 1 and foo!().bar would be accepted as equivalent to (foo!() + 1) and (foo!()).bar, whereas they are invalid today. This is the only backwards-compatible option, and covers most realistic cases, but is quite inconsistent (e.g., it handles + and - differently). This could also be cut down to handle fewer cases, denying foo!() + 1 for consistency with foo!() - 1 etc.
  2. Always resolve an operator after a macro invocation as a binary operator. This would break current code like foo!() -1 and foo!() (), but in practice such cases are quite rare. This has the advantage of making code using macros in places like foo!().bar() and generate_int!() - 1 behave as one would expect, but would break existing code like generate_structs! { /* ... lots of code ... */ } -1 in a rather confusing manner.
  3. Do some kind of hack that generates two possible parsing options for code like foo!() - 1, and then resolves which one to do once the macro is expanded. So if foo!() expands to struct Foo;, then treat it as a statement macro; if foo!() expands to 1i, treat it as an expression macro. I have no idea if this would even be possible, but it would probably be the ideal option, although it is backwards-incompatible.
  4. Assume that macros with () and [] are expressions and that macros with {} are statements. This is an attractive option as it follows the requirements for struct: tuple structs (which use ()) require a semicolon following the declaration, while normal structs (which use {}) don’t.

@reem
Copy link
Contributor

reem commented Oct 3, 2014

This is pretty annoying when trying to use macros to start a chain of operations, as it makes using the macro much uglier.

This is an issue in rust-enforce as it makes the start of the chain (enforce!(something)) instead of enforce!(something).

ftxqxd added a commit to ftxqxd/rust that referenced this issue Oct 4, 2014
This fixes the most commonly-encountered case of rust-lang#5941.
@alexcrichton
Copy link
Member

cc @nick29581, could you migrate this to the RFCs repo?

@rust-highfive
Copy link
Collaborator

This issue has been moved to the RFCs repo: rust-lang/rfcs#364

flip1995 pushed a commit to flip1995/rust that referenced this issue Aug 28, 2020
Don't lint if it has always inline attribute

Don't trigger the lint `trivially_copy_pass_by_ref` if it has `#[inline(always)]` attribute.

Note: I am not particularly familiar with `inline` impacts, so I implemented this the way that if only `#[inline]` attribute is here (without `always`), the lint will still trigger. Also, it will still trigger if it has `#[inline(never)]`.
Just tell me if it sounds too much conservative.

Fixes: rust-lang#5876

changelog: none
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-parser Area: The parsing of Rust source code to an AST A-syntaxext Area: Syntax extensions P-medium Medium priority
Projects
None yet
Development

No branches or pull requests

9 participants