check stability of macro invocations#48524
Conversation
99f2951 to
504bacf
Compare
| // feature-gate the macro invocation | ||
| if let Some((feature, issue)) = unstable_feature { | ||
| // only if the outer span doesn't allow unstable invocations | ||
| // TODO: compare crates of span and def_site_span (can't figure out how) |
There was a problem hiding this comment.
We only need to check stability for external crates but I can't figure out how to get the crate ID of a span or macro def in this context.
|
@kennytm @petrochenkov Test implemented, need help on one bit. |
|
I've made an attempt at the crate thing, based on spans. It seems hacky but I dunno. |
| @@ -0,0 +1,16 @@ | |||
| // Copyright 2018 The Rust Project Developers. See the COPYRIGHT | |||
There was a problem hiding this comment.
compile-fail (instead of compile-fail-fulldeps) would be enough since we don't use procedural macros here.
|
|
||
| let ident = ident.unwrap_or_else(|| keywords::Invalid.ident()); | ||
| let validate_and_set_expn_info = |def_site_span, | ||
| let validate_and_set_expn_info = |self_: &mut Self, // arg instead of capture |
There was a problem hiding this comment.
Mega-Nit: this is conventionally used for this in rustc.
| // macro features will count as lib features | ||
| !feats.declared_lib_features.iter().any(|&(feat, _)| feat == feature) | ||
| }) { | ||
| let explain = format!("macro {}! is unstable", path); |
There was a problem hiding this comment.
If you are checking stability, then you can check deprecation as well - these two properties are normally checked together.
IIRC, there was even a separate issue about deprecation attributes being ignored on macros.
There was a problem hiding this comment.
That's doable, except I don't know how to get the crate's #[warn/allow/deny(deprecation)] flag in this context.
There was a problem hiding this comment.
I guess we have to separately find and store that value as we walk the AST while expanding macros. It's doable if we want this feature.
There was a problem hiding this comment.
Is this not already tracked somewhere in the AST, like for the dead code lints? Or is macro expansion too early for that?
There was a problem hiding this comment.
Deprecation tracking is done in phase 3 at the same time as stability tracking. I'm having to more-or-less naively reimplement the latter here (unless we want to do what @petrochenkov suggested and refactor stability checking entirely into phase 2) so deprecation tracking is going to have to be reimplemented as well.
There was a problem hiding this comment.
Ok, if there are complications then let's leave this for later.
|
|
||
| let opt_expanded = match *ext { | ||
| DeclMacro(ref expand, def_span) => { | ||
| if let Err(msg) = validate_and_set_expn_info(def_span.map(|(_, s)| s), |
There was a problem hiding this comment.
Could you add a test for macros 2.0 (macro m { ... }) as well?
There was a problem hiding this comment.
Wait, I see, the stability information is not filled for macros 2.0.
There was a problem hiding this comment.
It can be if we want it to.
There was a problem hiding this comment.
We certainly want it, stability of macro items not being checked is a bug in the same way as stability of macro_rules not being checked.
| if let Some((feature, issue)) = unstable_feature { | ||
| let crate_span = self_.cx.current_expansion.crate_span.unwrap(); | ||
| // don't stability-check macros in the same crate | ||
| if !crate_span.contains(def_site_span) |
There was a problem hiding this comment.
I don't know how this is going to behave in corner cases (e.g. macros defined by macros), but this seems to be the conservative solution (if one span contains another, they are in one crate) and I'll r+ this if nobody comes up with a better solution until in few days.
There was a problem hiding this comment.
I was going to add the macro-generating-macro case to the test as well.
|
Some legitimate compilation errors from Travis. |
That seems like a lot of code duplication. |
|
This will need a crater run as well since it's a breaking change (unless we want to warn for a release cycle and then make it a hard error). |
|
Should do a crater run either way.
…On Tue, Feb 27, 2018 at 5:45 PM, Austin Bonander ***@***.***> wrote:
This will need a crater run as well since it's a breaking change (unless
we want to warn for a release cycle and then make it a hard error).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#48524 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAC3n54X4eIIIZcSesKv-IsAyvZaS9-Jks5tZIWQgaJpZM4SSJSD>
.
|
Nah, privacy and partially stability already work this way because they follow name resolution. When we resolve some name to its definition (this can happen during different compilation stages because some resolution is purely name based and some is type based) we can immediately check that definition for privacy and stability. |
Why? I'd be happy to run crater on many PRs "just in case" if it took 1-2 days like before, but now it takes eternity. |
|
☀️ Test successful - status-travis |
|
Leaving the review to @petrochenkov. AIUI the point of a crater run and warning cycle is for macros in the std library which are unstable, but used by clients because of this loophole? If that is correct then it seems that there is a legitimate breaking change here if there are any unstable macros in the standard library (e.g., |
52c52bd to
69035f2
Compare
|
@petrochenkov squashed |
|
Crater run started. Expect results in ~5 days. |
|
Crater results: http://cargobomb-reports.s3.amazonaws.com/pr-48524/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. |
|
They seem to be all spurious failure or syntax extensions (this is
libsyntax-breaking).
…On Wed, Mar 14, 2018 at 8:32 PM, aidanhs ***@***.***> wrote:
Crater results: http://cargobomb-reports.s3.amazonaws.com/pr-48524/index.
html. 'Blacklisted' crates (spurious failures etc) can be found here
<https://github.com/rust-lang-nursery/crater/blob/master/blacklist.md>.
If you see any spurious failures not on the list, please make a PR against
that file.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#48524 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAC3nw3Rtoiu4dPDdSsWSFvfIGD64nIeks5teba4gaJpZM4SSJSD>
.
|
|
Excellent. |
|
📌 Commit 69035f2 has been approved by |
|
⌛ Testing commit 69035f2 with merge f6bcd1b7f6ebf62501d533b79353787e6916e28c... |
|
💔 Test failed - status-appveyor |
|
this appears to be the error causing the failure: https://ci.appveyor.com/project/rust-lang/rust/build/1.0.6683/job/bcrb0bd5rwa26uvr#L14450 |
|
@bors retry 3 hour timeout (38 minutes in stage1-rustc) |
check stability of macro invocations I haven't implemented tests yet but this should be a pretty solid prototype. I think as-implemented it will also stability-check macro invocations in the same crate, dunno if we want that or not. I don't know if we want this to go through `rustc::middle::stability` or not, considering the information there wouldn't be available at the time of macro expansion (even for external crates, right?). r? @nrc closes #34079 cc @petrochenkov @durka @jseyfried #38356
|
☀️ Test successful - status-appveyor, status-travis |
I haven't implemented tests yet but this should be a pretty solid prototype. I think as-implemented it will also stability-check macro invocations in the same crate, dunno if we want that or not.
I don't know if we want this to go through
rustc::middle::stabilityor not, considering the information there wouldn't be available at the time of macro expansion (even for external crates, right?).r? @nrc
closes #34079
cc @petrochenkov @durka @jseyfried #38356