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

Tracking issue for libsyntax breaking changes #31645

Closed
Manishearth opened this issue Feb 14, 2016 · 44 comments
Closed

Tracking issue for libsyntax breaking changes #31645

Manishearth opened this issue Feb 14, 2016 · 44 comments
Labels
A-syntaxext Area: Syntax extensions C-tracking-issue Category: A tracking issue for an RFC or an unstable feature.

Comments

@Manishearth
Copy link
Member

Manishearth commented Feb 14, 2016

See also: https://internals.rust-lang.org/t/please-consider-stability-of-libsyntax/2947

We decided to batch up breaking changes to oft-used unstable compiler internals (mainly, libsyntax) until they're large enough; so that the impact of these breaking changes is minimized. (For example, every time libsyntax breaks, aster and transitively everyone using serde on nightly has to fix it)

Let's try to coordinate these in the future through this issue. We can list breaking PRs here and batch them up when we feel that they're large enough (or when there's another inevitable breaking change about to merge). Merge them when there's enough time for the folks below to know about it, and preferably make PRs to the larger projects below beforehand.

CCing stakeholders:

Add more if you know of them!

@Manishearth Manishearth added metabug Issues about issues themselves ("bugs about bugs") A-syntaxext Area: Syntax extensions labels Feb 14, 2016
@BurntSushi
Copy link
Member

I would also like to re-submit my plea that commits with breaking changes are tagged, and if possible, at least some message that hints at or explains how to fix breakages.

Other crates with macros in use (apparently) are docopt_macros and quickcheck_macros.

@Manishearth
Copy link
Member Author

Yeah, they get tagged with [breaking-change]. With this bug in place they should also be posted here first and we can notify everyone well in advance before a rollup.

bors added a commit that referenced this issue Apr 6, 2016
[breaking-batch] Add support for `pub(restricted)` syntax in the AST

This PR allows the AST to represent the `pub(restricted)` syntax from RFC 1422 (cc #32409).

More specifically, it makes `ast::Visibility` non-`Copy` and adds two new variants, `Visibility::Crate` for `pub(crate)` and `Visitibility::Restricted { path: P<Path>, id: NodeId }` for `pub(path)`.

plugin-[breaking-change] cc #31645
r? @pnkfelix
@Manishearth
Copy link
Member Author

#32767 is a breaking change that will land soon, along with #32688.

I'd normally wait a few days before merging these so that everyone can fix their code early, but in this case #32688 already landed so there's not much I can do.

This will definitely break syntex and friends, cc @erickt. I've never looked at diesel's code so it may or may not break it, @sgrif you may want to have a look before this lands in the nightly.

@sgrif
Copy link
Contributor

sgrif commented Apr 6, 2016

Thanks for the ping, definitely going to cause breakage.

Manishearth added a commit to Manishearth/rust that referenced this issue Apr 6, 2016
 The AST part of rust-lang#31937

Unlike HIR, AST still uses `Option` for field names because parser can't know field indexes reliably due to constructions like
```
struct S(#[cfg(false)] u8, u8); // The index of the second field changes from 1 during parsing to 0 after expansion.
```
and I wouldn't like to put the burden of renaming fields on expansion passes and syntax extensions.

plugin-[breaking-change] cc rust-lang#31645
r? @Manishearth
bors added a commit that referenced this issue Apr 6, 2016
Batch up all plugin breaking changes

#32688 already landed so we should get this into the same nightly.

cc #31645
@Manishearth
Copy link
Member Author

#33041 will land in a few days. It changes the parser and some of the token tree representation

  • Definitely will break syntex/aster/serde (cc @erickt)
  • Might break Diesel, Regex, phf (cc @sgrif, @sfackler, @BurntSushi): anyone who uses macros operating on token trees.

@Manishearth
Copy link
Member Author

Note that #31414 has been added to the rollup. It will break derive plugins, not sure what else.

@Manishearth
Copy link
Member Author

#33157 is also in the list

Manishearth added a commit to Manishearth/rust that referenced this issue Apr 24, 2016
…arth

 Track the span corresponding to the `|...|` part of the closure.

lifted from rust-lang#32756
cc rust-lang#31645

libsyntax-[breaking change]
@Manishearth
Copy link
Member Author

Merging now: #33179

bors added a commit that referenced this issue Apr 24, 2016
Batch up breaking libsyntax changes

Contains:

 - #33125
 - #33041
 - #33157

cc #31645
@Manishearth
Copy link
Member Author

Next breaking batch will contain the above PRs, waiting on the last one. cc @dtolnay @sfackler @mystor @sgrif @BurntSushi

@Manishearth
Copy link
Member Author

Everything seems reviewed, breaking batch will be made todayish (and land over the weekend).

cc @dtolnay @BurntSushi @sgrif @sfackler @llogiq

@Manishearth
Copy link
Member Author

Batch at #36066

@Manishearth
Copy link
Member Author

Landed!

@Manishearth
Copy link
Member Author

Manishearth commented Sep 6, 2016

Stuff for next batch:

mergerollup CensoredUsername:stmt_let_typed_fix 35874 Manishearth
mergerollup jonas-schievink:whats-a-pirates-favorite-data-structure 36599 pnkfelix
mergerollup pnkfelix:attrs-on-generic-formals 34764 eddyb

@mcarton
Copy link
Member

mcarton commented Sep 23, 2016

@Manishearth don't know if it's to late for the two other PRs to make it into the next nightly, but #36551 was a syntax breaking change and has already been merged.

@Manishearth
Copy link
Member Author

Too late :/ But we're rolling it up now anyway

Manishearth added a commit to Manishearth/rust that referenced this issue Sep 30, 2016
…orite-data-structure, r=

 Contains a syntax-[breaking-change] as a separate commit (cc rust-lang#31645).nnAlso renames slice patterns from `PatKind::Vec` to `PatKind::Slice`.
@Manishearth
Copy link
Member Author

Manishearth commented Sep 30, 2016

Breaking batch at #36857 cc @dtolnay @sgrif @BurntSushi

Will merge tomorrowish.

@dtolnay
Copy link
Member

dtolnay commented Sep 30, 2016

This is exciting - I am not planning to do a serde_macros release so we will see if people revolt or move to serde_derive.

@Manishearth
Copy link
Member Author

Should we PSA this?

@dtolnay
Copy link
Member

dtolnay commented Sep 30, 2016

The announcement that we are dropping serde_macros was at the top of reddit for a while and also popular on u.r-l.o. I think people will put two and two together when their builds fail. Worst case we get a handful of issues filed and we point them to the announcement. Do you know a better way to reach people who have not seen either of those already?

@sfackler
Copy link
Member

You could potentially cut a new release that panics with a message telling people to move to serde-derive.

@mcarton
Copy link
Member

mcarton commented Sep 30, 2016

@dtolnay Has this been announced in the Project Updates section of TWiR?

@dtolnay
Copy link
Member

dtolnay commented Sep 30, 2016

No we just missed the last one.

@Manishearth
Copy link
Member Author

The panicky method feels a bit invasive imo.

It should be worth mentioning somewhere (perhaps on the crates.io page, or in the readme) that the method for upgrading is to switch over to the new macro version.

@eddyb
Copy link
Member

eddyb commented Sep 30, 2016

If it has access to the diagnostics machinery, printing a deprecation warning wouldn't be that hard.

Manishearth added a commit to Manishearth/rust that referenced this issue Oct 1, 2016
…orite-data-structure, r=pnkfelix

 Contains a syntax-[breaking-change] as a separate commit (cc rust-lang#31645).nnAlso renames slice patterns from `PatKind::Vec` to `PatKind::Slice`.
@Manishearth
Copy link
Member Author

This landed. beware the breakage!

@sgrif
Copy link
Contributor

sgrif commented Oct 3, 2016

We should be done with the macros 1.1 port later today so I no longer need to get pinged on these. Thank you for including me though!

@Mark-Simulacrum Mark-Simulacrum added C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. and removed metabug Issues about issues themselves ("bugs about bugs") labels Jul 24, 2017
@petrochenkov
Copy link
Contributor

Proc macros are not tied to libsyntax now, so this is no longer relevant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-syntaxext Area: Syntax extensions C-tracking-issue Category: A tracking issue for an RFC or an unstable feature.
Projects
None yet
Development

No branches or pull requests