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

Use crates.io version of rustc-serialize #40527

Closed
nrc opened this issue Mar 14, 2017 · 16 comments
Closed

Use crates.io version of rustc-serialize #40527

nrc opened this issue Mar 14, 2017 · 16 comments
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@nrc
Copy link
Member

nrc commented Mar 14, 2017

We must ensure that the crates.io version has all the functionality the internal one does. Apparently the only thing missing is https://github.com/rust-lang/rust/blob/master/src/libserialize/serialize.rs#L748-L815

Then we just need to change the deps in the Cargo.tomls and remove the crate.

Motivation is that I want save-analysis to depend on an external crate which in turn depends on rustc-serialize (the crates.io version). (We can't use Serde because of complex dependencies on the proc macro crate). That means we will pull in the crates.io version, so we may as well just have that, rather than two versions kicking around (and two sources of truth, etc.).

cc #40177

@alexcrichton
Copy link
Member

There's an attempt to do this for liblog as well, but that has issues I'll need to address before it lands.

@alexcrichton
Copy link
Member

(oops didn't mean to close)

@nrc
Copy link
Member Author

nrc commented Mar 15, 2017

So, thinking about this, we're somewhat getting into the question of the future of librustc-serialize. The internal and external variants have diverged somewhat, most notably in #36551. The internal version now uses unstable code, whereas the external version is stable.

So a few courses of possible action:

  • we un-stabilise the external version and bring it up to date. The philosophy here is that most users should use Serde and rustc-serialize is just for use in rustc, its just hosted outside the repo for ease of development
  • we land the changes as Cargo features, so you can opt in to the unstable stuff
  • we re-implement the changes using macros (@eddyb suggests this is possible in the PR). The philosophy in these two cases is that rustc-serialize is a general purpose serialisation library that happens to be used by the compiler for some specific stuff that most people won't bother themselves with.
  • we could abandon the external version and only use the internal version. This breaks some things I'm doing with save-analysis, but perhaps there are work-arounds.

@rust-lang/libs @eddyb thoughts?

@sfackler
Copy link
Member

I'd feel a bit uncomfortable about declaring the external rustc-serialize "unstabilized" - it's still being downloaded 11,000 times a day!

The specialization stuff does seem like it'd be reasonable to have under a Cargo feature, though.

@sfackler
Copy link
Member

It seems like the thing we should really be doing is switching over to serde, right?

@nrc
Copy link
Member Author

nrc commented Mar 15, 2017

@sfackler see discussion in #40177 tl;dr: rustc-serialize does some stuff that Serde doesn't and which it doesn't make sense to move into Serde. We might consider using Serde for JSON stuff and rustc-serialize (or something new) for metadata, etc.

@sfackler
Copy link
Member

Cool, makes sense.

@alexcrichton
Copy link
Member

I don't personally quite have a handle on the difference between the internal and external versions. @eddyb could you clarify what they are?

(that'd help guide my opinion at least on what to do here)

@eddyb
Copy link
Member

eddyb commented Mar 16, 2017

The specialization stuff does seem like it'd be reasonable to have under a Cargo feature, though.

It's not, it's actually abusing specialization unsoundness. #40177 and #36588 are relevant.
@nrc Is it possible for save-analysis and effectively anything involving JSON to switch to serde?

If so, the only blocker is me figuring out how to get serde working with the compiler bootstrapping.
How high-priority is this, i.e. what's the timeline here? @nikomatsakis also wants me to investigate moving off of rustc-serialize, for a different reason (incremental/on-demand, mostly).

@nrc
Copy link
Member Author

nrc commented Mar 17, 2017

@eddyb we currently have problems with external crates using Serde - because it links against the proc_macro crate and thus there are build order issues. Also because we'd need to support proc macros with a stage 1 build in order to build the stage 2 compiler. So, basically moving to Serde for JSON stuff is a lot more effort than it sounds like :-(

@eddyb
Copy link
Member

eddyb commented Mar 17, 2017

@nrc Right, that's what I mean, I'm supposed to solve those problems... somehow!

frewsxcv added a commit to frewsxcv/rust that referenced this issue Mar 20, 2017
Use rls-data crate

This basically pulls out a bunch of data structures used by save-analysis for serialization into an external crate, and pulls that crate in using Rustbuild. The RLS can then share these data structures with the compiler which in some cases will allow more efficient communication between the compiler and the RLS (i.e., without serialisation).

Along the way, I have to pull in rls-span, which is the RLS's way of defining spans (more type-safe than the compiler's built-in way). This is basically just to convert from compiler spans to RLS spans.

I also pull in the crates.io version of rustc-serialize, which is a bit annoying, but seems to be the only way to have serialisable data in an external crate. To make this work, all of the save-analysis crate has to use this version too (cc rust-lang#40527).

Finally I pull in a line from rust-lang#40347 to make the unstable crate checking stuff working.

There are a lot of changes to save-analysis but they are all mechanical and trivial - changing from using `From` to `Into` (because of orphan rules) being the main thing.

r? @alexcrichton
@Mark-Simulacrum Mark-Simulacrum added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 13, 2017
@Mark-Simulacrum Mark-Simulacrum added the C-cleanup Category: PRs that clean code up or issues documenting cleanup. label Jul 27, 2017
@steveklabnik
Copy link
Member

Triage: still using an internal version.

@Xanewok
Copy link
Member

Xanewok commented Apr 17, 2019

@eddyb with #49219 merged, is it possible to do the switch to crates.io version now?

@Xanewok
Copy link
Member

Xanewok commented Apr 18, 2019

#60053 switches save-analysis to use serde, which is the only usage of crates.io version of rustc_serialize in the Rust tree IIRC.

Because of this, I believe we wouldn’t have to switch to crates.io version for other usages if we need the unstable/internal API in the compiler for rustc_serialize usage in-tree?

@jonas-schievink
Copy link
Contributor

This feels like... something we should just close at this point, maybe? The unsound specialization abuse shouldn't really be on crates.io, and even without that the benefits of updating it are questionable as almost everyone has moved off of rustc-serialize by now.

@Mark-Simulacrum
Copy link
Member

I would be in favor of closing this, yeah, it doesn't seem useful to track separately (we probably do want a bug for the soundness hole we're exploiting? Not sure what issue that is). Going to go ahead and do that and if someone disagrees feel free to reopen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

8 participants