Use runnable kind to disambiguate between cargo and shell commands#22295
Draft
sunshowers wants to merge 1 commit intorust-lang:masterfrom
Draft
Use runnable kind to disambiguate between cargo and shell commands#22295sunshowers wants to merge 1 commit intorust-lang:masterfrom
sunshowers wants to merge 1 commit intorust-lang:masterfrom
Conversation
rustbot
added
the
S-waiting-on-review
Author
|
I'm wondering though if r-a should just drop the |
Found this bug while investigating why configuring nextest based on the instructions at rust-lang#21137 (comment) wasn't working within Zed. Previously, we'd use `serde(untagged)`, preferring cargo over shell commands. The problem is that every instance of a shell command is a valid instance of a cargo command. For example, the shell command: ```json { "label": "test my_test", "kind": "shell", "args": { "environment": {"RUSTC_TOOLCHAIN": "/path/to/toolchain"}, "cwd": "/project", "program": "cargo", "args": ["nextest", "run", "--package", "my-crate", "--lib", "--", "my_test", "--exact", "--include-ignored"] } } ``` would end up getting deserialized as a Cargo command, silently dropping `program` and `args`. With this fix, we now use the provided `kind` as a tag. We do have to introduce a `#[serde(flatten)]` unfortunately, which has a few side effects due to internal buffering, but `#[serde(untagged)]` also does internal buffering so this doesn't make things worse. See zed-industries/zed#54011 for the original fix to Zed.
Author
|
(going to follow up on the test failure tomorrow) |
rustbot
removed
the
S-waiting-on-review
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See zed-industries/zed#54011 for the original fix to Zed. That project had copied over these type definitions from rust-analyzer, so we should fix that here too, even though I don't believe r-a deserializes these in production.
Found this bug while investigating why configuring nextest based on the instructions at #21137 (comment) wasn't working within Zed.
Previously, we'd use
serde(untagged), preferring cargo over shell commands. The problem is that every instance of a shell command is a valid instance of a cargo command. For example, the shell command:{ "label": "test my_test", "kind": "shell", "args": { "environment": {"RUSTC_TOOLCHAIN": "/path/to/toolchain"}, "cwd": "/project", "program": "cargo", "args": ["nextest", "run", "--package", "my-crate", "--lib", "--", "my_test", "--exact", "--include-ignored"] } }would end up getting deserialized as a Cargo command, silently dropping
programandargs.With this fix, we now use the provided
kindas a tag. We do have to introduce a#[serde(flatten)]unfortunately, which has a few side effects due to internal buffering, but#[serde(untagged)]also does internal buffering so this doesn't make things worse.