Missing tokens in custom derive input generated by macro #51331

dtolnay · 2018-06-03T17:45:39Z

The script below has a macro_rules macro that expands to:

#[derive(Print)]
pub struct S(<MyDispatch as $crate::Dispatch>::Call);

But inside of the derive(Print) custom derive the tokens received are:

pub struct S(<MyDispatch as ::repro_dispatch>::Call);

The ::Dispatch seems to be missing.

Originally reported in serde-rs/serde#1306.

rustc 1.28.0-nightly (4ecf12b 2018-06-02)
Repro script:

#!/bin/sh

cargo new --lib repro
cargo new --lib repro_dispatch
cargo new --lib repro_derive

echo >repro/src/lib.rs '
#[macro_use]
extern crate repro_dispatch;

#[macro_use]
extern crate repro_derive;

pub struct MyDispatch;

impl repro_dispatch::Dispatch for MyDispatch {
    type Call = ();
}

problem!();
'

echo >>repro/Cargo.toml '
repro_dispatch = { path = "../repro_dispatch" }
repro_derive = { path = "../repro_derive" }
'

echo >repro_dispatch/src/lib.rs '
pub trait Dispatch {
    type Call;
}

#[macro_export]
macro_rules! problem {
    () => {
        #[derive(Print)]
        pub struct S(<MyDispatch as $crate::Dispatch>::Call);
    }
}
'

echo >repro_derive/src/lib.rs '
extern crate proc_macro;
use proc_macro::TokenStream;

#[proc_macro_derive(Print)]
pub fn derive(input: TokenStream) -> TokenStream {
    println!("\n\n{}\n\n", input);
    "".parse().unwrap()
}
'

echo >>repro_derive/Cargo.toml '
[lib]
proc-macro = true
'

cargo build --manifest-path repro/Cargo.toml

The text was updated successfully, but these errors were encountered:

dtolnay · 2018-06-03T17:48:35Z

The behavior is correct with rustc 1.15.0 but the current incorrect behavior exists since 1.16.0.

nikomatsakis · 2018-06-07T14:26:11Z

cc @dtolnay @rust-lang/release — perhaps somebody could bisect this to a specific nightly? (I guess it's too old to get a specific PR)

cargo-bisect-rustc may help here :)

nikomatsakis · 2018-06-07T14:28:16Z

Also, how does it come to happen that $crate is still in there and not replaced during macro expansion...? Maybe I'm unclear on the phasing of how $crate works.

nikomatsakis · 2018-06-07T14:28:39Z

Marking as P-high but don't have an assignee yet, will revisit next week when we've hopefully had a chance to bisect.

Mark-Simulacrum · 2018-06-07T15:15:59Z

2016-12-06 has the odd result of pub struct S(<MyDispatch as ::::Dispatch>::Call);.

2016-12-27 is the first nightly to have pub struct S(<MyDispatch as ::repro_dispatch>::Call); as the result; the previous nightly 2016-12-23 has the (correct, I presume) pub struct S(<MyDispatch as ::repro_dispatch::Dispatch>::Call); result.

Mark-Simulacrum · 2018-06-07T15:24:12Z

These are the bors commits between those two nightlies (a173778...77f7c7a).

77f7c7a Auto merge of #38274 - elahn:windows-readconsole-ctrl-z, r=alexcrichton
b7e5148 Auto merge of #38314 - japaric:do-not-delete-enable-llvm-backend, r=alexcrichton
f536d90 Auto merge of #38542 - YaLTeR:fastcall-fix, r=pnkfelix
65c043f Auto merge of #38154 - petrochenkov:altname, r=jseyfried
8493dbe Auto merge of #38536 - retep998:flauschige-kaninchen, r=petrochenkov
5752eae Auto merge of #38598 - brson:em, r=alexcrichton
44ad63e Auto merge of #38490 - jseyfried:def_id_vis, r=nrc
20b6c16 Auto merge of #38539 - jseyfried:fix_resolve_hang, r=eddyb
c74ac6c Auto merge of #38566 - jseyfried:fix_import_resolution_bug, r=eddyb
e60aa62 Auto merge of #38594 - steveklabnik:rollup, r=steveklabnik
00e61d4 Auto merge of #38443 - frewsxcv:file-docs, r=brson
d86cf13 Auto merge of #38062 - alexcrichton:fix-line-writer, r=brson
8d65c8d Auto merge of #38268 - withoutboats:parse_where_higher_rank_hack, r=eddyb
4d07320 Auto merge of #38523 - camlorn:disable_field_reordering, r=nikomatsakis
467a7f0 Auto merge of #38533 - jseyfried:legacy_custom_derive_deprecation, r=nrc
00b4019 Auto merge of #38529 - nrc:save-sig, r=nikomatsakis
ce4461f Auto merge of #38511 - Mark-Simulacrum:drop-glue, r=eddyb
99913c5 Auto merge of #38401 - redox-os:redox_cross, r=brson
82611a0 Auto merge of #38232 - jseyfried:refactor_global_paths, r=nrc
c8e7ec4 Auto merge of #38562 - brson:rm-llvm-lock, r=brson

I think #38232, #38566, #38539, #38490, #38154 are possible candidates based on branch names. I'll try to run git bisect in this commit range.

Mark-Simulacrum · 2018-06-07T20:47:30Z

I was.. largely unsuccessful at running git bisect. Maybe someone else will have more luck.

ExpHP · 2018-06-08T11:46:09Z

I figure it's easier to bisect the Bors commits which are sure to build correctly, rather than to run git bisect. Overnight I ran a build of 467a7f0, and got the erroneous output, cutting it down to these:

467a7f0 Auto merge of #38533 - jseyfried:legacy_custom_derive_deprecation, r=nrc
00b4019 Auto merge of #38529 - nrc:save-sig, r=nikomatsakis
ce4461f Auto merge of #38511 - Mark-Simulacrum:drop-glue, r=eddyb
99913c5 Auto merge of #38401 - redox-os:redox_cross, r=brson
82611a0 Auto merge of #38232 - jseyfried:refactor_global_paths, r=nrc
c8e7ec4 Auto merge of #38562 - brson:rm-llvm-lock, r=brson

My money's on jseyfried:refactor_global_paths but I won't be able to run another build until I get to work.

Slightly modified repro script for working with such old versions of rust

#!/bin/sh

# set the linked toolchain here
TOOLCHAIN=stage2

rm -rf repro
rm -rf repro_dispatch
rm -rf repro_derive
cargo new --lib repro
cargo new --lib repro_dispatch
cargo new --lib repro_derive

echo >repro/src/lib.rs '
#[macro_use]
extern crate repro_dispatch;

#[macro_use]
extern crate repro_derive;

pub struct MyDispatch;

impl repro_dispatch::Dispatch for MyDispatch {
    type Call = ();
}

problem!();
'

echo >>repro/Cargo.toml '
repro_dispatch = { path = "../repro_dispatch" }
repro_derive = { path = "../repro_derive" }
'

echo >repro_dispatch/src/lib.rs '
pub trait Dispatch {
    type Call;
}

#[macro_export]
macro_rules! problem {
    () => {
        #[derive(Print)]
        pub struct S(<MyDispatch as $crate::Dispatch>::Call);
    }
}
'

echo >repro_derive/src/lib.rs '
#![feature(proc_macro, proc_macro_lib)]
extern crate proc_macro;
use proc_macro::TokenStream;

#[proc_macro_derive(Print)]
pub fn derive(input: TokenStream) -> TokenStream {
    println!("\n\n{}\n\n", input);
    "".parse().unwrap()
}
'

echo >>repro_derive/Cargo.toml '
[lib]
proc-macro = true
'

CARGO_INCREMENTAL=0 cargo +$TOOLCHAIN build --manifest-path repro/Cargo.toml
CARGO_INCREMENTAL=0 cargo +$TOOLCHAIN rustc --manifest-path repro/Cargo.toml -- --version

ExpHP · 2018-06-08T19:16:59Z

Okay, I have confirmed that 82611a0 exhibits the issue, and c8e7ec4 does not, so it is #38232. cc @jseyfried

ExpHP · 2018-06-10T19:33:22Z

The following test case (replacing the similarly-named macro in the OP) helps clarify the nature of the bug. It is not "overwriting" the Dispatch path component, but rather, it is missing the entire last path component (with type parameters):

#[macro_export]
macro_rules! problem {
    () => {
        #[derive(Print)]
        pub struct S(<MyDispatch as $crate::Dispatch::Second<Test>>::Call);
    }
}

The ::Second<Test> is dropped:

pub struct S(<MyDispatch as ::repro_dispatch::Dispatch>::Call);

(Note: tested on nightly, because I foolishly did not keep my build of 82611a0)

Edit: Additionally, only the trait path is affected. Adding $crate before MyDispatch works fine.

The PR is largely one refactoring commit. I've been reading through looking for the kinds of things that might enable a bug like this, but haven't found the culprit.

ExpHP · 2018-06-10T22:02:29Z

I have a theory. I believe this was buggy prior to jseyfried's PR, but the PR caused the bug to affect this specific test case.

The following function (used exclusively by libsyntax_ext) uses a Folder to normalize $crate away from paths:

https://github.com/jseyfried/rust/blob/f10f50b42639718b2580d10802f05f2b6ff209d5/src/librustc_resolve/macros.rs#L102-L123

However, the trait path is stored in an ast::QSelf, which to this day stores a position field indicating the end of the trait path. The method linked above only overrides fold_path, but the default definitions for fold_ty, fold_expr, and fold_pat all simply fold the path and keep the old position.

Prior to the PR, a path segment was deleted if (and only if) the macro was local. After the PR, a path segment is inserted if (and only if) the macro was from another crate. In each of these cases, the QSelf position field is invalidated.

If my theory is correct, then prior to #38232, it should have manifested as printing extra tokens if the definition of problem! is moved from repro_dispatch to repro. (However I no longer have the c8e7ec4 build, and I'm not sure where @Mark-Simulacrum is successfully downloading ancient nightlies from!)

Mark-Simulacrum · 2018-06-10T22:09:42Z

I'm downloading nightlies via cargo-bisect-rustc's bisection; I also found that for some reason rustup itself refuses to install those nightlies.

ExpHP · 2018-06-10T22:55:22Z

Thanks!

I was able to verify my prediction; nightly-2016-12-23 handles the original test case correctly, but duplicates the ::Call when the macro is defined locally:

#[nonlocal]
pub struct S(<MyDispatch as ::repro_dispatch::Dispatch::Second<Test>>::Call);

#[local]
pub struct S(<MyDispatch as ::Dispatch::Second<Test>::Call>::Call);

Repro with local and nonlocal macros

#!/bin/sh

# set the linked toolchain here
TOOLCHAIN=bisector-nightly-2016-12-23-x86_64-unknown-linux-gnu

rm -rf repro
rm -rf repro_dispatch
rm -rf repro_derive
cargo new --lib repro
cargo new --lib repro_dispatch
cargo new --lib repro_derive

echo >repro/src/lib.rs '
#[macro_use]
extern crate repro_dispatch;

#[macro_use]
extern crate repro_derive;

pub struct MyDispatch;

impl repro_dispatch::Dispatch for MyDispatch {
    type Call = ();
}

#[macro_export]
macro_rules! problem_local {
    () => {
        #[derive(Print)]
        #[local]
        pub struct S(<MyDispatch as $crate::Dispatch::Second<Test>>::Call);
    }
}

problem!();
problem_local!();
'

echo >>repro/Cargo.toml '
repro_dispatch = { path = "../repro_dispatch" }
repro_derive = { path = "../repro_derive" }
'

echo >repro_dispatch/src/lib.rs '
pub trait Dispatch {
    type Call;
}

#[macro_export]
macro_rules! problem {
    () => {
        #[derive(Print)]
        #[nonlocal]
        pub struct S(<MyDispatch as $crate::Dispatch::Second<Test>>::Call);
    }
}
'

echo >repro_derive/src/lib.rs '
#![feature(proc_macro, proc_macro_lib)]
extern crate proc_macro;
use proc_macro::TokenStream;

#[proc_macro_derive(Print)]
pub fn derive(input: TokenStream) -> TokenStream {
    println!("\n\n{}\n\n", input);
    "".parse().unwrap()
}
'

echo >>repro_derive/Cargo.toml '
[lib]
proc-macro = true
'

CARGO_INCREMENTAL=0 cargo +$TOOLCHAIN build --manifest-path repro/Cargo.toml
CARGO_INCREMENTAL=0 cargo +$TOOLCHAIN rustc --manifest-path repro/Cargo.toml -- --version

I'll start working on a PR.

ExpHP · 2018-06-10T23:20:57Z

Silly question: Would adding an AST type like

pub struct QPath(Option<QSelf>, Path);

be overkill? This pattern describes all appearances of QSelf in the AST, and I feel the most principled solution to this bug would be to add a fold method for this pairing of types. (whereas if I just overrode fold_expr, fold_ty, and fold_pat in the one function linked above, it could easily get neglected the next time QSelf is added somewhere)

Edit: It appears that such a type in fact used to exist (evidence), which kind of explains the bizarre name of QSelf. It was removed in d31b9eb for some reason.

Edit 2: also perhaps libsyntax is not as malleable as I thought, due to a number of rustc plugin crates that still exist...

ExpHP · 2018-06-11T23:12:44Z

Since I wasn't sure about the above, I wrote two possible fixes, both of which I am now testing:

A self-contained fix that makes EliminateCrateVar override fold_{expr,ty,pat}, but it's messy
A cleaner fix that involves adding the following method to syntax::fold::Folder:
```
fn fold_qpath(&mut self, Option<QSelf>, Path) -> (Option<QSelf>, Path)
```

petrochenkov · 2018-06-12T06:19:32Z

@ExpHP
New fold_qpath delegating to fold_path by default (without changing qpaths in AST) looks like a good solution.

ExpHP · 2018-06-12T13:02:42Z

Thanks for the feedback! PR is up.

Fix for $crate var normalization in proc macro for externally defined macros Fixes #51331, a bug that has existed in at least *some* form for a year and a half. The PR includes the addition of a `fold_qpath` method to `syntax::fold::Folder`. Overriding this method is useful for folds that modify paths in a way that invalidates indices (insertion or removal of a component), as it provides the opportunity to update `qself.position` in `<A as B>::C` paths. I added it because the bugfix is messy without it. (unfortunately, grepping around the codebase, I did not see anything else that could use it.)

dtolnay added A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) C-bug Category: This is a bug. labels Jun 3, 2018

dtolnay mentioned this issue Jun 3, 2018

Trait name lost with Derive(Deserialize) generated in remote crate. serde-rs/serde#1306

Closed

dtolnay added the regression-from-stable-to-stable Performance or correctness regression from one stable version to another. label Jun 3, 2018

rphmeier mentioned this issue Jun 6, 2018

Make substrate generic paritytech/substrate#169

Merged

7 tasks

nikomatsakis added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 7, 2018

nikomatsakis added the P-high High priority label Jun 7, 2018

ExpHP added a commit to exphp-forks/rust that referenced this issue Jun 12, 2018

fix issue rust-lang#51331 by updating qself.position

d13bfd2

ExpHP mentioned this issue Jun 12, 2018

Fix for $crate var normalization in proc macro for externally defined macros #51519

Merged

bors closed this as completed in #51519 Jun 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Missing tokens in custom derive input generated by macro #51331

Missing tokens in custom derive input generated by macro #51331

dtolnay commented Jun 3, 2018 •

edited

Loading

dtolnay commented Jun 3, 2018

nikomatsakis commented Jun 7, 2018

nikomatsakis commented Jun 7, 2018

nikomatsakis commented Jun 7, 2018

Mark-Simulacrum commented Jun 7, 2018

Mark-Simulacrum commented Jun 7, 2018 •

edited

Loading

Mark-Simulacrum commented Jun 7, 2018

ExpHP commented Jun 8, 2018 •

edited

Loading

ExpHP commented Jun 8, 2018

ExpHP commented Jun 10, 2018 •

edited

Loading

ExpHP commented Jun 10, 2018 •

edited

Loading

Mark-Simulacrum commented Jun 10, 2018

ExpHP commented Jun 10, 2018

ExpHP commented Jun 10, 2018 •

edited

Loading

ExpHP commented Jun 11, 2018 •

edited

Loading

petrochenkov commented Jun 12, 2018

ExpHP commented Jun 12, 2018

Missing tokens in custom derive input generated by macro #51331

Missing tokens in custom derive input generated by macro #51331

Comments

dtolnay commented Jun 3, 2018 • edited Loading

dtolnay commented Jun 3, 2018

nikomatsakis commented Jun 7, 2018

nikomatsakis commented Jun 7, 2018

nikomatsakis commented Jun 7, 2018

Mark-Simulacrum commented Jun 7, 2018

Mark-Simulacrum commented Jun 7, 2018 • edited Loading

Mark-Simulacrum commented Jun 7, 2018

ExpHP commented Jun 8, 2018 • edited Loading

ExpHP commented Jun 8, 2018

ExpHP commented Jun 10, 2018 • edited Loading

ExpHP commented Jun 10, 2018 • edited Loading

Mark-Simulacrum commented Jun 10, 2018

ExpHP commented Jun 10, 2018

ExpHP commented Jun 10, 2018 • edited Loading

ExpHP commented Jun 11, 2018 • edited Loading

petrochenkov commented Jun 12, 2018

ExpHP commented Jun 12, 2018

dtolnay commented Jun 3, 2018 •

edited

Loading

Mark-Simulacrum commented Jun 7, 2018 •

edited

Loading

ExpHP commented Jun 8, 2018 •

edited

Loading

ExpHP commented Jun 10, 2018 •

edited

Loading

ExpHP commented Jun 10, 2018 •

edited

Loading

ExpHP commented Jun 10, 2018 •

edited

Loading

ExpHP commented Jun 11, 2018 •

edited

Loading