Update codegen for 2018-06-22 nightly. #666

jebrosen · 2018-06-19T06:47:37Z

Fixes #660 in the master branch. ~~Depends on rust-lang/rust#51664.~~

parse_seq is identical to the method removed from libsyntax.

Also includes fix for next breakage, introduced around 2018-06-22.

~~parse_optional_str and mk_mac_expr (which are private) are now re-implemented by Rocket in parser_ext.rs by a copy/paste and fixing up the paths.~~
parse_seq was deleted. This reimplementation is not quite identical: it uses parse_seq_to_end, which we already asked be made public, instead of parse_seq_to_before_end, which is still private. I think the span associated with the result is one character longer than it was in the original implementation. I would like to fix that but I am not sure how to correct it (or even inspect or verify it).

~~If a "quick fix" that makes no changes to libsyntax is useful or helpful, this appears to work. It does have a likelihood of bitrot, but Rocket will eventually move away from libsyntax anyway.~~

If requesting changes to libsyntax is preferred, I think we can get by with parse_optional_str and mk_mac_expr public, and either reintroduce parse_seq or re-implement it (correctly, not necessarily this implementation) in Rocket. Having parse_seq_to_before_end would help if we were to re-implement parse_seq in Rocket.

~~EDIT: I have not (yet) verified that the nightly bound (2018-06-12) is actually sufficient, but I believe it should be.~~

SergioBenitez · 2018-06-20T08:48:50Z

@jebrosen The implementations for these functions: are they copied straight from the rustc source code, or did you reimplement them yourself?

jebrosen · 2018-06-20T09:29:28Z

These are all from rustc with only path/use directive changes, and the difference mentioned for parse_seq. From the standpoint of correctness, they should be fine now but would need to be kept up to date with any future changes in libsyntax. From the standpoint of attribution and maintenance, I would definitely ask the rust project for their opinion before considering merging this or any similar code. I probably jumped the gun by posting this as a PR.

Rereading at my original comment, it was not very clear that my intent here is to demonstrate that it is possible for this code to be duplicated and in the case of of parse_seq resurrected in rocket, not to argue that it is a desirable approach.

From my point of view, these are the options I see in order of desirability:

Move away from using libsyntax, which I understand Rocket is not quite ready to do yet.
Request the methods to be made public and/or defined again, which will take at least one or two days, and they may not want to bring back parse_seq.
This solution, which I consider a stopgap in case parse_seq will not be brought back in libsyntax.

SergioBenitez · 2018-06-20T12:17:50Z

We absolutely want to move away from libsyntax and compiler-plugins in general in favor of proc-macros, but proc-macros appear to be more unstable that compiler-plugins themselves. What's more, they're ill-defined at the moment, quite buggy, and not powerful enough. I plan to aid in ameliorating this situation soon.

I think we should proceed with a combination of 2) and 3) as follows: if the method still exists in libsyntax, lets submit a PR to make them public again. If not, let's reimplement them in rocket for now. We want to reduce our exposure to breakage as much as possible.

jebrosen · 2018-06-20T18:03:21Z

Sounds good.

I am preparing a PR for rust-lang/rust that makes the these methods public, and will remove them from this PR. parse_seq can then be identical to the implementation that was removed from rustc.

@Mark-Simulacrum

…-Simulacrum make more libsyntax methods public Followup for rust-lang#51502, which was sufficient only for the latest stable release of Rocket. The `master` branch uses a few more. I plan to reimplement the deleted method `parse_seq` in Rocket (see rwf2/Rocket#666), rather than resurrecting it in libsyntax. r? @Mark-Simulacrum

jebrosen · 2018-06-22T23:06:35Z

Rebased, rewrote, and also updated for compatibility with changes introduced by rust-lang/rust#48149.

jebrosen · 2018-06-23T02:51:27Z

core/codegen/src/decorators/derive_form.rs

@@ -27,11 +27,11 @@ fn struct_lifetime(ecx: &mut ExtCtxt, item: &Annotatable, sp: Span) -> Option<St
            ItemKind::Struct(_, ref generics) => {
                let mut lifetimes = generics.params.iter()
                    .filter_map(|p| match *p {
-                        GenericParam::Lifetime(ref def) => Some(def),
+                        GenericParam { ref ident, ref kind, .. } if *kind == GenericParamKind::Lifetime => Some(ident),


This is the only part I'm not quite certain of. Seems to work, but there might be a better way of doing this.

Does ident include the initial tick '? You should take a look at the generated code and ensure that it looks correct, even if it's accepted by the compiler otherwise. Aside from this, this looks good, though it would seem that kind doesn't need to be a ref kind.

I'm pushing a slightly different fix for this in 0.3. Let's use the same one for master.

jebrosen · 2018-06-23T02:52:46Z

core/codegen/src/utils/parser_ext.rs

@@ -14,6 +15,10 @@ pub trait ParserExt<'a> {

    // Like `parse_ident` but also looks for an `ident` in a `Pat`.
    fn parse_ident_inc_pat(&mut self) -> PResult<'a, Ident>;
+
+    // Duplicates previously removed method in libsyntax


Should this link back to the rust repository more concretely? I wasn't sure if I should refer to a PR, commit, URL, or just leave a generic note like this.

Let's link to the PR that removed the method. Something like, "See https://github.com/rust-lang/rust/foo/bar/baz for more details.".

SergioBenitez · 2018-06-23T06:54:32Z

Merged in 5b8f8ee. Thanks for the excellent work, @jebrosen!

jebrosen requested a review from SergioBenitez June 19, 2018 06:47

jebrosen mentioned this pull request Jun 20, 2018

make more libsyntax methods public rust-lang/rust#51664

Merged

jebrosen changed the title ~~Update codegen for 2018-06-12 nightly.~~ Update codegen for 2018-06-??? nightly. Jun 20, 2018

jebrosen force-pushed the nightly0612-master branch from e0e96b1 to c5a24ee Compare June 22, 2018 23:04

jebrosen changed the title ~~Update codegen for 2018-06-??? nightly.~~ Update codegen for 2018-06-22 nightly. Jun 22, 2018

Update codegen for 2018-06-22 nightly.

f33c8e3

jebrosen force-pushed the nightly0612-master branch from c5a24ee to f33c8e3 Compare June 23, 2018 02:22

jebrosen commented Jun 23, 2018

View reviewed changes

SergioBenitez closed this Jun 23, 2018

SergioBenitez added the pr: merged This pull request was merged manually. label Jun 23, 2018

jebrosen deleted the nightly0612-master branch June 29, 2018 17:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update codegen for 2018-06-22 nightly. #666

Update codegen for 2018-06-22 nightly. #666

jebrosen commented Jun 19, 2018 •

edited

Loading

SergioBenitez commented Jun 20, 2018

jebrosen commented Jun 20, 2018

SergioBenitez commented Jun 20, 2018

jebrosen commented Jun 20, 2018

jebrosen commented Jun 22, 2018

jebrosen Jun 23, 2018

SergioBenitez Jun 23, 2018

SergioBenitez Jun 23, 2018

jebrosen Jun 23, 2018

SergioBenitez Jun 23, 2018

SergioBenitez commented Jun 23, 2018

Update codegen for 2018-06-22 nightly. #666

Update codegen for 2018-06-22 nightly. #666

Conversation

jebrosen commented Jun 19, 2018 • edited Loading

SergioBenitez commented Jun 20, 2018

jebrosen commented Jun 20, 2018

SergioBenitez commented Jun 20, 2018

jebrosen commented Jun 20, 2018

jebrosen commented Jun 22, 2018

jebrosen Jun 23, 2018

Choose a reason for hiding this comment

SergioBenitez Jun 23, 2018

Choose a reason for hiding this comment

SergioBenitez Jun 23, 2018

Choose a reason for hiding this comment

jebrosen Jun 23, 2018

Choose a reason for hiding this comment

SergioBenitez Jun 23, 2018

Choose a reason for hiding this comment

SergioBenitez commented Jun 23, 2018

jebrosen commented Jun 19, 2018 •

edited

Loading