hygiene: Decouple transparencies from expansion IDs

[petrochenkov]

And remove fallback to parent modules during resolution of names in scope.

This is a breaking change for users of unstable macros 2.0 (both procedural and declarative), code like this:

#![feature(decl_macro)]

macro m($S: ident) {
    struct $S;
    mod m {
        type A = $S;
    }
}

fn main() {
    m!(S);
}

or equivalent

#![feature(decl_macro)]

macro m($S: ident) {
    mod m {
        type A = $S;
    }
}

fn main() {
    struct S;
    m!(S);
}

stops working due to module boundaries being properly enforced.

For proc macro derives this is still reported as a compatibility warning to give actix_derive, diesel_derives and palette_derive time to fix their issues.

Fixes #50504 in accordance with this comment.

[rust-highfive]

r? @aturon

(rust_highfive has picked a reviewer for you, use r? to override)

[petrochenkov]

r? @oli-obk

[petrochenkov]

Needs check-only crater run (stable derives are affected).
ping @rust-lang/infra

[kennytm]

@bors try

[bors]

⌛️ Trying commit e0c3ee5 with merge 2b91a9e...

[oli-obk]

you lost a doc comment

[oli-obk]

can you check that we can still access things outside the expansion here?

[oli-obk]

and that we cannot access anything from outside the expansion without an import

[bors]

☀️ Test successful - status-travis
State: approved= try=True

[kennytm]

cc @pietroalbini check-only crater run wanted (details are in the crater status spreadsheet already).

[pietroalbini]

Crater run started. It should finish in ~2 days.

[pietroalbini]

Hi @petrochenkov (crater requester), @oli-obk (PR reviewer)! Crater results are at: http://cargobomb-reports.s3.amazonaws.com/pr-51952/index.html. 'Blacklisted' crates (spurious failures etc) can be found here. If you see any spurious failures not on the list, please make a PR against that file.

(interested observers: Crater is a tool for testing the impact of changes on the crates.io ecosystem. You can find out more at the repo if you're curious)

[pietroalbini]

52 crates regressed. Seems both actix and diesel are broken by this.

Dependency tree

The tool used to build the tree is not perfect, the tree might be a bit off. It only includes registry crates.

actix                     
| actix-web               
| | xiod                  
| | event-web             
| | | event-bot           
| | webthing              
| | dummyhttp             
| | actix-form-data       
| | kubeless              
| | sentry-actix          
| | h2n5                  
| | actix-web-httpauth    
| | | actix-httpbin       
| | actix-protobuf        
| | lockchain-http        
| | etf_balancer          
| | fie                   
| | actix-redis           
| | miniserve             
| | actix-web-sql-identity
| actix-ogn               
diesel_errors             
instaclone_backend        
cargo-tally               
finchers                  
nickel-diesel             
oxide-auth                
uuid-b64                  
diesel_geometry           
diesel_logger             
diesel_ltree              
migrations_internals      
| migrations_macros       
| | diesel_migrations     
| | | til                 
diesel_full_text_search   
diesel_derives_traits     
| diesel_derives_extra    
diesel-derive-newtype     
diesel-derive-more        
diesel-chrono-duration    
graphql_client            
robin                     
timeskwire                
rustep                    
palette                   
mocktopus                 
diesel-derive-enum        

[petrochenkov]

actix_derive, diesel_derives and palette_derive are root causes for most of regressions and they actually have bugs and use inaccessible names from parent modules.
(There are couple of other root regressions, but they use unstable proc macros.)
cc @fafhrd91 @sgrif @Ogeon @alexcrichton

The common pattern is

#[derive(Something)]
struct MyStruct;

being expanded into

struct MyStruct;

mod __implementation_details {
    // Note, that `MyStruct` is not in scope here.
    impl Something for MyStruct {
        let x: SomethingElseFromTheParentModule = ...;
    }
}

The "universal solution" is to do this:

mod __implementation_details {

    use super::*; // <---

    // Now `MyStruct` and `SomethingElseFromTheParentModule ` are in scope.
    impl Something for MyStruct {
       let x: SomethingElseFromTheParentModule = ...;
    }
}

Perhaps we can send fixes, release minor versions and land this fix.

[alexcrichton]

Thanks for investigating @petrochenkov! Out of curiosity, is it possible to apply this fix for just proc_macro and proc_macro_attribute? Or is it only feasible to apply this to all procedural macros?

In any case I think we should send fixes upstream and publish ASAP. Hopefully that'll buy us as many possibilities as we can to fix this!

[petrochenkov]

Alternative "universal solution":

fn __implementation_details() {
    impl Something for MyStruct {
       let x: SomethingElseFromTheParentModule = ...;
    }
}

This may look silly, but this may be even better solution for encapsulating implementation details than introducing a module (which acts as a "name barrier") and thus losing names in scope, including those that can't be recovered with use super::*.

For example, mod __implementation_details + use super::* won't work if the item with derive is itself placed in a function (or other non-module block).

fn outer() {
    #[derive(Something)]
    struct MyStruct;
}

[petrochenkov]

@alexcrichton

Out of curiosity, is it possible to apply this fix for just proc_macro and proc_macro_attribute?

This should be possible to do with a targeted hack, if I'm not mistaken about the root cause of the bug.
I'll try tomorrow.

[sgrif]

@alexcrichton I've opened #52545 which is related to this. It sounds like the fix for that is not straightforward or easy. Currently there's no obvious migration path for crates which are affected by this. What do you think about reverting this for the time being until there's a clear path forward for existing crates?

[sgrif]

I'm also surprised that this change was made so quietly. This is a pretty major breaking change to a feature that's been stable for a year and a half. I would like to think this would have at least received an RFC. This is very clearly not a soundness issue, and whether it is even a bug is arguable IMO.

This makes me extremely worried about whether Rust will actually live up to its stability promises. For a feature that has been stable for this long, where there were at least a dozen known cases of folks relying on this behavior, there really should have been more discussion -- not just on whether the behavior was intended or not, but on whether it's reasonable to change it at this point given that Rust is meant to be stable.

[petrochenkov]

We should not revert this PR as is and reintroduce the issues that were fixed here, that would be harmful for our hygiene system long-term.
The special compatibility code that is currently supported under lint is enough to cover diesel's use case for some time.
I think we can tweak the lint so it can be allow-ed from inside the diesel at most (currently it's attached to crate root and can't be disabled locally, but we can do better).

[petrochenkov]

These are all consequences of rushed stabilization of macros 1.1 and low "bus factor" for hygiene system (not enough qualified reviewing was done).

[sgrif]

While I appreciate where you're coming from, stability should not mean "unless we decide we change our minds". I agree with you that this is something that should have been discussed before the feature became stable in the first place, but that does not mean that it should be OK for the rug to get pulled out from under the ecosystem on a stable feature.

[alexcrichton]

@sgrif did diesel or any other crates you know of stop compiling as a result of this change? This change was landed under the presumption that nothing was broken and crates continued to compile, if that's not the case we should fix that!

If crates continued to compile, however, then I think it's a bit disingenous to say that the rug was pulled out from under you or that this is a major breaking change. This fixed bug exhibits behavior which is clearly a bug in procedural macros which has affected stable Rust for quite some time.

We always try quite hard to keep crates compiling on stable. This does not mean that we will prevent ourselves from fixing bugs though as we're doing in this situation. We are tasked, however, with fixing the bug while keeping existing code compiling.

[petrochenkov]

@sgrif
Bugs with compatibility implications regularly happened and will happen in the future. A lot of them are fixed silently and nobody notices, and for those with noticeable impact the process so far was to continue support, but issue a deprecation warning - exactly what was done here.

[sgrif]

@alexcrichton

did diesel or any other crates you know of stop compiling as a result of this change?

Yes, Diesel did. You also listed a ton of affected crates in #50504 (comment). Again, I think calling it a "fixed bug" is quite arguable. Personally I'd expect hygiene to mean that a given ident is resolved in the scope it was given from. My main point is that I don't think it's an obvious conclusion one way or the other, and I would have liked to see a discussion around it.

We always try quite hard to keep crates compiling on stable. This does not mean that we will prevent ourselves from fixing bugs though as we're doing in this situation.

I agree. But there does become a point when folks start relying on behavior enough that there is a conversation to be had about whether fixing the "bug" does more harm than good (there are several cases like this in Rails for example). At a certain point something in the grey area ends up being a defacto feature.

I think it's also important to define what "bug" means here. I couldn't find anything in the relevant RFCs which explicitly defines how this behavior should interact. At best this is a clarification of intent, but given that it's on a stable feature, I don't think that should be done without giving the community an opportunity to voice their opinion.

@petrochenkov

Bugs with compatibility implications regularly happened and will happen in the future.

I agree. I do not think this is obviously a bug. It's also much more nuanced than you are making it out to be, see my above comments.

[alexcrichton]

@sgrif so to be clear, diesel compiled on stable, and then the exact same source code no longer compiles on nightly? I'm not talking about warnings, but literal compiler errors. The crater report you gisted is not even tied to this change, so I'd just want to be clear about what breaking is. If it's broken, can you also gist the error message you're getting? (steps to reproduce would also be great!)

I agree there's possibility for a gray area, but I disagree that this is not obviously a bug. All of macros 1.1 is "call site" hygiene which is "as if the tokens are copy/pasted". That's clearly not the case when you're generating mod statements that don't actually import the items inside of them.

[sgrif]

@alexcrichton

so to be clear, diesel compiled on stable, and then the exact same source code no longer compiles on nightly? I'm not talking about warnings, but literal compiler errors.

Most published versions of Diesel were shipped with #[deny(warnings)] -- A practice that we have since stopped. That said, this is still going to eventually become an error. And regardless of whether something is an error or a warning, users will still report compiler warnings as bugs to crate maintainers, making it basically the same from our point of view.

The crater report you gisted is not even tied to this change, so I'd just want to be clear about what breaking is.

I linked to a discussion on the issue closed by this PR, not sure how it's unrelated. Pretty much every crate there is generating a module for the same reason -- to be able to have an isolated scope for use, and to be able to safely add extern crate to work around the lack of $crate.

I agree there's possibility for a gray area, but I disagree that this is not obviously a bug. All of macros 1.1 is "call site" hygiene which is "as if the tokens are copy/pasted". That's clearly not the case when you're generating mod statements that don't actually import the items inside of them.

This is a fair point. Really there's a broader issue here is that proc macros (and derives in particular) have no way to isolate their scope, and want to do things that don't have obvious interactions with hygiene (such as use statements). This is part of why I wish there had been a bit more discussion. Rather than the use case folks are relying on being considered, warnings/errors being generated for our users, and no workable path forward.

[petrochenkov]

All of macros 1.1 is ... "as if the tokens are copy/pasted".

I think this is what was specified in the RFC for 1.1 and announcement for 1.2, yes.
In other words, macros 1.1-1.2 don't provide "qualitatively new" abilities compared to non-macro code, i.e. just writing tokens by hand. (Compare with macro_rules that exhibit the same "copy-pasting" behavior and don't allow this trick.)

[alexcrichton]

@sgrif our stability policy does not consider new warnings or changes to warnings a breaking change due to #[deny(warnings)]. That's because it doesn't affect downstream usage of a crate, only the crate itself. That should mean that all crates depending on diesel continue to work, despite diesel itself needing a fix (which can be immediately remediated by removing deny(warnings) for the near-term).

The crater report you linked was a blanket denial of all procedural macros generating mod statements. This PR is not that change. This PR is a change that warns about using items that weren't imported in generated modules. While that report you linked may be similar to the fallout here, it is only tangentially related.

There is many many issues with procedural macros and how they do not have many hygiene controls, they're forced to just splat tokens into the user's program. These problems are well-known though and the solution is under development but is still not ready for stabilization.

[sgrif]

our stability policy does not consider new warnings or changes to warnings a breaking change due to #[deny(warnings)].

I understand that. This is not just a new default warn lint. This is a warning for what will end up being a breaking change. It also doesn't really matter in terms of the impact that this has on downstream crates. If someones build starts spamming warnings, we still get multiple reports a day (the most recent versions of Diesel do not #[deny(warnings)], but we have still gotten multiple reports about this on those versions)

The crater report you linked was a blanket denial of all procedural macros generating mod statements. This PR is not that change.

I looked through about half of them, and every single one I looked at is going to be affected by this change. As I said in my previous comment, there is very little reason to emit a module from a derive unless you are using it to isolate scope.

These problems are well-known though and the solution is under development but is still not ready for stabilization.

Has there been an RFC for this? This is my main point. This is a stable feature, and it really feels like things about it are just being changed with no input from the community.

[sgrif]

Regardless of anything else, I hope you understand the frustration here. A lot of folks use Diesel with Rocket or other crates that require nightly, so we tend to feel pains from that pretty quick. Users on older versions that still had #[deny(warnings)] have had their builds break completely. Others are just spammed with warnings. Both have opened issues and complained in our support channels. Today there is no migration path forward for us to fix this issue for our users.

[petrochenkov]

Sorry :(
I'll try to fix the lint so it can be allow-ed locally in diesel without bothering clients (this weekend most probably, maybe tomorrow).

[sgrif]

Even if we're able to allow it, that's at best a temporary fix. At the end of the day this is a compatibility warning for something that will be breaking eventually -- meaning that any version of Diesel (or any other affected crate) that doesn't handle it will stop working.

Saying that this isn't a breaking change because it's a warning, or that there'll be a fix that lets us silence the warning is seriously sidestepping the issue here. From a library maintainer's point of view, being told "your code will stop working" is no different from it not working. My concern is both with the lack of a working migration path, but also with this happening at all.

I understand that this is considered to be a bug by the relevant teams. However, I really don't think enough consideration has been given to existing code.

@alexcrichton directly said:

This change was landed under the presumption that nothing was broken and crates continued to compile

Since the only code that's going to stop compiling is code using #[deny(warnings)], that's of course true....... But that really sidesteps the fact that a bunch of code will stop compiling once this becomes a hard error.

[alexcrichton]

@sgrif yes I'm sorry that Diesel is going through these troubles, this particular migration could probably have gone more smoothly!

The warnings here were intended to be like all other future incompatibility warnings the compiler issues: they don't break existing code (modulo lint denial) and they are supressable in the near term if necessary. The migration path here was thought to be simple enough but it wasn't exercised for Diesel's case specifically and you've discovered there's a bug or two which prevents a feasible migration path, but we'll work on that!

The compiler has a well documented policy on how to fix a bug in the compiler. It appears we accidentally left out the tracking issue part of that, but otherwise that policy was followed. To reiterate some motivation of that policy, no future-incompatible compiler warning is turned into an error, ever, if enough of the ecosystem is still using it. We would never consider turning this into a hard error unless Diesel had successfully migrated to no longer have the warning issued. I disagree with your point of "is no different from it not working", this is the crucial distinction.

[petrochenkov]

#52589