-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Macro expansion bypasses #[derive_*] stability checks. #32655
Comments
cc #29644 |
there are different possible outcomes for what we do here, depending on how hard it is to come up with a fix (@eddyb may already have one in the works), and what the impact of such a fix is according to crater. |
(assigning P-high to reflect the priority of deciding what to do.) |
Attempt at a fix and crater results can be found at #32671. In IRC @durka and I looked at the crates that depend on And that doesn't count anyone who has a Some food for thought: @nrc's macro plans could eventually allow for It's not the worst that could happen, and I haven't seen specific plans for making Note that you can't use That said, I leave this to @rust-lang/lang to decide. Do we just upload a new version of serde and/or do we ungate |
Chiming in, as primary
Please contact me whenever about this - I believe Dropbox is using some subset of I suspect there's a nice way to deal with this (at least in terms of |
@mrmonday You could have pinned just the minor version, not the patch, i.e. |
Since the fix for this likely breaks syntex, cc #31645. Please don't merge the fix without letting me know first. |
I'm just on my way out, but as long as we can fix syntex first, I think it'd be fine to make this change in the compiler. The syntex consumers are pretty familiar with breaking changes in order to track stable and nightly plugin support, so doing a version bump to avoid this error wouldn't be that bad. How about we fix this in syntex, do a serde version bump for both v6 and v7, then see how things look in crater? |
@erickt One way that would work pretty well IMO is to replace all |
This avoids rust-lang/rust#32655. # Conflicts: # syntex/Cargo.toml
I actually don't see any crates pinned to 0.29.0 or 0.30.0, did I miss some? I see a bunch of |
@durka: Not sure. I just saw that there were still some downloads for |
Let's keep trying to land the fix, it sounds like it should be possible. I really don't want to stabilise |
I've changed libpnet to depend on |
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.
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.
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.
(hmm it seems like this was fixed, but I'm not clear what PR actually fixed it. In particular, PR #32671 never landed...) |
well bisection indicates it was fixed (in the sense that we started issuing the lint even when the custom attribute was hidden) between nightly-2016-04-28 (cda7c1c 2016-04-27) and nightly-2016-05-07 (62e2b2f 2016-05-06). rustup says we don't have prebuilt distributions for the dates between those two points. ... oh, maybe I should try looking at the slew of commits from LeoTestard referencing this issue: PR #32791 is what resolved this. |
One can use macro expansion to bypass all early attribute checks, directly:
Or by using the built-in
include!
macro.Found while investigating
libpnet
breakage.This affects any use of
#[derive(...)]
insyntex
, as virtually everyone usesinclude!
.Even though I don't like stabilizing this, there may be too many cases in the wild.
The text was updated successfully, but these errors were encountered: