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.

Sign up for GitHub

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

Jump to bottom

Deprecate RustcEncodable and RustcDecodable. #83160

Merged
merged 1 commit into from
Mar 16, 2021
Merged

Deprecate RustcEncodable and RustcDecodable. #83160

merged 1 commit into from
Mar 16, 2021

Conversation

m-ou-se
Copy link
Member

@m-ou-se m-ou-se commented Mar 15, 2021

We can't remove the RustcEncodable and RustcDecodable derive macros from the prelude, but we can deprecate them.

@rust-highfive
Copy link
Collaborator

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 15, 2021
@m-ou-se
Copy link
Member Author

m-ou-se commented Mar 15, 2021 •
edited
Loading

r? @petrochenkov

@m-ou-se m-ou-se added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Mar 15, 2021
@m-ou-se
Copy link
Member Author

m-ou-se commented Mar 15, 2021 •
edited
Loading

I suppose this technically requires an FCP.

As far as I understand, these macros are ancient left-overs from builtin support for rustc-serialize. We don't even show these in the nightly docs anymore. Let's mark them as deprecated.

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Mar 15, 2021 •
edited by joshtriplett
Loading

Team member @m-ou-se has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Mar 15, 2021
@jyn514
Copy link
Member

jyn514 commented Mar 15, 2021

See also https://internals.rust-lang.org/t/what-is-rustc-decodable-encodable-and-why-is-it-stable/13744.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@m-ou-se m-ou-se force-pushed the deprecate-rustc-serialize-derives branch from 852d04e to 924e522 Compare March 15, 2021 19:16
@petrochenkov petrochenkov added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 15, 2021
@BurntSushi
Copy link
Member

Wow. I'm surprised these haven't been deprecated yet.

These were indeed ancient leftovers from the pre-serde era of automatic type driven serialization. IIRC, their names were specifically prefixed with Rustc because we knew that they would eventually be replaced by something better. (And indeed, they were, with many thanks to @erickt and @dtolnay!)

@rfcbot
Copy link

rfcbot commented Mar 15, 2021

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Mar 15, 2021
@eddyb
Copy link
Member

eddyb commented Mar 15, 2021

Wow. I'm surprised these haven't been deprecated yet.

Likely only due to technical limitations, I seem to recall @petrochenkov making it so all of the builtin macros have a presence in the standard library (even w/o any body), at some point in the last couple years.

@m-ou-se
Copy link
Member Author

m-ou-se commented Mar 15, 2021

Seems a bit overkill to wait for a 10 day FCP (and miss the release cycle) for a simple long-overdue deprecation. Marking the FCP as finished.

@m-ou-se m-ou-se added finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Mar 15, 2021
@eddyb
Copy link
Member

eddyb commented Mar 15, 2021

Just to be sure, can we have a test that shows a deprecation warning? I'm worried the current system will only warn on a reexport (or at least an implicit import), not on any use of the derives.

@m-ou-se
Copy link
Member Author

m-ou-se commented Mar 15, 2021 

warning: use of deprecated macro `RustcEncodable`: rustc-serialize is deprecated and no longer supported
 --> src/main.rs:1:10
  |
1 | #[derive(RustcEncodable)]
  |          ^^^^^^^^^^^^^^
  |
  = note: `#[warn(deprecated)]` on by default

@eddyb
Copy link
Member

eddyb commented Mar 15, 2021 •
edited
Loading

Thanks! Feels a bit strange that there were no preexisting tests that changed output, but maybe that's because such a test would need to get the library from crates.io (or at least some mock version of it)?

(compiler/rustc_serialize no longer has a compatible interface to the old derives being deprecated here, it was changed for the custom derives in compiler/rustc_macros/src/serialize.rs, and should probably be renamed anyway)

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Mar 15, 2021

📌 Commit 924e522 has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 15, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Mar 16, 2021
@Dylan-DPC
Rollup merge of rust-lang#83160 - m-ou-se:deprecate-rustc-serialize-d…
1a513c4 
…erives, r=petrochenkov

Deprecate RustcEncodable and RustcDecodable.

We can't remove the `RustcEncodable` and `RustcDecodable` derive macros from the prelude, but we can deprecate them.
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Mar 16, 2021
Closed
@JohnTitor JohnTitor mentioned this pull request Mar 16, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 16, 2021
@bors
Auto merge of rust-lang#83199 - JohnTitor:rollup-zrfk94a, r=JohnTitor
1d6754d 
Rollup of 10 pull requests

Successful merges:

 - rust-lang#81822 (Added `try_exists()` method to `std::path::Path`)
 - rust-lang#83072 (Update `Vec` docs)
 - rust-lang#83077 (rustdoc: reduce GC work during search)
 - rust-lang#83091 (Constify `copy` related functions)
 - rust-lang#83156 (Fall-back to sans-serif if Arial is not available)
 - rust-lang#83157 (No background for code in portability snippets)
 - rust-lang#83160 (Deprecate RustcEncodable and RustcDecodable.)
 - rust-lang#83162 (Specify *.woff2 files as binary)
 - rust-lang#83172 (More informative diagnotic from `x.py test` attempt atop beta checkout)
 - rust-lang#83196 (Use delay_span_bug instead of panic in layout_scalar_valid_range)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 39af66f into rust-lang:master Mar 16, 2021
@rustbot rustbot added this to the 1.52.0 milestone Mar 16, 2021
@m-ou-se m-ou-se deleted the deprecate-rustc-serialize-derives branch March 17, 2021 20:01
@rfcbot rfcbot added the to-announce Announce this issue on triage meeting label Mar 25, 2021
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Apr 4, 2021
@CDirkx CDirkx mentioned this pull request May 4, 2021
Closed
@chorman0773 chorman0773 mentioned this pull request Jan 5, 2022
Closed
@jhpratt jhpratt mentioned this pull request Sep 20, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@petrochenkov petrochenkov

Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.52.0
Development

Successfully merging this pull request may close these issues.