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

Implement `#[deprecated]` attribute (RFC 1270) #30206

Merged
merged 3 commits into from Dec 16, 2015

Conversation

Projects
None yet
8 participants
@petrochenkov
Contributor

petrochenkov commented Dec 4, 2015

Closes #29935

The attributes deprecated and rustc_deprecated are completely independent in this implementation and it leads to some noticeable code duplication. Representing deprecated as

Stability {
    level: Stable { since: "" },
    feature: "",
    depr: Some(Deprecation),
}

or, contrariwise, splitting rustc_deprecation from stability makes most of the duplication go away.
I can do this refactoring, but before doing it I must be sure, that further divergence of deprecated and rustc_deprecated is certainly not a goal.

cc @llogiq

@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Dec 4, 2015

r? @brson

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

@llogiq

This comment has been minimized.

Contributor

llogiq commented Dec 4, 2015

Great work, @petrochenkov! I think allowing the features to diverge further was the express intent of the RFC. This means we can use #[rustc_deprecation] as is and iterate on the #[deprecation] feature, e.g. adding other optional fields.

@petrochenkov

This comment has been minimized.

Contributor

petrochenkov commented Dec 4, 2015

I'd prefer the standard library to migrate to #[deprecated] eventually (after its stabilization, for example), maybe with slightly stricter checks, like now. I.e. I see #[rustc_deprecated] as a temporary measure.

@brson

This comment has been minimized.

Contributor

brson commented Dec 10, 2015

This patch looks quite thorough. Good work.

"`#[deprecated]` attribute is unstable");
fileline_help!(self.tcx.sess, attr.span(), "add #![feature(deprecated)] to \
the crate features to enable");
}

This comment has been minimized.

@brson

brson Dec 10, 2015

Contributor

This check should probably be in libsyntax/feature_gate.rs

// unmarked impls for it. See FIXME above for more details.
debug!("lookup: trait_id={:?}", trait_id);
return lookup_deprecation(tcx, trait_id);

This comment has been minimized.

@brson

brson Dec 10, 2015

Contributor

The duplication of this method strikes me as particularly bad. Can the contents be abstracted into lookup_uncached_generic?

This comment has been minimized.

@petrochenkov

petrochenkov Dec 12, 2015

Contributor

I've removed some obsolete code from these functions instead, should be better now.

@@ -577,74 +577,71 @@ impl LateLintPass for MissingDebugImplementations {
declare_lint! {
DEPRECATED,
Warn,
"detects use of #[rustc_deprecated] items"
"detects use of `#[deprecated]` or `#[rustc_deprecated]` items"

This comment has been minimized.

@brson

brson Dec 10, 2015

Contributor

The existence of #[rustc_deprecated] is an implementation detail. Can we change this string to 'detects use of deprecated items'?

// #[deprecated] can't be used in staged api
#![feature(deprecated, staged_api)]

This comment has been minimized.

@brson

brson Dec 10, 2015

Contributor

Didn't we remove the staged_api feature?

This comment has been minimized.

@petrochenkov

petrochenkov Dec 12, 2015

Contributor

We removed #[staged_api] attribute, not #[feature(staged_api)].
#[feature(staged_api)] gates stability attributes.

fn trait_deprecated(&self) {}
#[deprecated(since = "1.0.0", note = "text")]
fn trait_deprecated_text(&self) {}
}

This comment has been minimized.

@brson

brson Dec 10, 2015

Contributor

Why isn't there a test with the deprecated attribute on the trait itself?

@petrochenkov petrochenkov force-pushed the petrochenkov:newdepr branch from aebbae0 to 67a9784 Dec 12, 2015

@petrochenkov

This comment has been minimized.

Contributor

petrochenkov commented Dec 12, 2015

Updated with comments addressed.
I've also added rustdoc support for #[deprecated]. (I intended to avoid doing this initially and merge Deprecation into Stability instead.)

@brson

This comment has been minimized.

brson commented on src/librustc/middle/stability.rs in 105bd15 Dec 14, 2015

Huh, this is a surprising patch. Were these four blocks of code in these two lookup_*_uncached methods not actually doing anything useful? Where is this work done now?

This comment has been minimized.

Owner

petrochenkov replied Dec 14, 2015

All trait impls have annotations now (this work was done in rust-lang#29152 and rust-lang#29083), so there's no need to infer them from annotations on traits.
But this is not important anyway, because annotations on trait impls are not currently used during checking phase and serve only as documentation.

@brson

This comment has been minimized.

Contributor

brson commented Dec 15, 2015

@bors r+

@bors

This comment has been minimized.

Contributor

bors commented Dec 15, 2015

📌 Commit 67a9784 has been approved by brson

@bors

This comment has been minimized.

Contributor

bors commented Dec 16, 2015

⌛️ Testing commit 67a9784 with merge ac2c5ff...

bors added a commit that referenced this pull request Dec 16, 2015

Auto merge of #30206 - petrochenkov:newdepr, r=brson
Closes #29935

The attributes `deprecated` and `rustc_deprecated` are completely independent in this implementation and it leads to some noticeable code duplication. Representing `deprecated` as
```
Stability {
    level: Stable { since: "" },
    feature: "",
    depr: Some(Deprecation),
}
```
or, contrariwise, splitting rustc_deprecation from stability makes most of the duplication go away.
I can do this refactoring, but before doing it I must be sure, that further divergence of `deprecated` and `rustc_deprecated` is certainly not a goal.

cc @llogiq

@bors bors merged commit 67a9784 into rust-lang:master Dec 16, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@durka

This comment has been minimized.

Contributor

durka commented Dec 22, 2015

Is there a reason (heh) that this was implemented with #[deprecated(note = "...")] instead of #[deprecated(reason = "...")] as in the RFC? Should we amend the RFC to match (the rename was mentioned as an alternative but not in the main text)?

@petrochenkov

This comment has been minimized.

Contributor

petrochenkov commented Dec 22, 2015

@durka

Is there a reason (heh) that this was implemented with #[deprecated(note = "...")] instead of #[deprecated(reason = "...")] as in the RFC?

Yes, by request from the RFC author, also see rust-lang/rfcs#1270 (comment)
RFC should really be amended since it's the central piece of documentation about this feature at the moment.

@liigo

This comment has been minimized.

Contributor

liigo commented Dec 23, 2015

I'm surprised. Implementation violate the RFC (note VS reason) intentionally without any reason, and surprised that was merged without any query. I don't oppose note (I like it), I just obey the RFC.

(This is not the first time i noticed that RFC was violated by intention. Please respect RFC! Please amend RFC first if you think there is an error.)

@llogiq

This comment has been minimized.

Contributor

llogiq commented Dec 23, 2015

I'm a bit busy ATM but will push a PR to change the RFC shortly to alleviate the situation.

@llogiq

This comment has been minimized.

Contributor

llogiq commented Dec 23, 2015

@durka

This comment has been minimized.

Contributor

durka commented Dec 23, 2015

Oops, I had already submitted the same PR at rust-lang/rfcs#1423. @liigo your language is a bit stronger than necessary since this decision was essentially made in the RFC thread as it was being accepted, but just didn't make it into the text on time.

@liigo

This comment has been minimized.

Contributor

liigo commented Dec 24, 2015

since this decision was essentially made in the RFC thread

I don't think so. Only three people says "note seems fine", no consensus, no confirmed by the libs (or lang) team.

When RFC 1270 was merging, Alex said:

we can always tweak minor details like naming here and there after this is merged

That is, note was not accepted yet at that time.

@llogiq

This comment has been minimized.

Contributor

llogiq commented Dec 24, 2015

It isn't unusual to tweak details in the implementation after an RFC is accepted. Please forgive my cavalier attitude, but I don't think changing a field name on an unstable attribute requires the whole teams' consenus.

@petrochenkov

This comment has been minimized.

Contributor

petrochenkov commented Dec 24, 2015

I've just noticed, this is not marked with "relnotes", but probably should be.

@bluss bluss added the relnotes label Jan 11, 2016

@bluss

This comment has been minimized.

Contributor

bluss commented Jan 11, 2016

Is there a tracking issue for stabilization?

@petrochenkov petrochenkov deleted the petrochenkov:newdepr branch Sep 21, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment