Skip to content
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

Destabilize RustcEncodable and RustcDecodable #92594

Conversation

chorman0773
Copy link
Contributor

@chorman0773 chorman0773 commented Jan 5, 2022

This adds the feature rustc_encodable_decodable and gates the previously stable RustcDecodable and RustcEncodable (though deprecated, hidden, and unusable) macros behind them.

These macros have always been a mystery. They have no documentation (and are notably now hidden), cannot be used on stable (except to refer to them, which this PR changes) as their expansion references the rustc_serialize crate (which itself is only exposed as rustc_private), and cannot reasonably be implemented outside of rustc (except as a compile error).
They were deprecated in #83160, but as they cannot be expanded on stable, I see no reason why they cannot be made unstable (or alternatively, removed; I had no issues in rustc itself, from destabilizing the macros, and the only error was in libstd where the macro is reexported in the prelude), however, I would recommend a crater run to be sure (I highly doubt many crates have use core::prelude::v1::RustcDecodable;, though).
See also: https://internals.rust-lang.org/t/what-is-rustc-decodable-encodable-and-why-is-it-stable/13744.

@rust-highfive rust-highfive self-assigned this Jan 5, 2022
@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @rust-highfive (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 5, 2022
@chorman0773
Copy link
Contributor Author

r? highfive

@Mark-Simulacrum
Copy link
Member

Is there a reason to feature-gate these vs. just removing them entirely? I believe these are still usable on stable -- the following compiles with rustc-serialize from crates.io in Cargo.toml, for example. So removal/de-stabilization is a breaking change, and I'm not sure if it's warranted.

#[derive(RustcDecodable, RustcEncodable)]
struct Bar {
    f: u32,
}

fn main() {
    println!("{}", rustc_serialize::json::encode(&Bar { f: 3 }).unwrap());
}

@rust-log-analyzer

This comment has been minimized.

@chorman0773
Copy link
Contributor Author

chorman0773 commented Jan 5, 2022

Is there a reason to feature-gate these vs. just removing them entirely?

I featured gated them for now, but they could be removed entirely instead.

I believe these are still usable on stable -- the following compiles with rustc-serialize from crates.io in Cargo.toml, for example. So removal/de-stabilization is a breaking change, and I'm not sure if it's warranted.

Indeed, which is why I suggested a crater run.
Leaving them in, though, leaves a burden of compatibility, of something entirely unspecified, and highly rustc-specific, for others working on rust implementations (and, arguably, the fact that it does work if you pull in a crate is worse, since it means they can't just be implemented as compile_error!("unsupported"), which is the only logical implementation I can see).

@rdrpenguin04
Copy link

By the way, why is Cargo.lock being modified? It doesn't look like anything there is relevant to this PR to me.

@Aaron1011
Copy link
Member

There's a disturbingly high number of reverse dependencies on rustc-serialize: from https://crates.io/crates/rustc-serialize/reverse_dependencies, there are 598. I think most of them are optional dependencies behind an off-by-default feature, but it's still bad.

I think this definitely needs a Crater run.

@chorman0773
Copy link
Contributor Author

By the way, why is Cargo.lock being modified? It doesn't look like anything there is relevant to this PR to me.

I guess because x.py wanted to?

@rust-log-analyzer

This comment has been minimized.

@petrochenkov petrochenkov self-assigned this Jan 6, 2022
@camelid camelid added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jan 6, 2022
// #13544
// FIXME: This should be replaced with some other derive macro
#![feature(rustc_encodable_decodable)]
#[crate_type="lib"] // Why is this an outer attribute?
Copy link
Member

@camelid camelid Jan 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what the comment is about, but this should be:

Suggested change
#[crate_type="lib"] // Why is this an outer attribute?
#![crate_type="lib"]

EDIT: Ah, I see, the attribute was pre-existing. Strange. Not sure what it's for then.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment is because the #[crate_type="lib"] line appears in the original version of the test, so I inserted a comment questioning it. It caused my first attempt to fix the test to fail because I saw only the attribute itself and not the relatively insignficant ! when I added the #![feature(rustc_encodale_decodable)] line.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's quite strange. I bet the #[ version of the attribute does nothing, and rustc is just not warning about it.

@petrochenkov
Copy link
Contributor

There was already a crater run 2.5 years ago - #62507 (comment).
RustcDecodable caused 418 regressions, and RustcEncodable - 211 regressions.

feature = "rustc_encodable_decodable",
issue = "none",
reason = "RustcDecodable cannot be used stably"
)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can mark both macros as soft-unstable (see macro bench for an example), then the instability will be reported as a deny-by-default lint and won't break other crates depending on crates using these macros.

#[unstable(
feature = "rustc_encodable_decodable",
issue = "none",
reason = "RustcDecodable cannot be used stably"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
reason = "RustcDecodable cannot be used stably"
reason = "unstable implementation detail of the `rustc` compiler, do not use"

@@ -55,7 +55,7 @@ pub use core::prelude::v1::{
pub use core::prelude::v1::concat_bytes;

// Do not `doc(inline)` these `doc(hidden)` items.
#[stable(feature = "builtin_macro_prelude", since = "1.38.0")]
#[unstable(feature = "rustc_encodable_decodable", issue = "none")]
#[allow(deprecated)]
pub use core::prelude::v1::{RustcDecodable, RustcEncodable};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a similar reexport in library/core/src/prelude/v1.rs that also needs to be marked as unstable.

@petrochenkov petrochenkov removed their assignment Jan 6, 2022
@Aaron1011
Copy link
Member

I think this would be a good case the the recently-stabilized future-incompat-report future. Getting rustc to emit a report item for just these deprecations will require a few tweaks, though.

@petrochenkov
Copy link
Contributor

If this is reported as the SOFT_UNSTABLE lint, then it's already a future-compatibility lint, and no additional action should be necessary.

@Aaron1011
Copy link
Member

I was referring to #71249, which will show aessage to downstream crates relying on any of the direct users of these macros.

@chorman0773
Copy link
Contributor Author

I still would like to see an up-to-date crater run on making it fully-unstable, if possible. Not having an implementation detail of rustc be public and stable at all would be idea, but, if not that, then a deny-by-default fcw would be fair enough. Otherwise, there needs to probably be better documentation of the thing.

@Mark-Simulacrum
Copy link
Member

Happy to queue up a Crater run on a hard error or deny by default lint -- is that the current state?

I'd personally be fine with an r+ with something that lets dependencies still build -- deny by default lint, for example, seems reasonable.

@Mark-Simulacrum
Copy link
Member

@craterbot retry

@craterbot
Copy link
Collaborator

🛠️ Experiment pr-92594 queued again.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🚧 Experiment pr-92594 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-92594 is completed!
📊 258 regressed and 3 fixed (205674 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Jan 20, 2022
@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 21, 2022
@nnethercote
Copy link
Contributor

FWIW, I stumbled across these recently and was confused by them. They look like they are defined in core/std, but you have to use rustc_serialize from crates.io for them to work, which is an odd combination. I consider this a minor argument in favour of them going away.

@JohnCSimon
Copy link
Member

Ping from triage:
@chorman0773 Can you post your status on this PR?

When it's ready for review send a message containing
@rustbot ready to switch to S-waiting-on-review

@chorman0773
Copy link
Contributor Author

Sorry about that. I got busy with other things. I've grabbed the logs, and want to go through them to see what crates failed, and whether the failures can be fixed. Until that time, it might be best just to proceed as-is, with the deny-by-default, rather than going straight for hard error.

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 13, 2022
// #13544
// FIXME: This should be replaced with some other derive macro
#![feature(rustc_encodable_decodable)]
#[crate_type="lib"] // Why is this an outer attribute?

extern crate rustc_serialize;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can likely swap this test out for derive(Debug) which'll make it simpler and drop the soft-deprecated feature use.

@Mark-Simulacrum
Copy link
Member

r=me with the PR description updated to reflect soft-deprecation and to avoid misleading commentary about inability to use these on stable.

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 14, 2022
@saethlin
Copy link
Member

Of the published crates that this breaks, only 6 have a publication in the last year, and the median publication date is January 2017 (5 years ago). I tossed the list of broken crates from crater into a little script to grab repository links and last publication dates, for anyone who wants to peruse the list: broken-crates.gz

@JohnCSimon
Copy link
Member

@chorman0773
Ping from triage: Can you please post the status of this PR?

@chorman0773
Copy link
Contributor Author

chorman0773 commented Mar 20, 2022 via email

@JohnCSimon JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 11, 2022
@JohnCSimon JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 8, 2022
@JohnCSimon
Copy link
Member

@saethlin
Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this.
Note: if you do please open the PR BEFORE you push to it, else you won't be able to reopen - this is a quirk of github.
Thanks for your contribution.

@rustbot label: +S-inactive

@JohnCSimon JohnCSimon closed this May 22, 2022
@rustbot rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label May 22, 2022
@saethlin
Copy link
Member

Thanks for the ping but I think you meant @chorman0773

@nnethercote
Copy link
Contributor

For posterity: this ended up being done in #116016.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.