Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign upStabilize attribute macros on inline modules #64273
Conversation
This comment has been minimized.
This comment has been minimized.
r? @eddyb (rust_highfive has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
Stabilization reportMacro attributes are now supported on inline module items ( The feature is a part of the franken-feature |
This comment was marked as resolved.
This comment was marked as resolved.
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
This comment has been minimized.
This comment has been minimized.
r? @Centril |
This comment was marked as resolved.
This comment was marked as resolved.
Ping from triage: |
This comment has been minimized.
This comment has been minimized.
(triage: I'll look at this PR during the weekend.) |
This comment was marked as resolved.
This comment was marked as resolved.
|
Stabilize macros in some more positions - Fn-like macros and attribute macros in `extern` blocks - Fn-like procedural macros in type positions - ~Attribute macros on inline modules~ (moved to rust-lang#64273) Stabilization report: rust-lang#63931 (comment). Closes rust-lang#49476 cc rust-lang#54727
Stabilize macros in some more positions - Fn-like macros and attribute macros in `extern` blocks - Fn-like procedural macros in type positions - ~Attribute macros on inline modules~ (moved to rust-lang#64273) Stabilization report: rust-lang#63931 (comment). Closes rust-lang#49476 cc rust-lang#54727
This comment has been minimized.
This comment has been minimized.
Declaring reviewer bankruptcy on this PR (sorry!) and reassigning to r? @pnkfelix who is considering the macro related changes together. (I'm a bit bummed by the visitor approach that is unfortunately necessary to land this. From a compiler POV it's trivial but from a spec POV it's not.) |
This comment has been minimized.
This comment has been minimized.
FWIW, that's the same approach that is used for gating macro-expanded |
This comment was marked as resolved.
This comment was marked as resolved.
Ping from triage |
This comment was marked as resolved.
This comment was marked as resolved.
Will do, after a review and FCP. |
This comment was marked as resolved.
This comment was marked as resolved.
joelpalmer
commented
Oct 14, 2019
Ping from triage: @Centril @pnkfelix @petrochenkov any updates? Should this have an FCP label? |
This comment has been minimized.
This comment has been minimized.
We discussed this in recent lang team meetings. The general agreement was that in the long term, we should resolve the semantics for how expansion interacts with out-of-line modules, which raises issues (aka technical artifacts, aka warts) such as the ones documented at #64197.
But we don't have to solve the problem of out-of-line modules now: The semantics that is being stabilized here for attributes on inline modules seems like the reasonable one to adopt, and however we resolve the out-of-line module case should be compatible with what is being stabilized here. The notes from the language team meeting include the following suggestion for the documentation:
In case you cannot tell, I was writing the above up as notes to prepare an FCP merge command. But given the questions I ended up posing to myself at the end of the notes, I want to hold off on actually issuing the FCP merge request until I have privately resolved my questions. I'm going to download this branch and play with it locally to try to ensure I understand the implications before firing off the FCP merge. |
This comment has been minimized.
This comment has been minimized.
Okay, I think I have reconciled the points that I noted in my earlier comment. The reason it probably makes sense to include documentation about the current semantics, even though the feature is unstable, is that the procedural macros are executed with the given inputs, before the stability error is issued. In other words, the macros observe the current token streams you get for out-of-line modules (and may exhibit some behavior based upon them), and thus it makes sense for us to document that. I also double-checked the current test suite. The tests that are affected by this are:
It would be good for the test suite to be expanded to include the case of modules within non-module blocks, like this: // A failing test
#[identity_attr]
fn bar() { #[path="outline_f.rs"] mod outline_f; } //~ ERROR ...
// A passing test
#[identity_attr]
fn baz() { mod inline_g { } } (More information about what I did in the details block.) I built this branch locally, made an attribute macro named #[styx]
mod inline_a { }
// OKAY
#[styx]
mod inline_b { mod inline_b1 { } }
// OKAY
#[styx]
fn foo() { mod inline_c { } }
// Got: error[E0658]: non-inline modules in proc macro input are unstable
#[styx]
mod outline_d;
// Got: error[E0658]: non-inline modules in proc macro input are unstable
#[styx]
mod inline_e { mod outline_e1; }
// FYI: Without a `path` attribute, this gets "Cannot declare a non-inline module inside a block unless it has a path attribute" (which seems good to me)
//
// Got: error[E0658]: non-inline modules in proc macro input are unstable
#[styx]
fn bar() { #[path="outline_f.rs"] mod outline_f; }
// OKAY
//
// Contains the text:
// ```
// use styx::styx;
//
// #[styx]
// mod inline_g1 { }
// ```
mod outline_g; |
This comment has been minimized.
This comment has been minimized.
I had originally written that merging this PR need not be held up on waiting for the addition of the test case I requested above. (And I still do believe that to be true.) However, I do think the requested documentation addition should be part of the stabilization here. I don't have time at the moment to try to figure out where that documentation addition belongs. @petrochenkov , if you have time in the next 24 hours or so to add that sentence in the appropriate place (and, if you can, the test I requested), then I'll issue an FCP merge request. Otherwise, if you don't have time, I'll try to figure it out myself tomorrow. |
This comment has been minimized.
This comment has been minimized.
What probably some proc. macros might want to do is to load those out-of-line modules into their attributed module definition. Would it make sense to also provide documentation about how those users should probably be tackling this issues as long as there are no stable alternatives? Recommendations how to tackle this potentially very common use case might be of use. For example, they might make use of |
This comment has been minimized.
This comment has been minimized.
Hmm. Establishing what the best-practices/recommendations are for those scenarios sounds like something that could be done with this PR, but it could also wait until later, no? Or at least, I personally don't feel comfortable trying to devise what the best-practices are there, but I also don't feel comfortable blocking stabilization of the inline module case on the development of those best-practices. |
This comment has been minimized.
This comment has been minimized.
Further discussion in tonight's lang team meeting led the T-lang members present to conclude that the FCP merge request does not need to be blocked on either of the points (the test I requested and the documentation addition). So, having said that: @rfcbot fcp merge |
This comment has been minimized.
This comment has been minimized.
rfcbot
commented
Nov 21, 2019
•
Team member @pnkfelix has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
This comment has been minimized.
This comment has been minimized.
Updated with the requested tests. |
This comment has been minimized.
This comment has been minimized.
I'm not even sure what the current semantics are, it may depend on whether expansion goes through pretty-printing or not, and whether nested modules are involved.
#64273 (comment) mentions a practice that can hardly be named "best", but works (including on stable, after this PR is merged). // lib.rs
include!("my_file.rs");
// my_file.rs
#[my_attr]
mod inline { /* ... */ } |
This comment was marked as resolved.
This comment was marked as resolved.
|
This comment has been minimized.
This comment has been minimized.
rfcbot
commented
Dec 5, 2019
|
This comment has been minimized.
This comment has been minimized.
Okay, I'm going to beta nominate this since there's still chance to get this into 1.40. Q: Why we should backport this to beta. Q: Why we can backport this to beta. |
This comment has been minimized.
This comment has been minimized.
If this is to get accepted into beta before branching (and I don't really want to, but do see that it could plausibly be done) it would need to happen in the next ~4 days or so before beta branches next week. It would likely be best to bring it up for acceptance at tomorrow's compiler team (planning) meeting; cc @pnkfelix @nikomatsakis |
This comment has been minimized.
This comment has been minimized.
We decided against backport in today's design meeting on the basis of the the general rule of "don't backport stabilizations without a very strong reason". |
petrochenkov commentedSep 7, 2019
While still gating non-inline modules in proc macro input.
Split from #63931
cc #54727