-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Deprecate no_debug and custom_derive #37128
Conversation
☔ The latest upstream changes (presumably #37118) made this pull request unmergeable. Please resolve the merge conflicts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good; left a few suggestions for improvement.
..) = g { | ||
cx.span_lint(DEPRECATED, | ||
attr.span, | ||
&format!("use of deprecated attribute: {}", name)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: probably we want:
"use of deprecated attribute: `{}`"
// ^ ^ note ticks
right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, let's link the deprecation to a tracking issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This follows the format for deprecated items. That probably should have back ticks too (added), but we don't have the tracking issue number anywhere so we can't link to it very easily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well for deprecated items we give a reason, right? I imagined adding the issue number (or maybe just a string) to the Deprecated
variant of Stability
, in any case.
let name = &*attr.name(); | ||
for &(n, _, ref g) in feature_gate::KNOWN_ATTRIBUTES { | ||
if n == name { | ||
if let &feature_gate::AttributeGate::Gated(feature_gate::Stability::Deprecated, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: this seems mega-inefficient...but probably it doesn't matter as there aren't that many attributes, and that table ain't that long, I guess?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At minimum we could filter out the list of deprecations once rather than per attribute =)
// fn() is not Debug | ||
impl ::std::fmt::Debug for AttributeGate { | ||
fn fmt(&self, fmt: &mut ::std::fmt::Formatter) -> ::std::fmt::Result { | ||
match *self { | ||
Gated(ref name, ref expl, _) => write!(fmt, "Gated({}, {})", name, expl), | ||
Gated(_, ref name, ref expl, _) => write!(fmt, "Gated({}, {})", name, expl), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why exclude Stability
from the Debug
printout?
@nikomatsakis changes made |
More changes for review |
☔ The latest upstream changes (presumably #37269) made this pull request unmergeable. Please resolve the merge conflicts. |
rebased |
tidy errors:
|
r=me once tidy is fixed; thanks |
@bors: r=nikomatsakis |
📌 Commit f6e0b3a has been approved by |
⌛ Testing commit f6e0b3a with merge 5694dd1... |
💔 Test failed - auto-win-gnu-32-opt-rustbuild |
|
📌 Commit 38f993f has been approved by |
Deprecate no_debug and custom_derive r? @nikomatsakis
💔 Test failed - auto-win-gnu-32-opt-rustbuild |
Ok that failure corresponds to a failure to free on Windows. That's terrifying! @bors: retry |
⌛ Testing commit 38f993f with merge 3c4de7f... |
💔 Test failed - auto-mac-64-opt |
@bors: retry |
I'm inclined not to backport at this point. Not because it's harmful per se but just because we can just push deprecation back one cycle, so why not? What do you think @nrc? |
@nikomatsakis yeah, it is getting kind of late into the cycle for a backport. |
Removed beta-nominated tag: decided against backport. |
⌛ Testing commit 38f993f with merge 918bc3a... |
💔 Test failed - auto-linux-64-opt |
Hm that's two segfaults at the same location on two different platforms, maybe a legitimate segfault? |
Dammit, Rust is not meant to segfault |
Has a custom deprecation since deprecating features is not supported and is a pain to implement
📌 Commit 8c4a39c has been approved by |
Deprecate no_debug and custom_derive r? @nikomatsakis
FTR r was nikomatsakis, not alexcrichton |
I'd like to request that this deprecation be reverted, and I plan on submitting a PR to do so. This deprecation penalizes developers that write multiple types of syntax extensions instead of only custom derives, and it seems unnecessary while macros 2.0 are still some time away. At present, it is possible to have a single #![feature(plugin, custom_derive)]
#![plugin(codegen_crate)] With this deprecation, however, this is no longer possible. The alternative, of course, is to use macros 1.1. But custom derives via macros 1.1 must be implemented in a separate crate of type #![feature(plugin)]
#![plugin(codegen_crate)]
#[macro_use] extern crate derive_crate; I believe this will lead to user confusion: syntax extensions are now coming from two different crates, and they must be imported via completely different mechanisms. While before a single declaration brought in all syntax extensions, now two different declarations are used to bring in syntax extensions. This is also significantly more unergonomic for the developer. This is because it is no longer feasible to easily share code between custom derives and other syntax extensions. The work-arounds are to have one create depend on the other, pushing logic to one "master" crate, or to create a third crate that contains the shared logic. Clearly, neither are as ergonomic as the existing implementation. All of this issues will likely be ameliorated by the unification macros 2.0 promises to bring. But macros 2.0 won't land in a nightly release any time soon, and they won't be stabilized until quite some time after that. So why deprecate this unnecessarily early? It penalizes developers that write multiple types of syntax extensions. We should keep this around until macros 2.0 are within reach of stabilization. |
r? @nikomatsakis