For anyone altering code or reviewing changes in libsyntax, please consider the affect your changes will have on tools and syntax extensions. Although we don’t offer stability guarantees, libsyntax is public API and is used by many syntax extensions and tools. If you change API you are making a breaking change. This has costs and we should be sure that the benefits outweigh those costs.
Just because libsyntax is unstable, does not mean we can make breaking changes without consequences. We should treat libsyntax essentially like we treated the standard libraries before 1.0. Breaking changes are fine, but there must be justification.
There have been several PRs landed in the past few weeks which are very low benefit ‘tidying up’ patches, but which have broken large numbers of syntax extensions. More worrying is that I have observed several ‘rubber stamp’ reviews where it doesn’t look like the reviewer has considered stability of the API at all. Please, lets do better than this!
While I agree with the spirit, it is unclear to me what this means in practice. As I remember, we broke standard libraries before 1.0 over quite small things, for example better naming.
For example, before reading this, I would have “rubber stamp” approved PR changing ExprAgain to ExprContinue. (It would be quite unfortunate if we froze ExprAgain.) After reading this, it is unclear to me what I should do in this case. I still think it is okay, but I can imagine disagreements.
I think the key thing is intention. If we think renaming a function will lead to a better situation for users both inside and outside of the compiler, then yes, go for it! If the renaming is a marginal improvement which is just tidying up some ‘internals’ without thinking of external users, then I’d say no. I’m not saying we need to freeze libsyntax, just that when making (and reviewing changes) we should be concious of external users as well as ourselves.
To get specific, I think renaming ExprAgain to ExprContinue is totally acceptable because it brings the AST names closer to the syntax and thus will make life easier for future plugin/tool authors. In contrast, renaming codemap to code_map (which really bothers me, btw) whilst an improvement in aesthetic terms, does not have enough benefit to justify breaking our users (at least without some consideration and perhaps combining it with other renames to make the breakage less painful, etc.).
If in doubt, checking on irc is a great way to get some consensus about these kinds of decisions (which of course are full of grey areas). Or ping people on the PR - I’d rather a PR takes a day or two longer to land due to being over-cautious than we break our users by not being cautious enough.
In principle, stability is nice, but I feel like this pushes us more and more towards de facto stabilisation of the compiler. Incrementally increasing the activation energy needed to make a change will mean (in theory) things break less often, and so every change that does happen is more painful (e.g. more people rely on it because it feels more stable) and hence harder to justify, creating somewhat of a vicious cycle.
That said, it doesn’t seem to bad to try to at least bring it up on IRC, and/or try to batch PRs like this.
I would personally be happy if breaking changes to the public API were tagged as such. (There have been at least two that i can recall in recent memory that weren’t.)
I’d be happy if we collected such breaking changes and release them in bulk (say every two or three weeks) preferably with a PSA detailing the breakage a few hours before merging.
Yeah, that would slow down compiler development and is therefore probably too costly for now. OTOH, if we could collect those changes at one branch, it would still be easy to test and I would personally volunteer to write the PSAs.
How much would it really slow down development? rust-phf's had to be fixed ~3 times for super minor refactors in the last couple of weeks, all of which could have been trivially prevented with a #[deprecated] shim function that we could keep around until the next release or whatever.
@huon I agree that we don’t want to go too far in the opposite direction, in no way am I proposing moving towards stability for libsyntax. One thing that breaks the vicious cycle is that some changes which are pretty minor can just not happen at all (or until we stop exposing libsyntax to tools/plugins). The cost is some cruftiness in exchange for some stability - I believe that is a price worth paying. The other mitigating factor is that large breaking changes are often not much more effort to deal with for plugin authors than small changes, but have much higher benefit for compiler devs. That means changes are more likely to be accepted as they get larger.
@burntsushi Agreed! I’ve been trying to encourage this, I would like other reviewers to do so too.
@llogiq I would like to do this, it is a good idea. However, it is something we haven’t done before and would require quite a bit of coordination and I’m not sure how it would work with testing infrastructure, etc. The cost in peoples’ time to set this up might be prohibitive.
@sfackler my experience is that the cumulative cost can be quite high due to dependencies - there’s a lot of upgrading, a lot of head scratching, etc. even in projects that only indirectly depend on libsyntax. This might be acceptable for dedicated plugin authors, but is friction for downstream users and complicates stuff like Servo’s Rustup or maintaining projects for benchmarking. The frequency of the breakage is also an issue - if these only broke once per cycle it would be better than once every week or so (in fact, maybe we should have an unstable but slow moving branch, I imagine this won’t be popular though).
Hm, this is kinda sorta exactly what I was thinking would cause the vicious cycle: by not doing the least necessary changes, all the other more necessary changes become more painful... and some of those changes become the new "least necessary" ones, with the bar slowly moving up and up.
The breaking changes to libsyntax are still quite painful when they happen for me because, even though it doesn’t happen as often now, when it does, it takes many days for the syntax extensions my projects depend on to be updated accordingly, which forces me to either roll back to an older version of Rust nightly, or halt development until the syntax extensions are updated. (Specifically, I’m talking about serde_codegen/serde_macros and diesel_codegen.) Is there any possibility that when breaking changes to libsyntax are ready to land, there is some coordination with high-usage crates that will be broken by it so that they can release new crates right away?
Sounds like the Rust team is doing their part then! Thank you for that, Manish! I guess there isn’t much else that can be done other than hope the crate authors can publish new versions more quickly.