Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

check stability of macro invocations #48524

Merged
merged 1 commit into from
Mar 16, 2018

Conversation

abonander
Copy link
Contributor

@abonander abonander commented Feb 25, 2018

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 25, 2018
@abonander abonander force-pushed the check-macro-stability branch 2 times, most recently from 99f2951 to 504bacf Compare February 25, 2018 06:14
@kennytm kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 25, 2018
@petrochenkov petrochenkov self-assigned this Feb 25, 2018
@abonander abonander changed the title [needs test] check stability of macro invocations check stability of macro invocations Feb 25, 2018
// 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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@abonander
Copy link
Contributor Author

@kennytm @petrochenkov Test implemented, need help on one bit.

@kennytm kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 25, 2018
@abonander
Copy link
Contributor Author

I've made an attempt at the crate thing, based on spans. It seems hacky but I dunno.

@petrochenkov
Copy link
Contributor

I don't know if we want this to go through rustc::middle::stability or not

Rather the opposite, we need to move all stability checking into resolution/expansion (well, except for all the type-based stuff like methods or fields) to fix long-standing issues like #15702 and #23937.

@@ -0,0 +1,16 @@
// Copyright 2018 The Rust Project Developers. See the COPYRIGHT
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

compile-fail (instead of compile-fail-fulldeps) would be enough since we don't use procedural macros here.

@@ -531,11 +532,35 @@ impl<'a, 'b> MacroExpander<'a, 'b> {
let path = &mac.node.path;

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's doable, except I don't know how to get the crate's #[warn/allow/deny(deprecation)] flag in this context.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this not already tracked somewhere in the AST, like for the dead code lints? Or is macro expansion too early for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, if there are complications then let's leave this for later.

@@ -551,11 +576,9 @@ impl<'a, 'b> MacroExpander<'a, 'b> {

let opt_expanded = match *ext {
DeclMacro(ref expand, def_span) => {
if let Err(msg) = validate_and_set_expn_info(def_span.map(|(_, s)| s),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a test for macros 2.0 (macro m { ... }) as well?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, I see, the stability information is not filled for macros 2.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can be if we want it to.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going to add the macro-generating-macro case to the test as well.

@petrochenkov
Copy link
Contributor

Some legitimate compilation errors from Travis.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 26, 2018
@abonander
Copy link
Contributor Author

move all stability checking into resolution/expansion (well, except for all the type-based stuff like methods or fields)

That seems like a lot of code duplication.

@petrochenkov petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 27, 2018
@abonander
Copy link
Contributor Author

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).

@durka
Copy link
Contributor

durka commented Feb 27, 2018 via email

@petrochenkov
Copy link
Contributor

@abonander

That seems like a lot of code duplication.

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.

@petrochenkov
Copy link
Contributor

This will need a crater run

Why?
Library team was aware that macro stability is unchecked and always added new macros as insta-stable.
Only really unstable __implementation_detail! macros are marked as unstable, they should not be used by third parties and we can break them.

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.

@bors
Copy link
Contributor

bors commented Mar 1, 2018

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

@nrc nrc removed their assignment Mar 4, 2018
@nrc
Copy link
Member

nrc commented Mar 4, 2018

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., __thread_local_inner! from the issue). If that is the case then I think we probably need to go through the whole breaking change process here and have a crater run and a warning cycle.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@abonander abonander force-pushed the check-macro-stability branch from 52c52bd to 69035f2 Compare March 8, 2018 00:53
@abonander
Copy link
Contributor Author

@petrochenkov squashed

@Mark-Simulacrum
Copy link
Member

Crater run started. Expect results in ~5 days.

@aidanhs
Copy link
Member

aidanhs commented Mar 15, 2018

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.

@durka
Copy link
Contributor

durka commented Mar 15, 2018 via email

@kennytm kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Mar 15, 2018
@petrochenkov
Copy link
Contributor

Excellent.
@bors r+

@bors
Copy link
Contributor

bors commented Mar 15, 2018

📌 Commit 69035f2 has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 15, 2018
@bors
Copy link
Contributor

bors commented Mar 15, 2018

⌛ Testing commit 69035f2 with merge f6bcd1b7f6ebf62501d533b79353787e6916e28c...

@bors
Copy link
Contributor

bors commented Mar 16, 2018

💔 Test failed - status-appveyor

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 16, 2018
@abonander
Copy link
Contributor Author

this appears to be the error causing the failure: https://ci.appveyor.com/project/rust-lang/rust/build/1.0.6683/job/bcrb0bd5rwa26uvr#L14450

@kennytm
Copy link
Member

kennytm commented Mar 16, 2018

@bors retry

3 hour timeout (38 minutes in stage1-rustc)

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 16, 2018
@bors
Copy link
Contributor

bors commented Mar 16, 2018

⌛ Testing commit 69035f2 with merge a7170b0...

bors added a commit that referenced this pull request Mar 16, 2018
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
@bors
Copy link
Contributor

bors commented Mar 16, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: petrochenkov
Pushing a7170b0 to master...

@bors bors merged commit 69035f2 into rust-lang:master Mar 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

able to invoke unstable macro 2.0
9 participants