Feature gate clean #32791

LeoTestard · 2016-04-07T09:14:28Z

This PR does a bit of cleaning in the feature-gate-handling code of libsyntax. It also fixes two bugs (#32782 and #32648). Changes include:

Change the way the existing features are declared in feature_gate.rs. The array of features and the Features struct are now defined together by a single macro. featureck.py has been updated accordingly. Note: there are now three different arrays for active, removed and accepted features instead of a single one with a Status item to tell wether a feature is active, removed, or accepted. This is mainly due to the way I implemented my macro in the first time and I can switch back to a single array if needed. But an advantage of the way it is now is that when an active feature is used, the parser only searches through the list of active features. It goes through the other arrays only if the feature is not found. I like to think that error checking (in this case, checking that an used feature is active) does not slow down compilation of valid code. :) But this is not very important...
Feature-gate checking pass now use the Features structure instead of looking through a string vector. This should speed them up a bit. The construction of the Features struct should be faster too since it is build directly when parsing features instead of calling has_feature dozens of times.
The MacroVisitor pass has been removed, it was mostly useless since the #[cfg]-stripping phase happens before (fixes Invocation of feature-gated macros inside #[cfg]-guarded code is not checked. #32648). The features that must actually be checked before expansion are now checked at the time they are used. This also allows us to check attributes that are generated by macro expansion and not visible to MacroVisitor, but are also removed by macro expansion and thus not visible to PostExpansionVisitor either. This fixes Attributes on macro-generated macro invocations are not feature-gate-checked #32782. Note that in order for #[derive_*] to be feature-gated but still accepted when generated by #[derive(Trait)], I had to do a little bit of trickery with spans that I'm not totally confident into. Please review that part carefully. (It's in libsyntax_ext/deriving/mod.rs.)::

Note: this is a [breaking change], since programs with feature-gated attributes on macro-generated macro invocations were not rejected before. For example:

macro_rules! bar (
    () => ()
);

macro_rules! foo (
    () => (
        #[allow_internal_unstable] //~ ERROR allow_internal_unstable side-steps
        bar!();
    );
);

foo!();

rust-highfive · 2016-04-07T09:14:41Z

r? @nrc

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

eddyb · 2016-04-07T09:29:02Z

cc #32655 and #32671

LeoTestard · 2016-04-07T09:36:16Z

@eddyb: Ha, I see we used the same trick for spans for #[derive_*]. :) This reassures me a bit. What do you think we should do? Do you want to merge your own PR first and me to rebase mine? Or should we just merge that one?

eddyb · 2016-04-07T09:39:17Z

@LeoTestard AFAICT syntex was fixed not to generate code that trips over such a check, but @nikomatsakis would still like a warning for a release cycle (old syntex output might still be around).

LeoTestard · 2016-04-07T09:43:23Z

You mean (well, he means) generate a warning instead of an error? That sounds tricky with my approach since the check in libsyntax_ext/deriving/mod.rs now not only catches uses inside macros but also all other uses. I have no idea how to check if a use comes from a macro or not. Suggesting to run cargo update when the error comes from syntex sounds feasible though.

eddyb · 2016-04-07T09:45:09Z

@LeoTestard the error is not "from syntex", but rather you get a file that syntex pretty-printed. I can't think of a way to distinguish that, since the actual code could be anything the user put through syntex.

LeoTestard · 2016-04-07T17:38:04Z

Ok now it should only warn for #[derive_*] for the cases that did not error before. It's tricky and based on whether the parent of the top of the expansion backtrace points to a valid ExpnInfo and is not NO_EXPANSION. It seems to work for the few interesting cases I've tested.

Note that this PR is still a breaking change for all the other uses of feature-gated attributes that were not checked in macro expansion (see #32782) and that are now checked and just reported as errors.

r?

bors · 2016-04-07T22:41:08Z

☔ The latest upstream changes (presumably #32016) made this pull request unmergeable. Please resolve the merge conflicts.

LeoTestard · 2016-04-08T09:19:28Z

Rebased onto master. r?
(cc @nikomatsakis @pnkfelix)

jseyfried · 2016-04-09T01:52:33Z

@LeoTestard I believe this PR is only feature gate checking attributes of macros in item positions.

For example, the following would be erroneously allowed (it is an currently error):

macro_rules! method {
    () => { fn f() {} }
}

struct S;
impl S {
    #[allow_internal_unstable]
    method! {}
}

jseyfried · 2016-04-09T04:10:05Z

src/libsyntax_ext/deriving/mod.rs

@@ -96,6 +96,36 @@ fn expand_derive(cx: &mut ExtCtxt,
            let mut found_partial_eq = false;
            let mut found_eq = false;

+            // This span is **very** sensitive and crucial to
+            // getting the stability behavior we want. What we are
+            // doing is marking `#[structural_match]` with the


If I understand correctly, we could also be marking #[derive_*] with this span -- the comment should probably reflect that.

jseyfried · 2016-04-09T05:48:41Z

Reviewed, LGTM modulo comments.

LeoTestard · 2016-04-09T16:02:40Z

@jseyfried Ok, I fixed what you said. I added another commit for removing useless code since it's not directly related. Let me know if you want me to squash it/rebase it/whatever.

jseyfried · 2016-04-09T21:28:00Z

src/libsyntax/ext/expand.rs

+            if let Some(&Features { pushpop_unsafe: false, .. }) =
+                fld.cx.ecfg.features {
+                feature_gate::emit_feature_err(
+                    &fld.cx.parse_sess.span_diagnostic, "pushpop_unsafe", expr_span,


This should be "placement_in_syntax" (Travis is failing because of this).

Yeah I saw that but I didn't have the time to fix it earlier... I guess I copy-pasted too fast. Sorry about that.

jseyfried · 2016-04-09T21:39:56Z

src/libsyntax/ext/expand.rs

+                                      &fld.cx.parse_sess.codemap(),
+                                      &fld.cx.ecfg.features.unwrap());
+    }
+


I believe we still need to stability check macros in expression and statement positions (search for drop(attrs) in this file).
I would refactor this out into a new function in expand.rs (something like check_attributes(attr: &ast::Attribute, fld: &mut MacroExpander)) and then inline and remove feature_gate::check_attribute.

Are you sure we should inline? feature_gate::Context is private for the moment...

Good point -- not inlining is ok

LeoTestard · 2016-04-21T12:22:55Z

Done. r?

nikomatsakis · 2016-04-21T20:39:01Z

@LeoTestard it seems like tidy doesn't build on this branch?

LeoTestard · 2016-04-21T21:29:05Z

@nikomatsakis Huh, yeah, my bad, I thought I tested this but that was before I had to rebase it against the complete rewriting of tidy checks I guess. u__u

LeoTestard · 2016-04-21T23:38:26Z

@nikomatsakis Fixed, I think.

This pass was supposed to check use of gated features before `#[cfg]`-stripping but this was not the case since it in fact happens after. Checks that are actually important and must be done before macro expansion are now made where the features are actually used. Close rust-lang#32648. Also ensure that attributes on macro-generated macro invocations are checked as well. Close rust-lang#32782 and rust-lang#32655.

nikomatsakis · 2016-04-28T00:04:31Z

r? @nikomatsakis

nikomatsakis · 2016-04-28T00:05:56Z

@bors r+

bors · 2016-04-28T00:05:57Z

📌 Commit fa23d10 has been approved by nikomatsakis

bors · 2016-04-28T01:35:30Z

⌛ Testing commit fa23d10 with merge 435095f...

Feature gate clean This PR does a bit of cleaning in the feature-gate-handling code of libsyntax. It also fixes two bugs (#32782 and #32648). Changes include: * Change the way the existing features are declared in `feature_gate.rs`. The array of features and the `Features` struct are now defined together by a single macro. `featureck.py` has been updated accordingly. Note: there are now three different arrays for active, removed and accepted features instead of a single one with a `Status` item to tell wether a feature is active, removed, or accepted. This is mainly due to the way I implemented my macro in the first time and I can switch back to a single array if needed. But an advantage of the way it is now is that when an active feature is used, the parser only searches through the list of active features. It goes through the other arrays only if the feature is not found. I like to think that error checking (in this case, checking that an used feature is active) does not slow down compilation of valid code. :) But this is not very important... * Feature-gate checking pass now use the `Features` structure instead of looking through a string vector. This should speed them up a bit. The construction of the `Features` struct should be faster too since it is build directly when parsing features instead of calling `has_feature` dozens of times. * The MacroVisitor pass has been removed, it was mostly useless since the `#[cfg]-stripping` phase happens before (fixes #32648). The features that must actually be checked before expansion are now checked at the time they are used. This also allows us to check attributes that are generated by macro expansion and not visible to MacroVisitor, but are also removed by macro expansion and thus not visible to PostExpansionVisitor either. This fixes #32782. Note that in order for `#[derive_*]` to be feature-gated but still accepted when generated by `#[derive(Trait)]`, I had to do a little bit of trickery with spans that I'm not totally confident into. Please review that part carefully. (It's in `libsyntax_ext/deriving/mod.rs`.):: Note: this is a [breaking change], since programs with feature-gated attributes on macro-generated macro invocations were not rejected before. For example: ```rust macro_rules! bar ( () => () ); macro_rules! foo ( () => ( #[allow_internal_unstable] //~ ERROR allow_internal_unstable side-steps bar!(); ); ); ``` foo!();

bors · 2016-04-28T04:52:42Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-32cross-opt, auto-linux-64-cargotest, auto-linux-64-cross-armhf, auto-linux-64-cross-armsf, auto-linux-64-cross-freebsd, auto-linux-64-cross-netbsd, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-opt-mir, auto-linux-64-opt-rustbuild, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-mac-64-opt-rustbuild, auto-mac-cross-ios-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-32-opt-rustbuild, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-cross-opt, auto-win-msvc-32-opt, auto-win-msvc-64-cargotest, auto-win-msvc-64-opt, auto-win-msvc-64-opt-mir, auto-win-msvc-64-opt-rustbuild

rust-highfive assigned nrc Apr 7, 2016

LeoTestard force-pushed the feature-gate-clean branch from fc6a691 to 0629913 Compare April 7, 2016 09:16

LeoTestard force-pushed the feature-gate-clean branch from 0629913 to 71c4e6f Compare April 7, 2016 17:30

LeoTestard force-pushed the feature-gate-clean branch from 71c4e6f to 5074c7c Compare April 7, 2016 20:22

LeoTestard force-pushed the feature-gate-clean branch 2 times, most recently from 7493c3c to f654c8c Compare April 7, 2016 23:28

LeoTestard mentioned this pull request Apr 8, 2016

Avoid gated feature checking unconfigured expanded items #32846

Merged

jseyfried reviewed Apr 9, 2016
View reviewed changes

LeoTestard force-pushed the feature-gate-clean branch 2 times, most recently from 6151172 to aa36987 Compare April 9, 2016 16:01

jseyfried reviewed Apr 9, 2016
View reviewed changes

LeoTestard force-pushed the feature-gate-clean branch 2 times, most recently from 2650886 to cb29b20 Compare April 9, 2016 21:38

jseyfried reviewed Apr 9, 2016
View reviewed changes

LeoTestard force-pushed the feature-gate-clean branch from fa121f4 to 21bf716 Compare April 21, 2016 23:37

LeoTestard force-pushed the feature-gate-clean branch from 21bf716 to fa23d10 Compare April 21, 2016 23:39

LeoTestard added 3 commits April 22, 2016 01:40

Fix tidy for the new syntax of feature declarations in libsyntax.

11f1eb0

Remove some useless code.

fa23d10

rust-highfive assigned nikomatsakis and unassigned nrc Apr 28, 2016

bors merged commit fa23d10 into rust-lang:master Apr 28, 2016

This was referenced Apr 28, 2016

Implement constant support in MIR. #33130

Merged

rustc: Implement custom panic runtimes #32900

Merged

Feature-gate #[derive_*] even when produced by macro expansion. #32671

Closed

bluss added the relnotes Marks issues that should be documented in the release notes of the next release. label Apr 28, 2016

jseyfried mentioned this pull request May 28, 2016

Fix the span of generated #[derive_*] attributes #33926

Merged

nikomatsakis mentioned this pull request Nov 2, 2016

Use impl obligations as initial environment for specialization #37541

Merged

nikomatsakis added a commit to nikomatsakis/rust that referenced this pull request Nov 2, 2016

add rust-lang#32791 test case

797e042

brson pushed a commit to brson/rust that referenced this pull request Nov 3, 2016

add rust-lang#32791 test case

416580e

pnkfelix mentioned this pull request Jul 10, 2019

Macro expansion bypasses #[derive_*] stability checks. #32655

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature gate clean #32791

Feature gate clean #32791

LeoTestard commented Apr 7, 2016

rust-highfive commented Apr 7, 2016

eddyb commented Apr 7, 2016

LeoTestard commented Apr 7, 2016

eddyb commented Apr 7, 2016

LeoTestard commented Apr 7, 2016

eddyb commented Apr 7, 2016

LeoTestard commented Apr 7, 2016

bors commented Apr 7, 2016

LeoTestard commented Apr 8, 2016

jseyfried commented Apr 9, 2016

jseyfried Apr 9, 2016

jseyfried commented Apr 9, 2016

LeoTestard commented Apr 9, 2016

jseyfried Apr 9, 2016

LeoTestard Apr 9, 2016

jseyfried Apr 9, 2016

jseyfried Apr 9, 2016

LeoTestard Apr 9, 2016

jseyfried Apr 9, 2016

LeoTestard commented Apr 21, 2016

nikomatsakis commented Apr 21, 2016

LeoTestard commented Apr 21, 2016

LeoTestard commented Apr 21, 2016

nikomatsakis commented Apr 28, 2016

nikomatsakis commented Apr 28, 2016

bors commented Apr 28, 2016

bors commented Apr 28, 2016

bors commented Apr 28, 2016

Feature gate clean #32791

Feature gate clean #32791

Conversation

LeoTestard commented Apr 7, 2016

rust-highfive commented Apr 7, 2016

eddyb commented Apr 7, 2016

LeoTestard commented Apr 7, 2016

eddyb commented Apr 7, 2016

LeoTestard commented Apr 7, 2016

eddyb commented Apr 7, 2016

LeoTestard commented Apr 7, 2016

bors commented Apr 7, 2016

LeoTestard commented Apr 8, 2016

jseyfried commented Apr 9, 2016

jseyfried Apr 9, 2016

Choose a reason for hiding this comment

jseyfried commented Apr 9, 2016

LeoTestard commented Apr 9, 2016

jseyfried Apr 9, 2016

Choose a reason for hiding this comment

LeoTestard Apr 9, 2016

Choose a reason for hiding this comment

jseyfried Apr 9, 2016

Choose a reason for hiding this comment

jseyfried Apr 9, 2016

Choose a reason for hiding this comment

LeoTestard Apr 9, 2016

Choose a reason for hiding this comment

jseyfried Apr 9, 2016

Choose a reason for hiding this comment

LeoTestard commented Apr 21, 2016

nikomatsakis commented Apr 21, 2016

LeoTestard commented Apr 21, 2016

LeoTestard commented Apr 21, 2016

nikomatsakis commented Apr 28, 2016

nikomatsakis commented Apr 28, 2016

bors commented Apr 28, 2016

bors commented Apr 28, 2016

bors commented Apr 28, 2016