proc_macro! expansion can refer to items defined in the root w/o importing them

[japaric]

Apologies if this has already been reported; I didn't search too hard.

STR

// bar/src/lib.rs

#![feature(proc_macro)]

extern crate proc_macro;
#[macro_use]
extern crate quote;

use proc_macro::TokenStream;

#[proc_macro]
pub fn bar(_: TokenStream) -> TokenStream {
    quote!(mod __bar {
        // NOTE `Foo` is never imported in this module
        static mut BAR: Option<Foo> = None;
    }).into()
}

// foo/src/lib.rs

#![feature(proc_macro)]

extern crate bar;

use bar::bar;

struct Foo;

bar!();

This code compiles but I would expect it not to because the expanded form doesn't compile:

// foo/src/lib.rs -- `cargo expand`-ed and cleaned up
struct Foo;

mod __bar {
    static mut BAR: Option<Foo> = None;
}

$ cargo build
error[E0412]: cannot find type `Foo` in this scope
 --> src/lib.rs:4:28
  |
4 |     static mut BAR: Option<Foo> = None;
  |                            ^^^ not found in this scope
help: possible candidate is found in another module, you can import it into scope
  |
4 |     use Foo;
  |

error: aborting due to previous error

Meta

$ rustc -V
rustc 1.27.0-nightly (428ea5f6b 2018-05-06)

cc @jseyfried

[alexcrichton]

@petrochenkov do you know if this might be related to #50050? The hygiene here is indeed quite suspicious

[petrochenkov]

This looks like a different issue to me at a first glance (but we need to check on older releases whether this is a regression or not).
Or even two issues?
First, isn't quote supposed to have def-site hygiene? Here it exhibits something call-site-like.
Second, the bar's call-site location (in module system), in which Foo is in scope, is somehow mixed up with its call-site hygienic context.
At least quote! is not stabilized.

[alexcrichton]

@petrochenkov oh this is using the quote crate, not the one in proc_macro

[alexcrichton]

Looks like this is a bug on stable Rust --

// foo.rs
#![crate_type = "proc-macro"]

extern crate proc_macro;

use proc_macro::*;

#[proc_macro_derive(Foo)]
pub fn foo1(a: TokenStream) -> TokenStream {
    drop(a);
    "mod __bar { static mut BAR: Option<Something> = None; }".parse().unwrap()
}

// bar.rs
#[macro_use]
extern crate foo;

struct Something;

#[derive(Foo)]
struct Another;

fn main() {}

$ rustc +stable foo.rs && rustc +stable bar.rs -L .
... only warnings printed

[alexcrichton]

cc @nrc, you may want to take a look at this as well, especially because it's a bug on stable Rust which fixing may cause a lot of regressions

[nrc]

This looks correct to me - aiui we use call-site hygiene for proc macros, and so we're using the hygiene context from where bar! is written to resolve the name Foo, and it does exist there. Long term, we should be using def-site hygiene here and so it wouldn't compile because there is no Foo where bar is defined.

[alexcrichton]

@nrc hm but we've been advocating that "hygiene" in expanded macros for 1.1/1.2 is basically "copy/paste" hygiene, but in this case it's not?

[nrc]

No, this is very much a limitation of the hygiene algorithm here - in the proper hygiene setup when you create a new scope in a macro, you get a combination of the hygiene info for the new scope plus the 'base scope'. The intuition you want is that the base scope is the call-site (that gives 'copy and paste' hygiene). However, the macros 1.1/1.2 algorithm is essentially the base scope everywhere (i.e., we ignore any scopes introduced by the macro) and the base scope is the call site.

[petrochenkov]

@nrc
How do you plan to move to the "proper hygiene setup" from the current scheme (which seems incorrect/underimplemented to me) if proc_macro/proc_macro_attribute are stabilized now?

[petrochenkov]

Even more alarming example:

// foo.rs
#![crate_type = "proc-macro"]
extern crate proc_macro;
use proc_macro::*;

#[proc_macro_derive(Foo)]
pub fn foo1(_: TokenStream) -> TokenStream {
    "
    struct Outer;
    mod inner {
        type Inner = Outer; // `Outer` shouldn't be available from here
    }
    ".parse().unwrap()
}

// bar.rs
#![allow(unused)]

#[macro_use]
extern crate foo;

#[derive(Foo)] // OK?!
struct Dummy;

fn main() {}

I.e. the current implementation of call-site hygiene effectively flattens any module structures existing inside the macro.

[petrochenkov]

Perhaps we need to add some extra feature gate for macros generating modules?
(Possibly retroactively affecting proc_macro_derive).
Non-module scoping is not affected as far as I can conclude from some quick experiments.

[alexcrichton]

Yeah if this is the current state of hygiene then I'd want to definitely gate modules from being in the output of procedural macros and procedural attributes. I'd ideally prefer to aggressviely start warning against modules in the output of custom derive as well.

[alexcrichton]

I've posted #50587 to help evaluate the impact of forbidding modules from being generated by procedural derive macros

[alexcrichton]

Given the crater results it seems clear that we cannot retroactively prevent custom derive from generating modules. Current "offenders" are:

• pest_derive - #[derive(Parser)]
• vulkano-shader-derive - #[derive(VulkanoShader)]
• diesel - #[derive(QueryId)]
• soa_derive - #[derive(StructOfArray)]
• enumflags_derive - #[derive(EnumFlags)]
• robin-derives - #[derive(Builder)]
• juniper_codegen - #[derive(GraphQLEnum)]
• prost-derive - #[derive(Message)]
• ethabi-derive - ??

My PR purely disallowed mod items being generated and did not fix hygiene or anything like that. It's unknown what the fallout would be if we were to fix issues like mentioned above.

I'd still like to gate such features by default for maros 1.2 (attributes and bang macros) as I'd like to avoid worsening this problem further

[alexcrichton]

I've opened #50820 to separately gate procedural bang and attribute macros, and will leave custom derive alone for now.

[petrochenkov]

Looks like this issue is going to be fixed as well as a part of #50050.

[petrochenkov]

This is not #50050, but a separate issue after all.

This is what happens:

• During expansion InvocationCollector::collect creates a fresh expansion ID (aka Mark) for each macro invocation (like bar!() in the example above).
• Later expansion algorithm associate this ID with its position in the module system in various side tables like macro_defs/invocations in Resolver.
• Proc macros use the ID mentioned above for def-site hygiene, but they have to create an absolutely fresh expansion ID for call-site hygiene (currently in TokenStream::from_str) to assign the new transparency to it.
• The new ID remains unassociated with positions in the module system and due to various fallbacks ends up associated with the root module.

Possible solution: put the newly created ID into the same resolver tables with same values as the original ID.

Probably better solution: decouple expansion ID from transparency. Each token produced by an expansion can have an independent transparency, so we won't have to create fresh IDs to change transparency.

EDIT: Expansion IDs remaining unassociated with positions in module system are indeed an issue (fixed in #51952), but the issue was caused by other reason, namely this fallback in name resolution

rust/src/librustc_resolve/lib.rs

Lines 1948 to 1961 in 0c0315c

	let mut module_expansion = module.expansion.modern(); // for backward compatibility
	while let Some(parent) = module.parent {
	let parent_expansion = parent.expansion.modern();
	if module_expansion.is_descendant_of(parent_expansion) &&
	parent_expansion != module_expansion {
	return if parent_expansion.is_descendant_of(span.ctxt().outer()) {
	Some(parent)
	} else {
	None
	};
	}
	module = parent;
	module_expansion = parent_expansion;
	}

that worked for all proc macros 2.0 and declarative macros 2.0.
#51952 removes this fallback from all macros except for proc macro derives, in which it's reported as a compatibility warning.

[iddm]

How can I fix these errors now?

warning: cannot find type `users` in this scope
  --> src/models.rs:45:37
   |
45 | #[derive(Debug, Clone, AsChangeset, Insertable, Serialize, Deserialize)]
   |                                     ^^^^^^^^^^ names from parent modules are not accessible without an explicit import
   |
   = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
   = note: for more information, see issue #50504 <https://github.com/rust-lang/rust/issues/50504>

[alexcrichton]

@vityafx unfortunately as a user you can't do anything to fix that error, the fix would need to be in diesel's Insertable custom derive. In the meantime you can allow the lint though.

[lukedupin]

Here is an example of how to suppress the warning:
export RUSTFLAGS="-Aproc-macro-derive-resolution-fallback"

[apiraino]

Nitpicking on @lukedupin solution; maybe better adding:

#![allow(proc_macro_derive_resolution_fallback)]

at the top of your files that trigger the warning.

Setting an env variable somehow invalidates the compiler cache, therefore:

$ cargo check

and:

$ RUSTFLAGS="-Aproc-macro-derive-resolution-fallback" cargo check

are not the same and running one or the another will trigger a full (possible long) rebuild; f.e. your IDE may use cargo check for linting without setting that env var.

[xpe]

@apiraino I'd like to add a small but important correction to what you wrote, which suggested using an outer attribute, prefixed with #. Here is the proper syntax to use at the top of a file to disable the warning:

#![allow(proc_macro_derive_resolution_fallback)]

This uses an inner attribute, prefixed with #!, which includes the bang.

[Boscop]

Btw, I'm getting these kind of warnings also for diesel's AsChangeset, Insertable, Associations, Identifiable, Queryable and QueryableByName.

[kotauskas]

I feel like there's a bug with the lint itself. My proc macro expands to the following (example taken from its unit test, slightly modified to demonstrate the problem):

// Not a part of actual macro output, but is instead present at call site. I'm parsing the input struct's type
// specificially (the macro is a derive macro), so it's fine to avoid fully qualifying it, since use super::*;
// will take care of everything.
use std::time::SystemTime; 
mod entries {
    use super::*;
    #[doc = "The entry identifier type for the `field` field in the `MyConfigTable` config table."]
    pub enum Field {}
    impl ::snec::Entry for Field {
        type Data = SystemTime; // This is not in the prelude 
        const NAME: &'static str = "field";
    }
};
...

The use super::*; there is what is expected to make the name resolution work correctly instead of failing because of the module. The macro's output compiles just fine, but produces the lint.

Even though I'm completely not familiar with the implementation of the lint itself, I suspect that the culprit is that it simply checks for relative names which would are valid outside the module. I don't know whether it's implemented like this though, that's just a wild guess.