Fix hygiene issues in declare_program! and declare_loader!

[Aaron1011]

The declare_program! and declare_loader! macros both expand to
new macro definitions (based on the $name argument). These 'inner'
macros make use of the special $crate metavariable to access items in
the crate where the 'inner' macros is defined.

However, this only works due to a bug in rustc. When a macro is
expanded, all $crate tokens in its output are 'marked' as being
resolved in the defining crate of that macro. An inner macro (including
the body of its arms) is 'just' another set of tokens that appears in
the body of the outer macro, so any $crate identifiers used there are
resolved relative to the 'outer' macro.

For example, consider the following code:

macro_rules! outer {
    () => {
        macro_rules! inner {
            () => {
                $crate::Foo
            }
        }
    }
}

The path $crate::Foo will be resolved relative to the crate that defines outer,
not the crate which defines inner.

However, rustc currently loses this extra resolution information
(referred to as 'hygiene' information) when a crate is serialized.
In the above example, this means that the macro inner (which gets
defined in whatever crate invokes outer!) will behave differently
depending on which crate it is invoked from:

When inner is invoked from the same crate in which it is defined,
the hygiene information will still be available,
which will cause $crate::Foo to be resolved in the crate which defines 'outer'.

When inner is invoked from a different crate, it will be loaded from
the metadata of the crate which defines 'inner'. Since the hygiene
information is currently lost, rust will 'forget' that $crate::Foo is
supposed to be resolved in the context of 'outer'. Instead, it will be
resolved relative to the crate which defines 'inner', which can cause
incorrect code to compile.

This bug will soon be fixed in rust (see rust-lang/rust#72121),
which will break declare_program! and declare_loader!. Fortunately,
it's possible to obtain the desired behavior ($crate resolving in the
context of the 'inner' macro) by use of a procedural macro.

This commit adds a respan! proc-macro to the sdk/macro crate.
Using the newly-stabilized (on Nightly) Span::resolved_at method,
the $crate identifier can be made to be resolved in the context of the
proper crate.

Since Span::resolved_at is only stable on the latest nightly,
referencing it on an earlier version of Rust will cause a compilation error.
This requires the rustversion crate to be used, which allows conditionally
compiling code epending on the Rust compiler version in use. Since this method is already
stabilized in the latest nightly, there will never be a situation where
the hygiene bug is fixed (e.g. rust-lang/rust#72121)
is merged but we are unable to call Span::resolved_at.

[Aaron1011]

You can test this code with the upcoming hygiene changes as follows:

1. Install https://github.com/kennytm/rustup-toolchain-install-master
2. Run rustup-toolchain-install-master 70fc6a85bc30b2537a0d3b43e27bfa981104d3c3
3. Run cargo +70fc6a85bc30b2537a0d3b43e27bfa981104d3c3 test

[Aaron1011]

cc @jackcmay: It looks like you originally wrote the declare_program! macro.

[jackcmay]

Thanks @Aaron1011 I'll take a look!

[Aaron1011]

It looks like CI doesn't like the FIXME comment I added. Should I open an issue to track switching to $crate::?

[jackcmay]

That would be great, thanks @Aaron1011

Btw, what brought our issue into your radar?

[Aaron1011]

@jackcmay: I've opened #10933 to track switching to $crate.

Btw, what brought our issue into your radar?

This crate regressed in a crater run when I was working on rust-lang/rust#72121. Fixing this crate should allow the rustc change to land with a minimum of ecosystem breakage.

[jackcmay]

@Aaron1011 FYI, this PR should fix the CI failures: Aaron1011#1

[Aaron1011]

@jackcmay: I've merged your PR

[Aaron1011]

The 2020-06-10nightly is too old - newer nightlies contain bufixes that allow this change to compile. Do you need to support that nightly specifically? If so, I can add in extra logic to the version detection code.

[jackcmay]

Which nightly do you suggest?

[Aaron1011]

The most recent one as of today should be fine. If you'd like, I can find the earliest one that will work.

[mvines]

We aren't picky about which nightly to depend on generally. If there's a known one that works we can just update https://github.com/solana-labs/solana/tree/master/ci/docker-rust-nightly to use it

[jackcmay]

And https://github.com/solana-labs/solana/blob/master/ci/rust-version.sh

@Aaron1011 you want to try one out in your PR?

[Aaron1011]

I've bumped CI to the latest nightly - that should allow this PR to compile on both the latest stable and nightly.

[jackcmay]

@Aaron1011 FYI: Bumping our rust nightly involes releasing a new docker image. I'm in the process of that now, will ping you when ready.

[jackcmay]

@Aaron1011 When bumping to 2020-07-08 nightly I see the following error:

error: variable does not need to be mutable
    --> runtime/src/message_processor.rs:1357:13
     |
1357 |         let mut accounts = vec![
     |             ----^^^^^^^^
     |             |
     |             help: remove this `mut`
     |
     = note: `-D unused-mut` implied by `-D warnings`

error: aborting due to previous error

error: could not compile `solana-runtime`.

Source code is here:

solana/runtime/src/message_processor.rs

Line 1357 in f1c1152

let mut accounts = vec![

Broken nightly?

[Aaron1011]

The nightly looks correct - it still compiles without mut. However, it does not compile on stable, as it looks like the compiler is unecessarily calling DerefMut. I suspect that rust-lang/rust#72280 is related. EDIT: This is definitely caused by that PR, see rust-lang/rust#73592 for another example of this kind of warning being generated.

We should be able to add #[allow(unused_mut)] to the statements in question.

[Aaron1011]

I've fixed all local issues I've found (by running the CI script manually)

[mergify]

automerge label removed due to a CI failure

[mvines]

@jackcmay - check out the stable-perf build failure, do we need an update for xargo or some other bpf-sdk component?

error[E0658]: procedural macros cannot expand to macro definitions
 --> /var/lib/buildkite-agent/builds/bernal-1/solana-labs/solana/sdk/src/entrypoint_native.rs:77:1
   \|
   77 \| #[rustversion::not(since(1.46.0))]
   \| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   \|
   = note: for more information, see https://github.com/rust-lang/rust/issues/54727
   = help: add `#![feature(proc_macro_hygiene)]` to the crate attributes to enable

   error: aborting due to previous error
    
   For more information about this error, try `rustc --explain E0658`.
   error: could not compile `solana-sdk`.

[jackcmay]

Probably, upgrade of rust-bpf or we can modify our bpf sysroot. Or maybe do as the message suggests and add the crate attribute?

[Aaron1011]

I've conditionally applied #![feature(proc_macro_hygiene)] when a dev build of Rust is detected (which might be the rust-bpf-builder toolchain).

[Aaron1011]

It looks like the automerge label didn't work?

[ryoqun]

It looks like the automerge label didn't work?

Yeah, it seems that....

@mvines Ok to merge this? Or should I wait to investigate something?

[mvines]

Ah crap, Travis CI is being silly again. They continually flag PRs from new contributors as "abuse" because we're a blockchain and therefore must be trying to mine BTC/ETH on their CI machines 📉

Thanks so much for this PR @Aaron1011

[Aaron1011]

This will need to be backported to the 1.2 and 1.1 branches if they are to continue compiling once the rustc change is merged. Should I open additional PRs, or is there a standard backporting process?

[mvines]

@Aaron1011 - I think it's ok if 1.1 don't support the latest nightly, it'll be EOLed at the end of the month. v1.2 will live on for another couple months so that backport to that branch be useful, I initiated a backport

[ryoqun]

here (context: anza-xyz#109 (comment))