-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Project json compatibility improvements #21423
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
base: master
Are you sure you want to change the base?
Project json compatibility improvements #21423
Conversation
abc8ad3 to
006f659
Compare
ChayimFriedman2
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We generally do not provide forward compatability for our config. However in this case I do support this, since rust-project.json is often committed and used by different r-a versions. I do think this requires a more wide agreement though, so: CC @rust-lang/rust-analyzer.
I have a concern, though: this could lead to an effective stabilization of format versions we don't officially support, but can't stop supporting because they're too widespread.
|
I think it's warranted generally for the project JSON as opposed to config, not just because people commit their JSON to git, but also because the JSON is generated by other tools that have an independent release cycle. Especially if you're using workspace.discoverConfig. In contrast, RA config is usually done by humans and checked at the time of writing. I recall, from the design discussions around the runnable kinds, Matklad describing some principles for JSON formats and pushing for a vec of runnables rather than a struct, so we would be more relaxed as to what's acceptable and more flexible to accept multiple of each kind, etc. Originally, it was a struct like As a practical example, facebook/buck2#767 will make the rust-project JSON generator incompatible with most users' copies of rust-analyzer unless we wait for RA's half of this functionality to make it to a stable rustup release so that we can point users somewhere. I wish I had made this compatibility PR a year ago so this would not be the case. |
These are not used outside of the project-model crate, ie. instantly converted to other structures.
Makes clear to future editors of this code that they should add #[serde(default)] to new fields.
Was deleted when this code was moved in PR 18043.
006f659 to
f8bc65a
Compare
|
I think it's fine for us to say we don't necessarily support all version of this format here but keep a supported window (like we do with rust versions). Either way sounds like we should merge this quickly (and maybe beta back port as well?) |
|
I'm not sure given the Meta people would be able to merge that PR quick enough for a backport to matter, but maybe it would be helpful to the rest of the community. You can do it if you have the time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this makes sense to me :)
I do think we should update https://rust-analyzer.github.io/book/non_cargo_based_projects.html to explain that unknown runnable types are permitted but ignored. It might also be nice to log a warning on unknown runnables.
| /// May include {label} which will get the label from the `build` section of a crate. | ||
| Flycheck, | ||
|
|
||
| /// For forwards-compatibility, i.e. old rust-analyzer binary with newer workspace discovery tools |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: it's not that they're newer tools, it's that they're tool that can output additional types of runnable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is implied by this comment being attached to a serde(other) variant called "Unknown"
| #[serde(default)] | ||
| runnables: Vec<RunnableData>, | ||
| // | ||
| // New fields should be Option or #[serde(default)]. This applies to most of this datastructure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"most of this datastructure" is ambiguous: which parts of the datastructure? How about "everything should be optional except crates, so we can add support for new fields that old rust-project.json tools aren't generating yet".
|
Wearing my rust-project maintainer hat, we package r-a the same way as OSS does: including the binary in the VS Code extension. So if a user hasn't updated their extension, adding new types of runnables to rust-project.json is going to break their setup. Given that, making r-a tolerant to unknown runnable types seems worthwhile. |
With my former-Meta engineer/current rust-analyzer team member hat on, I recommend not committing and instead generating a
The details are already fading from my memory: rust-project was packaged with dotslash, so the repo/dotslash version might move forward, but the extension might be not updated, right? |
Adds better backwards- and forwards-compatibility to the project JSON deserializer. For example, today, if you use
flycheckrunnables from #18043 with an old rust-analyzer, it bails on deserializing the whole structure and you can't do anything. That's fixed (at least for new rust-analyzer). Otherwise adds and recommends more #[serde(default)].Updated: