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

Initial import of the Aster syntax::ast builder library #28006

Closed
wants to merge 1 commit into from

Conversation

erickt
Copy link
Contributor

@erickt erickt commented Aug 26, 2015

This imports the aster library into the Rust repository as an unstable library. Aster is a syntax::ast builder, that simplifies the generation of rust AST. It abstracts away many of the default options, and shields the user from fields being added, removed, or restructured in many circumstances. This allows a library like Serde to be much more reliably compiled on nightly, which has not been broken by aster in the past couple months.

This is specifically being done for the Servo project, which has started to use Serde and has gotten broken a few times because of the lag between libsyntax changes and the requisite changes needed to be made in Aster.

I realize that while this is an unstable library this still might need to go through the RFC process, so please let me know and I'll write one up.

cc @nrc, @pcwalton, @SimonSapin, @Manishearth

@rust-highfive
Copy link
Collaborator

r? @brson

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

@Manishearth
Copy link
Member

🎊

IMO we should let this bake on master for a while without an RfC and then make one when we plan to stabilize it. I don't see the point for an RfC in the more unstable parts of the compiler unless you want more feedback on it.

I'll do some reviewing of this PR later, no time now 😄

@erickt erickt force-pushed the aster branch 2 times, most recently from de0d962 to 2d35409 Compare August 26, 2015 20:37
@@ -1 +1 @@
Subproject commit 874dc4ee4cb782056469f003831bcda3e4cdf0df
Subproject commit bff69076975642c64e76dbeaa53476bfa7212086
Copy link
Contributor

Choose a reason for hiding this comment

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

Bad revert.

This imports the [aster](https://github.com/serde-rs/serde) library
into the Rust repository as an unstable library. Aster is a
`syntax::ast` builder, that simplifies the generation of rust AST.
It abstracts away many of the default options, and shields the
user from fields being added, removed, or restructured in many
circumstances. This allows a library like
[Serde](https://github.com/serde-rs/serde) to be much more
reliably compiled on nightly, which has not been broken by aster
in the past couple months.

This is specifically being done for the Servo project, which has
started to use Serde and has gotten broken a few times because
of the lag between libsyntax changes and the requisite changes
needed to be made in Aster.
@erickt
Copy link
Contributor Author

erickt commented Aug 26, 2015

@brson: thanks for catching that! I removed those changes.

@brson
Copy link
Contributor

brson commented Aug 26, 2015

r? @nrc

@rust-highfive rust-highfive assigned nrc and unassigned brson Aug 26, 2015
@brson
Copy link
Contributor

brson commented Aug 27, 2015

For my part I don't think I should make this decision. I can understand why we might want this in-tree to keep it from breaking, but I'm wary: we don't import entire crates into the compiler often; the prime motivation here is for Servo, not general improvement; there may be ways to offer the stability Servo needs out of tree (with more work perhaps); this forces our hand on compiler interfaces, positioning this as the way to build ASTs without a lot of premeditation on our end.

Just listing downsides. Don't feel strongly.

@Manishearth
Copy link
Member

the prime motivation here is for Servo

While that is the immediate motivation, I see lots of value in having a more stable libsyntax in-tree for people to use. Aster has already proven its public interface to be pretty stable, and this might pave the way for eventual libsyntax stabilization. If we don't like it, we can always remove it -- we're not proposing to mark aster as stable, but having a practically stable (even if officially unstable) library sounds better than a practically and officially unstable library.

(Also, serde isn't just used by Servo, it seems to be a rather popular library 😄)

@SimonSapin
Copy link
Contributor

This is specifically being done for the Servo project, which has started to use Serde and has gotten broken a few times because of the lag between libsyntax changes and the requisite changes needed to be made in Aster.

I don’t believe this is inevitable. With http://nightli.es/ or something like it we can be notified of breakage within a day or so, and with more people who can publish new releases Aster can be kept closely up-to-date with Nightly.

@erickt
Copy link
Contributor Author

erickt commented Aug 29, 2015

@SimonSapin: Did the aster breakage from earlier today cause you any problems?

@SimonSapin
Copy link
Contributor

No, I just got notified from https://github.com/SimonSapin/run-nightly

@nrc
Copy link
Member

nrc commented Aug 30, 2015

fwiw, I would much rather have a mechanism for keeping crates like this up to date without importing it into the distro. I think other projects could benefit (I know rustfmt would) and otherwise we will end up getting a bloated distro.

This should also be discussed somewhere more visible. I'll start an internals thread, it probably doesn't need an RFC.

The libs team have a path to stable which involves first being a 'blessed' crate as part of the rust-lang GH org, then possibly being added to the distro. We should probably follow that path here.

I'll postpone a proper review until we decide what to do.

@nrc
Copy link
Member

nrc commented Aug 30, 2015

@bors
Copy link
Contributor

bors commented Sep 6, 2015

☔ The latest upstream changes (presumably #27893) made this pull request unmergeable. Please resolve the merge conflicts.

@alexcrichton
Copy link
Member

I'm gonna close this due to inactivity (and I think the internals thread was a bit up in the air), but feel free to reopen!

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.

8 participants