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

My code fails to build with „unexpected end of macro invocation“ since nightly 2017-07-02 #43057

Closed
vorner opened this issue Jul 4, 2017 · 37 comments · Fixed by #43735
Closed
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) C-bug Category: This is a bug. regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@vorner
Copy link
Contributor

vorner commented Jul 4, 2017

Hello

I have this code: https://gitlab.labs.nic.cz/turris/pakon-aggregator (unfortunately, I don't have a minimal code example yet). It fails with rustc 1.20.0-nightly (067971139 2017-07-02) (for some reason this is the version I get if I run rustup default nightly-2017-07-03, it seems to be one day off) and newer, but passes with older versions of nighly as well as stable and beta.

The error I get is:

error: unexpected end of macro invocation
   --> src/keeper/column.rs:269:1
    |
269 | / column! {
270 | |     /// Identifier of an address inside an endpoint.
271 | |     ///
272 | |     /// This is usually used to specify aggregation by a value in queries.
...   |
357 | |     name Port u16;
358 | | }
    | |_^ in this macro invocation
    |
    = note: this error originates in a macro outside of the current crate

This is actually my own macro, so the „originates in a macro outside of the current crate“ part is wrong. Also, renaming the macro makes it compile again. This leads me to think something (the compiler) brought in a colliding macro with the same name.

Is my theory about the colliding macro true? Can this killing of my poor innocent macro be prevented somehow? Should local macros have precedence? Or, at least, should I get a better error message, like complaining when I define the macro, not when I use it? This makes me think that if I had the bad luck of having a same-name macro with the same invocation syntax, it would silently expand the wrong one.

Exact version

$ rustc --version --verbose
rustc 1.20.0-nightly (067971139 2017-07-02)
binary: rustc
commit-hash: 0679711398bef656699e1ff6b004ecccbdb67284
commit-date: 2017-07-02
host: x86_64-unknown-linux-gnu
release: 1.20.0-nightly
LLVM version: 4.0
@kennytm
Copy link
Member

kennytm commented Jul 4, 2017

Not sure if it is relevant, but there is already a column! macro in the standard library.

@TimNN
Copy link
Contributor

TimNN commented Jul 4, 2017

Minimized, side effect of #42938:

macro_rules! column { (foo) => { panic!() } }

fn main() {
    column!(foo);
}

@TimNN TimNN added the regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. label Jul 4, 2017
@TimNN
Copy link
Contributor

TimNN commented Jul 4, 2017

Or alternatively with a singularly unhelpful error message (column! isn't even used here, just defined):

macro_rules! column { (foo) => {} }

fn main() { panic!(); }
error: unexpected end of macro invocation
 --> <anon>:4:5
  |
4 |     panic!();
  |     ^^^^^^^^^
  |
  = note: this error originates in a macro outside of the current crate

@TimNN TimNN added A-diagnostics Area: Messages for errors, warnings, and lints A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) labels Jul 4, 2017
@PlasmaPower
Copy link
Contributor

Same thing happens with a file or line macro. #42938 just added column to the list. Should macros with those names be illegal, or should we work around them?

@kennytm
Copy link
Member

kennytm commented Jul 4, 2017

panic!() should use core::{file,line,column}!(). IIRC rust already has the qualified macro feature.

@TimNN
Copy link
Contributor

TimNN commented Jul 4, 2017

So I guess there are three "issues" here:

  1. If a macro expands to another macro, that macro is resolved where the expansion happens, not the definition (although that is expected as far as I know)
  2. We have macros in std which depend on other macros from std being available as defined in std. This is bad in my opinion and should maybe be fixed (one idea would be to to make macros like column! available as _rust_std_column! as well, for use in std. This would at least reduce the likelihood of them being redefined Live Edit: Even better what @kennytm said Real Edit: that doesn't work for compiler-provided macros like column!).
  3. Output column number info when panicking #42938 added a call to column! inside of panic! which causes the regression here.

Edit: Tagging as T-libs since the definition of the panic! macro is their responsibility although I guess compiler/lang may be applicable as well.

@TimNN
Copy link
Contributor

TimNN commented Jul 4, 2017

@kennytm: Sadly, that doesn't work: file!, line! and column! are compiler-builtin macros that don't exist in any crate.

@PlasmaPower
Copy link
Contributor

An empty implementation, however, does exist at the std/core root (presumably for documentation reasons). I don't know much about the compiler internals, but would it be possible to "fill in" that implementation, instead of creating a "new" builtin macro?

@vorner
Copy link
Contributor Author

vorner commented Jul 4, 2017

I see, so there was the column! macro in the library to start with and I should rename it.

Still, I think the error message could be a bit more helpful.

@PlasmaPower
Copy link
Contributor

PlasmaPower commented Jul 4, 2017

@vorner Your macro definition shouldn't interfere with the standard library, but yes, that's what's currently happening.

I realized that my proposed "filling in" solution also wouldn't work with #![no_core]. I'm not sure how to approach this.

@TimNN TimNN added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jul 5, 2017
@kennytm
Copy link
Member

kennytm commented Jul 5, 2017

Since #![no_core] is unstable I think telling the developers to use __rust_std_column__!() instead of column!() is permitted.

There should be a lint which warns about shadowing any of the built-in macros.

(Alternatively, would it be possible to rewrite file!, line!, column! as procedural macros (instead of libsyntax built-in plugins) that respects #35896 (macro modularization)? #![no_core] developers would still need to #[macro_use] extern crate core_macros; to use them.)

@Mark-Simulacrum Mark-Simulacrum added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jul 13, 2017
@brson
Copy link
Contributor

brson commented Jul 13, 2017

Needs to be prioritized. There's probably something we can do better here.

@Mark-Simulacrum Mark-Simulacrum added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jul 13, 2017
@est31
Copy link
Member

est31 commented Jul 13, 2017

cc @jseyfried -- this is due to an issue with macros 1.0 hygiene, maybe macros 2.0 can be made to prevent situations like this?

@jseyfried
Copy link
Contributor

jseyfried commented Jul 13, 2017

@est31 Yeah, macros 2.0 already prevents situations like this.
Example:

// crate A
pub macro n() { println!("Hello world!") }
pub macro m() { n!() }

// crate B
extern crate A;
use A::m;

macro n() {}
macro println() {}
//^ These don't interfere with the expansion of `m!()` below.

fn main() {
    m!(); // prints "Hello world!"
}

@jseyfried
Copy link
Contributor

jseyfried commented Jul 13, 2017

One way to fix this issue would be to migrate the std macros to macros 2.0. However, this would be a breaking change (e.g. people might be intentionally "overriding" dependencies of the std macros). Also, IIRC I tried something like this a while ago and found some breakage due to overriding in practice.

That being said, it might be worth assessing migrating with a warning cycle or some other partial mitigation.

cc @nrc

@jseyfried
Copy link
Contributor

jseyfried commented Jul 13, 2017

@est31 Caveat: while macros 2.0 prevents this situation, it doesn't help when using macros 1.0 from std. More specifically, a macros 2.0 can still "override" a dependency of an unhygienic macros 1.0.

It might be possible to special-case certain situation so that macros 2.0 do not "override" unhygienic macros 1.0 dependencies. For example, we could say:

  • Suppose a name is expanded from exclusively 1.0 macros.
  • Consider the resolution of this name (which is unhygienic) without any of the macros 2.0 in scope.
  • If the 2.0-less resolution matches how the name would resolve it it were fully hygienic (i.e. if it instead came from exclusively 2.0 macros), then use this resolution instead.

However, this would add complexity and probably has other downsides (e.g. breaking invariants preserved by macro expansion).

@alexcrichton alexcrichton added regression-from-stable-to-beta Performance or correctness regression from stable to beta. and removed regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. labels Jul 23, 2017
@alexcrichton
Copy link
Member

Out of curiosity, could we define unstable macros in the compiler like __column and use those in the panic! macro here? That'd fix the regression at least, right?

Othrewise, is there perhaps another sort of small patch we could apply to avoid the regression here? Beta's going out in just over a month!

@est31
Copy link
Member

est31 commented Jul 26, 2017

I see the nominated label, would be great to have a team talk about this. There needs to be a decision on whether this change is expected breakage or a bug? Or, to formulate the question more generally: is it forbidden for any macro of the standard library to change which macros it calls, or is such change allowed?

Also similar question: is it allowed for a macro in the standard library to become a wrapper of another macro? If it becomes, it adds 1 to the depth and could cause to code (which uses the recursion depth up to the limit, but doesn't attempt to surpass the limit) to be rejected that was previously accepted.

RFC 1105 has no section on macros...

@jseyfried
Copy link
Contributor

is it forbidden for any macro of the standard library to change which macros it calls, or is such change allowed?

I think it should be OK to move away from unhygienic "dependency overrides" (e.g. with @alexcrichton's idea) if it doesn't cause breakage in practice.

is it allowed for a macro in the standard library to become a wrapper of another macro? If it becomes, it adds 1 to the depth and could cause to code ... to be rejected

I doubt this would be an issue in practice, but if it is we could always add a small offset to the global recursion limit when computing the recursion limit for macros (macro expansion is stackless, so this is OK).

@Mark-Simulacrum Mark-Simulacrum added the C-bug Category: This is a bug. label Jul 26, 2017
@alexcrichton
Copy link
Member

@jseyfried just to confirm, but the "workaround" here would be to add a __column macro to the compiler versus to the standard library, because if the standard library had a __column macro that expanded to column!() that wouldn't work, right?

@jseyfried
Copy link
Contributor

jseyfried commented Jul 26, 2017

@alexcrichton Right.

We could instead define a declarative macros 2.0 __column in the standard library that expanded to column!(), and that would work due to hygiene; however, I think it would be cleaner to just add __column to the compiler.

@est31
Copy link
Member

est31 commented Jul 26, 2017

Why is adding __column to the compiler better?

@jseyfried
Copy link
Contributor

jseyfried commented Jul 26, 2017

column is already in the compiler, so adding __column would be a single-line change. Also, declarative macros 2.0 is unstable enough that I'm not sure we want to use for stable APIs yet.

@alexcrichton
Copy link
Member

@est31 out of curiosity, would you be willing to help implement the workaround here?

@est31
Copy link
Member

est31 commented Jul 27, 2017

@alexcrichton depends on whether such a workaround would actually be helping people. @vorner would it help you? @alexcrichton are there any results of a crater run on the beta available?

@vorner
Copy link
Contributor Author

vorner commented Jul 27, 2017

I already worked around the problem by renaming the macro (it is a private one to generate some code, so I don't care about its name). It was just the surprise the code broke.

But if you ask if it would have helped me if it was there to begin with, then I think yes ‒ I didn't have a clue what was wrong and only reporting the issue helped shed some light.

@alexcrichton
Copy link
Member

Ah unfortunately we don't have a crater run yet to see the impact of this.

@nikomatsakis
Copy link
Contributor

Discussed in compiler meeting. Awaiting some sort of crater run before assessing priority.

@alexcrichton
Copy link
Member

Libss concluded the same!

@est31
Copy link
Member

est31 commented Aug 5, 2017

I've looked at the root regressions from a crater run between stable and beta (link), and couldn't find any regression introduced by this issue. They would have shown up there because this PR made it into beta but is not in stable yet.

@arielb1
Copy link
Contributor

arielb1 commented Aug 5, 2017

@est31

There's #43672, which is an old version of Diesel.

@est31
Copy link
Member

est31 commented Aug 5, 2017

@arielb1 oh, right. Missed it :)

So dunno then... PR to https://github.com/stefanoc/magnet ? Or should this be fixed in the compiler because it affected diesel?

@arielb1
Copy link
Contributor

arielb1 commented Aug 5, 2017

I think this should be fixed in the standard library by making panic a macro 2.0.

@alexcrichton
Copy link
Member

@arielb1 above @jseyfried [mentioned]:

Also, declarative macros 2.0 is unstable enough that I'm not sure we want to use for stable APIs yet.

In that sense, would you be opposed to a compiler-defined __column macro (that's unstable) that libstd calls?

It does indeed seem like we should implement this!

@arielb1
Copy link
Contributor

arielb1 commented Aug 7, 2017

@alexcrichton

I don't care what fix happens, only that it does.

@alexcrichton
Copy link
Member

@est31 out of curiosity, would you be willing to help implement this?

@est31
Copy link
Member

est31 commented Aug 8, 2017

@alexcrichton sure!

IMO its not really needed, but you seem to insist...

bors added a commit that referenced this issue Aug 10, 2017
Avoid calling the column!() macro in panic

Closes #43057

This "fix" adds a new macro called `__rust_unstable_column` and to use it instead of the `column` macro inside panic. The new macro can be shadowed as well as `column` can, but its very likely that there is no code that does this in practice.

There is no real way to make "unstable" macros that are usable by stable macros, so we do the next best thing and prefix the macro with `__rust_unstable` to make sure people recognize it is unstable.

r? @alexcrichton
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) C-bug Category: This is a bug. regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.