Skip to content

WIP inherited deprecation fixes #26040 #26061

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

Merged
merged 3 commits into from
Jun 23, 2015
Merged

Conversation

Gankra
Copy link
Contributor

@Gankra Gankra commented Jun 7, 2015

Uncertain if this is the desired effect/strategy/testing.

r? @aturon

@Gankra
Copy link
Contributor Author

Gankra commented Jun 7, 2015

cc @brson if you have an opinion

@brson
Copy link
Contributor

brson commented Jun 8, 2015

Patch looks ok to me. I'm curious what impacts this had on the deprecatedness of std. Did anything new get deprecated?

@brson
Copy link
Contributor

brson commented Jun 8, 2015

Oh, there's a weird case in this patch: since it applies the parent deprecation tag, it could potentially apply the deprecation tag from one feature to a different feature, which is pretty bogus. It could possibly also apply a deprecated tag to something without an explicit stability level at all, which is illegal.

It's not likely to happen in sane code, but it could. You might add more conditions that the parent has the same stability level and feature.

@Gankra
Copy link
Contributor Author

Gankra commented Jun 8, 2015

Yeah I wasn't sure how "no stability" was supposed to interact here. I'm not sure what you mean about one feature getting the deprecation of another. It seems like if I deprecate a whole module, anything in there should inherit that, regardless of what features its under?

Would you be able to provide some test cases/examples of what you're imagining?

@brson
Copy link
Contributor

brson commented Jun 8, 2015

The concern about accidentally deprecating multiple features isn't as important as I was thinking, since features can be partially deprecated, and deprecations can happen during any version.

Here's one example where inheritance crosses features and could cause incorrect deprecation.

pub reexport a::f;

#[unstable(feature = "foo", ...)]
pub mod a {
    #[stable(feature = "bar", ...)]
    pub fn f() { }
}

With deprecation inheritance, if a is deprecated, the function f in the other feature 'bar' is also deprecated.

There's a current rule that says that anything marked deprecated, must also be explicitly marked unstable or stable. This is to prevent unstable+deprecated things accidentally making their way into stable releases by becoming stable+deprecated. With stable not inheriting though the only error I can think of making would be accidentally tagging something that has inherited deprecation with stable.

Imagine:

pub reexport a::f;

#[unstable(feature = "foo", ...)]
pub mod a {
    #[unstable(feature = "bar", ...)]
    pub fn f() { }
}

Then mod a is deprecated, deprecated f as well:

pub reexport a::f;

#[unstable(feature = "foo", ...)]
#[deprecated(...)]
pub mod a {
    #[unstable(feature = "bar", ...)]
    pub fn f() { }
}

Now part of feature 'bar' is unstable. Then five years later somebody says, 'bar' is finally stable. So they convert all the unstable bars to stable bars, without thinking about it too hard:

pub reexport a::f;

#[unstable(feature = "foo", ...)]
#[deprecated(...)]
pub mod a {
    #[stable(feature = "bar", ...)]
    pub fn f() { }
}

Now they've created a stable function that is immediately deprecated.

That's the only problematic counter-example i can think of offhand.

@Gankra
Copy link
Contributor Author

Gankra commented Jun 8, 2015

Since deprecations and stable are marked with a since it seems possible
to largely guard against this by checking deprecated_since >= stable_since.
Then this can only slip in if a deprecation and stabilization land in the
same release. Maybe not sufficient, though.

On Mon, Jun 8, 2015 at 10:41 AM, Brian Anderson [email protected]
wrote:

The concern about accidentally deprecating multiple features isn't as
important as I was thinking, since features can be partially deprecated,
and deprecations can happen during any version.

Here's one example where inheritance crosses features and could cause
incorrect deprecation.

pub reexport a::f;

#[unstable(feature = "foo", ...)]
pub mod a {
#[stable(feature = "bar", ...)]
pub fn f() { }
}

With deprecation inheritance, if a is deprecated, the function f in the
other feature 'bar' is also deprecated.

There's a current rule that says that anything marked deprecated, must
also be explicitly marked unstable or stable. This is to prevent
unstable+deprecated things accidentally making their way into stable
releases by becoming stable+deprecated. With stable not inheriting though
the only error I can think of making would be accidentally tagging
something that has inherited deprecation with stable.

Imagine:

pub reexport a::f;

#[unstable(feature = "foo", ...)]
pub mod a {
#[unstable(feature = "bar", ...)]
pub fn f() { }
}

Then mod a is deprecated, deprecated f as well:

pub reexport a::f;

#[unstable(feature = "foo", ...)]
#[deprecated(...)]
pub mod a {
#[unstable(feature = "bar", ...)]
pub fn f() { }
}

Now part of feature 'bar' is unstable. Then five years later somebody
says, 'bar' is finally stable. So they convert all the unstable bars to
stable bars, without thinking about it too hard:

pub reexport a::f;

#[unstable(feature = "foo", ...)]
#[deprecated(...)]
pub mod a {
#[stable(feature = "bar", ...)]
pub fn f() { }
}

Now they've created a stable function that is immediately deprecated.

That's the only problematic counter-example i can think of offhand.


Reply to this email directly or view it on GitHub
#26061 (comment).

@brson
Copy link
Contributor

brson commented Jun 10, 2015

@gankro, yes, assuming the person who switches the function from unstable to stable also updates the since field, your proposed assertion would catch the error. It sounds like a reasonable precaution to take, but really this doesn't need to be airtight as long as reviewers pay attention to stabilization.

@brson
Copy link
Contributor

brson commented Jun 10, 2015

Oh, unstable doesn't have a 'since' field, so there's no need to 'remember' to update it when switching from 'unstable' to 'stable'.

@Gankra
Copy link
Contributor Author

Gankra commented Jun 12, 2015

updated to validate stability against deprecation version.

@brson
Copy link
Contributor

brson commented Jun 23, 2015

@bors r+

@bors
Copy link
Collaborator

bors commented Jun 23, 2015

📌 Commit 2962b26 has been approved by brson

bors added a commit that referenced this pull request Jun 23, 2015
Uncertain if this is the desired effect/strategy/testing.

r? @aturon
@bors
Copy link
Collaborator

bors commented Jun 23, 2015

⌛ Testing commit 2962b26 with merge 6fed735...

@bors bors merged commit 2962b26 into rust-lang:master Jun 23, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants