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

warning: Quasi-quoting might make incremental compilation very inefficient: NtIdent(..) #40946

Closed
SimonSapin opened this issue Mar 31, 2017 · 15 comments · Fixed by #44601
Closed
Assignees
Labels
A-incr-comp Area: Incremental compilation C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@SimonSapin
Copy link
Contributor

I have a macro like this:

#[macro_export]
macro_rules! define_proc_macros {
    (
        $(
            $( #[$attr:meta] )*
            pub fn $proc_macro_name: ident ($input: ident : &str) -> String
            $body: block
        )+
    ) => {
        $(
            $( #[$attr] )*
            #[proc_macro_derive($proc_macro_name)]
            pub fn $proc_macro_name(derive_input: ::proc_macro::TokenStream)
                                    -> ::proc_macro::TokenStream {
                let $input = derive_input.to_string();
                let $input = $crate::_extract_input(&$input);
                $body.parse().unwrap()
            }
        )+
    }
}

pub fn _extract_input(derive_input: &str) -> &str {
    // …
}

When I use it, I get the warning in the title twice for every generated functions. When I move things into one crate to avoid "note: this error originates in a macro outside of the current crate", the span of the warning is on $proc_macro_name in #[proc_macro_derive($proc_macro_name)].

Steps to reproduce: runCARGO_INCREMENTAL=1 cargo +nightly build -p cssparser-macros in https://github.com/servo/rust-cssparser/.

I don’t understand this warning and what I can do about it. What does quasi-quoting mean in this context? How bad is the inefficiency really? Is it possible to fix it while keeping this macro’s flexibility?

CC @michaelwoerister and @nikomatsakis for #37787 that added this warning.

@michaelwoerister
Copy link
Member

That's strange. @jseyfried, do you see anything in the macro above that in a Token::Interpolated?
This is where the warning is generated by the way:

token::Token::Interpolated(ref non_terminal) => {

@michaelwoerister michaelwoerister added the A-incr-comp Area: Incremental compilation label Apr 4, 2017
@michaelwoerister
Copy link
Member

ping @jseyfried: Do you know what could be going on here? Where does the macro definition above introduce a Token::Interpolated?

@jseyfried jseyfried self-assigned this Apr 24, 2017
@jseyfried
Copy link
Contributor

The $proc_macro_name turns into a Nonterminal::Ident token. Normally, nonterminals are parsed into AST and don't cause warnings. However, now that #40346 has landed, attributes can have arbitrary tokens and the Nonterminal::Ident token ends up in the final AST/HIR.

I believe the solution here is to expand interpolated/nonterminal tokens into their underlying terminal tokens as we lower to HIR.

@SimonSapin
Copy link
Contributor Author

@jseyfried If I understand correctly, something is inefficient but what needs to change to fix it is in rustc rather than in my code?

@michaelwoerister
Copy link
Member

@SimonSapin That is correct. It's an inefficiency in rustc's handling of some parts of the HIR/AST.

@michaelwoerister
Copy link
Member

I believe the solution here is to expand interpolated/nonterminal tokens into their underlying terminal tokens as we lower to HIR.

@jseyfried, that sounds like a great solution. Would that be much work?

@jseyfried
Copy link
Contributor

@michaelwoerister
Once #40939 lands it should be simple.

@rushmorem
Copy link

I'm getting this warning too when using proc-macro-hack (cc @dtolnay). Is there any way to suppress it?

warning: Quasi-quoting might make incremental compilation very inefficient: NtIdent(..)
  --> derive/src/lib.rs:9:1
   |
9  |   proc_macro_expr_impl! {
   |  _^ starting here...
10 | |     pub fn args_impl(input: &str) -> String {
11 | |         args::Args::new(input)
12 | |             .process().tokens
13 | |             .to_string()
14 | |     }
15 | | }
   | |_^ ...ending here
   |
   = note: this error originates in a macro outside of the current crate

@seanmonstar
Copy link
Contributor

If it will take a while to fix this, it would be nice to at least be able to suppress the warning.

@michaelwoerister
Copy link
Member

@jseyfried Is there an ETA for #40939?

@michaelwoerister
Copy link
Member

Now that #40939 has landed, will you look into a fix, @jseyfried?

@jseyfried
Copy link
Contributor

@michaelwoerister Sure, I still think #40946 (comment) is the best fix; I'll implement this week.

@michaelwoerister
Copy link
Member

Thanks a lot, @jseyfried ❤️

kennytm added a commit to kennytm/rust that referenced this issue Jul 17, 2017
After rust-lang#43252 is merged, building stage0 libcore with -i (--incremental)
flag will cause 17 "Quasi-quoting might make incremental compilation very
inefficient: NtExpr(..)" warnings, as in rust-lang#40946.

Fixing the warning in rust-lang#40946 will take 12 weeks from now to make into the
next stage0, so it is quicker to workaround it in libcore instead.
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Jul 18, 2017
…n-rustbuild, r=alexcrichton

Workaround "Quasi-quoting is inefficient" warning in incremental rustbuild introduced in rust-lang#43252.

After rust-lang#43252 is merged, building stage0 libcore with `-i` (`--incremental`) flag will cause 17 "Quasi-quoting might make incremental compilation very inefficient: NtExpr(..)" warnings, as in rust-lang#40946.

```
warning: Quasi-quoting might make incremental compilation very inefficient: NtExpr(..)
   --> src/libcore/default.rs:133:21
    |
133 |             #[doc = $doc]
    |                     ^^^^
...
139 | default_impl! { (), (), "Returns the default value of `()`" }
    | ------------------------------------------------------------- in this macro invocation
(× 17)
```

True fix for rust-lang#40946 will take at least 12 weeks from now to make into the next stage0, so it is quicker to workaround it in libcore instead.

cc @vbrandl @jseyfried
@Mark-Simulacrum Mark-Simulacrum added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Jul 27, 2017
@nikomatsakis nikomatsakis added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 16, 2017
@nikomatsakis
Copy link
Contributor

@jseyfried any updates on status of this?

@alexcrichton
Copy link
Member

I've made a PR for this at #44601, although @jseyfried if that's not at all what you were thinking, just let me know!

alexcrichton added a commit to alexcrichton/rust that referenced this issue Sep 19, 2017
Right now the HIR contains raw `syntax::ast::Attribute` structure but nowadays
these can contain arbitrary tokens. One variant of the `Token` enum is an
"interpolated" token which basically means to shove all the tokens for a
nonterminal in this position. A "nonterminal" in this case is roughly analagous
to a macro argument:

    macro_rules! foo {
        ($a:expr) => {
            // $a is a nonterminal as an expression
        }
    }

Currently nonterminals contain namely items and expressions, and this poses a
problem for incremental compilation! With incremental we want a stable hash of
all HIR items, but this means we may transitively need a stable hash *of the
entire AST*, which is certainly not stable w/ node ids and whatnot. Hence today
there's a "bug" where the "stable hash" of an AST is just the raw hash value of
the AST, and this only arises with interpolated nonterminals. The downside of
this approach, however, is that a bunch of errors get spewed out during
compilation about how this isn't a great idea.

This PR is focused at fixing these warnings, basically deleting them from the
compiler. The implementation here is to alter attributes as they're lowered from
the AST to HIR, expanding all nonterminals in-place as we see them. This code
for expanding a nonterminal to a token stream already exists for the
`proc_macro` crate, so we basically just reuse the same implementation there.

After this PR it's considered a bug to have an `Interpolated` token and hence
the stable hash implementation simply uses `bug!` in this location.

Closes rust-lang#40946
bors added a commit that referenced this issue Sep 19, 2017
rustc: Forbid interpolated tokens in the HIR

Right now the HIR contains raw `syntax::ast::Attribute` structure but nowadays
these can contain arbitrary tokens. One variant of the `Token` enum is an
"interpolated" token which basically means to shove all the tokens for a
nonterminal in this position. A "nonterminal" in this case is roughly analagous
to a macro argument:

    macro_rules! foo {
        ($a:expr) => {
            // $a is a nonterminal as an expression
        }
    }

Currently nonterminals contain namely items and expressions, and this poses a
problem for incremental compilation! With incremental we want a stable hash of
all HIR items, but this means we may transitively need a stable hash *of the
entire AST*, which is certainly not stable w/ node ids and whatnot. Hence today
there's a "bug" where the "stable hash" of an AST is just the raw hash value of
the AST, and this only arises with interpolated nonterminals. The downside of
this approach, however, is that a bunch of errors get spewed out during
compilation about how this isn't a great idea.

This PR is focused at fixing these warnings, basically deleting them from the
compiler. The implementation here is to alter attributes as they're lowered from
the AST to HIR, expanding all nonterminals in-place as we see them. This code
for expanding a nonterminal to a token stream already exists for the
`proc_macro` crate, so we basically just reuse the same implementation there.

After this PR it's considered a bug to have an `Interpolated` token and hence
the stable hash implementation simply uses `bug!` in this location.

Closes #40946
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-incr-comp Area: Incremental compilation C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants