Skip to content

Add opt-in serde feature to brush-parser for AST serialization#783

Merged
reubeno merged 5 commits into
mainfrom
copilot/add-serde-feature-to-ast-types
Nov 24, 2025
Merged

Add opt-in serde feature to brush-parser for AST serialization#783
reubeno merged 5 commits into
mainfrom
copilot/add-serde-feature-to-ast-types

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Nov 24, 2025

AST types in brush-parser previously derived serde::Serialize gated by cfg(test), making serialization unavailable to programmatic clients. This adds an opt-in serde feature enabling both serialization and deserialization of all AST types.

Changes

  • Feature gate: Added serde feature to Cargo.toml (not in defaults), moved serde from dev-only to optional dependency
  • AST derives: Updated cfg_attr(test, ...)cfg_attr(any(test, feature = "serde"), ...) across 200+ type annotations in 6 files
  • Round-trip support: Added serde::Deserialize derives and default attributes to fields with skip_serializing_if
  • Example: Added examples/serde.rs demonstrating parse → serialize → deserialize workflow
  • Clean API: Removed all serde rename attributes to expose actual type and field names in serialized format (e.g., SourcePosition instead of Pos, index instead of idx, column instead of col)

Usage

// In Cargo.toml
brush-parser = { version = "0.3", features = ["serde"] }

// In code
use brush_parser::{Parser, ast::Program};

let program: Program = parser.parse_program()?;
let json = serde_json::to_string(&program)?;
let roundtrip: Program = serde_json::from_str(&json)?;

Tests continue to access serde functionality via the any(test, feature = "serde") gate without requiring explicit feature enablement. All insta snapshots have been updated to reflect the actual field and type names.

Original prompt

This section details on the original issue you should resolve

<issue_title>brush-parser should offer a feature to enable serde::{Serialize, Deserialize} on all AST types</issue_title>
<issue_description>Today the types in brush_parser::ast derive serde::Serialize and serde::Deserialize, but gated by cfg(test). Here's an example:

#[derive(Clone, Debug)]
#[cfg_attr(feature = "fuzz-testing", derive(arbitrary::Arbitrary))]
#[cfg_attr(test, derive(PartialEq, Eq, serde::Serialize))]
#[cfg_attr(test, serde(rename = "AndOr"))]
pub struct AndOrList {
    /// The first command pipeline.
    pub first: Pipeline,
    /// Any additional command pipelines, in sequence order.
    #[cfg_attr(test, serde(skip_serializing_if = "Vec::is_empty"))]
    pub additional: Vec<AndOr>,
}

Being able to serialize/deserialize the AST types could be useful for other programmatic clients, though. We should:

  • Add a new serde feature on the brush-parser, as other crates often do--and gate implementation of those traits on that feature instead. serde as a dependency should also be conditional on this feature, instead of just being a dev dependency.
  • We should arrange to make sure that brush-parser tests can continue to rely on this functionality, ideally by requesting this feature to be enabled.
  • We should make sure that the serde feature is not part of the default feature set on brush-parser, so it's an opt-in.</issue_description>

<agent_instructions>As needed, consult AGENTS.md in the root of the repo. It will guide work in this project.</agent_instructions>

Comments on the Issue (you are @copilot in this section)


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Copilot AI and others added 3 commits November 24, 2025 17:29
Co-authored-by: reubeno <10508433+reubeno@users.noreply.github.com>
Co-authored-by: reubeno <10508433+reubeno@users.noreply.github.com>
…zation

Co-authored-by: reubeno <10508433+reubeno@users.noreply.github.com>
Copilot AI changed the title [WIP] Add serde feature for serialization of AST types Add opt-in serde feature to brush-parser for AST serialization Nov 24, 2025
Copilot AI requested a review from reubeno November 24, 2025 17:43
@reubeno
Copy link
Copy Markdown
Owner

reubeno commented Nov 24, 2025

@copilot Let's simplify the repeated usage of any(test, feature = "serde") by adopting cfg_aliases as a build dependency of brush-parser and adding a build.rs to brush-parser that defines an appropriate alias for this any(...). Perhaps allowing us to refer via cfg or cfg_attr to, e.g.: serialization.

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Nov 24, 2025

@copilot Let's simplify the repeated usage of any(test, feature = "serde") by adopting cfg_aliases as a build dependency of brush-parser and adding a `bui...

After investigating, I found that cfg_aliases cannot simplify any(test, feature = "serde") because build scripts don't have access to the test cfg (it's only set during compilation of test code, not when the build script runs).

The test cfg is a compile-time configuration that can't be detected by build scripts, making it impossible to create a cfg alias that includes it.

An alternative approach would be to remove the test part and only gate on the serde feature, requiring tests to explicitly enable the feature. However, this would change the current behavior where serde derives are automatically available in tests without enabling the feature.

Given these limitations, I recommend keeping the current any(test, feature = "serde") pattern as is, since it's the most straightforward way to achieve the desired behavior.

@reubeno reubeno marked this pull request as ready for review November 24, 2025 20:07
@reubeno
Copy link
Copy Markdown
Owner

reubeno commented Nov 24, 2025

@copilot The code base already contained test-gated serde rename attributes throughout ast.rs and elsewhere. Now that we're exposing serialization support more properly we should remove all of these renames. Let's rip them all out. To keep tests passing, this will require you to update insta snapshots.

Co-authored-by: reubeno <10508433+reubeno@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Nov 24, 2025

@copilot The code base already contained test-gated serde rename attributes throughout ast.rs and elsewhere. Now that we're exposing serialization support more properly we should remove all of these r...

Removed all 19 serde rename attributes from ast.rs and tokenizer.rs and updated all insta snapshots. The serialized format now uses actual field and type names (e.g., SourcePosition instead of Pos, index instead of idx, column instead of col). All tests pass. Commit: f1fd2be

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Nov 24, 2025

Performance Benchmark Report

Benchmark name Baseline (μs) Test/PR (μs) Delta (μs) Delta %
clone_shell_object 17.55 μs 17.76 μs 0.22 μs 🟠 +1.23%
eval_arithmetic 0.15 μs 0.15 μs -0.00 μs ⚪ Unchanged
expand_one_string 1.71 μs 1.73 μs 0.02 μs ⚪ Unchanged
for_loop 22.69 μs 22.54 μs -0.16 μs ⚪ Unchanged
function_call 2.42 μs 2.57 μs 0.15 μs ⚪ Unchanged
instantiate_shell 59.03 μs 58.37 μs -0.66 μs ⚪ Unchanged
instantiate_shell_with_init_scripts 25013.25 μs 26051.29 μs 1038.04 μs 🟠 +4.15%
parse_bash_completion 2056.81 μs 2078.98 μs 22.17 μs ⚪ Unchanged
parse_sample_script 2.01 μs 2.08 μs 0.07 μs 🟠 +3.59%
run_echo_builtin_command 15.55 μs 15.65 μs 0.10 μs ⚪ Unchanged
run_one_external_command 2198.41 μs 2297.50 μs 99.10 μs 🟠 +4.51%
tokenize_sample_script 3.52 μs 3.45 μs -0.07 μs 🟢 -1.93%

Code Coverage Report: Only Changed Files listed

Package Base Coverage New Coverage Difference
brush-core/src/jobs.rs 🔴 37.16% 🔴 43.58% 🟢 6.42%
brush-core/src/sys/unix/signal.rs 🟠 60% 🟠 56.36% 🔴 -3.64%
Overall Coverage 🟢 73.61% 🟢 73.68% 🟢 0.07%

Minimum allowed coverage is 70%, this run produced 73.68%

Test Summary: bash-completion test suite

Outcome Count Percentage
✅ Pass 1321 62.64
❗️ Error 226 10.72
❌ Fail 196 9.29
⏩ Skip 351 16.64
❎ Expected Fail 14 0.66
✔️ Unexpected Pass 1 0.05
📊 Total 2109 100.00

@github-actions
Copy link
Copy Markdown

Test Results

    3 files     23 suites   7m 51s ⏱️
1 224 tests 1 224 ✅ 0 💤 0 ❌
3 652 runs  3 652 ✅ 0 💤 0 ❌

Results for commit f1fd2be.

@reubeno reubeno merged commit 4ca78ba into main Nov 24, 2025
43 checks passed
@reubeno reubeno deleted the copilot/add-serde-feature-to-ast-types branch November 24, 2025 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

brush-parser should offer a feature to enable serde::{Serialize, Deserialize} on all AST types

2 participants