Skip to content
This repository was archived by the owner on Jun 18, 2026. It is now read-only.

Fix positional nested parameter array parsing#70

Closed
TeamDman wants to merge 10 commits into
bearcove:mainfrom
TeamDman:teamy-positional-bug
Closed

Fix positional nested parameter array parsing#70
TeamDman wants to merge 10 commits into
bearcove:mainfrom
TeamDman:teamy-positional-bug

Conversation

@TeamDman

Copy link
Copy Markdown

A nested command structure caused me problems, see #69

    #[derive(Facet, Debug, PartialEq)]
    struct GradleCommand {
        #[facet(args::positional)]
        tasks: Vec<String>,

        #[facet(rename = "hide-logs", args::named, default = false)]
        hide_logs: bool,
    }

    #[derive(Facet, Debug, PartialEq)]
    #[repr(u8)]
    enum Command {
        Gradle {
            #[facet(flatten)]
            command: GradleCommand,
        },
    }

    #[derive(Facet, Debug, PartialEq)]
    struct Cli {
        #[facet(args::subcommand)]
        command: Command,
    }

The following would fail to parse if there was exactly one item specified

gradle runData
Error: unexpected token: got scalar, expected sequence start at command::Gradle.tasks
   ╭─[ <cli>:1:8 ]
   │
 1 │ gradle runData
   │        ───┬───
   │           ╰───── unexpected token: got scalar, expected sequence start at command::Gradle.tasks
───╯


error: test failed, to rerun pass `-p figue --test main`

The existing behaviour would succeed if multiple items were passed, however

gradle runData test

happy

Disclaimer: this fix was created by GPT-5.3 Codex.


Snapshot tests dislike windows lol

image

Normalize `multiple` CLI args to arrays on first value insert, so `Vec<T>` fields no longer parse as scalar when only one token is provided. This fixes inconsistent behavior where single trailing positional values failed with “expected sequence start” while multiple values succeeded.

- propagate `is_multiple` through positional, long/short flag, and parent adoption paths
- ensure first occurrence of multi-valued args is stored as a 1-item array
- keep existing subcommand/sequence behavior intact
- verifies with subcommand + flattened payload integration tests for single and multiple positional scalars
Copilot AI review requested due to automatic review settings February 12, 2026 22:59

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a CLI parsing edge case where positional Vec<T> fields inside a flattened subcommand payload would fail when exactly one item was provided (issue #69).

Changes:

  • Add coverage for flattened-subcommand positional Vec<String> parsing with single vs multiple tokens.
  • Teach the CLI layer to insert an array on first assignment when the target arg schema is multiple (so Vec<T> deserializes correctly even with a single value).
  • Propagate multiple metadata through parent-flag (“adoption agency”) lookups and flag parsing helpers.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
crates/figue/tests/integration/sequence.rs Adds regression tests for flattened subcommand positional Vec<String> (single + multiple tokens).
crates/figue/src/layers/cli.rs Adds is_multiple tracking and ensures single occurrences of multiple args are stored as arrays.
Comments suppressed due to low confidence (1)

crates/figue/src/layers/cli.rs:983

  • insert_value_to now takes is_multiple, but the occupied-entry path still always accumulates into an array even when is_multiple is false. That means repeating a non-multiple arg/flag (e.g. --port 1 --port 2) will silently produce a ConfigValue::Array and later fail deserialization with a confusing "expected scalar"-style error. Consider using is_multiple here to either (a) overwrite the previous value (last-one-wins) or (b) emit a dedicated diagnostic for duplicate non-multiple args, and only accumulate when is_multiple is true.
            indexmap::map::Entry::Occupied(mut entry) => {
                // Accumulate into array for repeated flags
                let existing = entry.get_mut();
                if let ConfigValue::Array(arr) = existing {
                    arr.value.push(value);
                } else {
                    // Convert to array with both values
                    let placeholder = ConfigValue::Null(Sourced {
                        value: (),
                        span: None,
                        provenance: None,
                    });
                    let old = core::mem::replace(existing, placeholder);
                    *existing = ConfigValue::Array(Sourced {
                        value: vec![old, value],
                        span: None,
                        provenance: None,
                    });
                }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…de struct and passing it into parse_flag_value_simple.
Use last-value-wins for repeated non-multiple args in CLI insertion logic, while preserving accumulation for multiple fields. Add an integration regression test for repeated --port to ensure non-multiple named args stay scalar and parse cleanly.
@TeamDman

Copy link
Copy Markdown
Author

Existing behaviour

#[test]
fn test_duplicate_non_multiple_named_last_value_wins() {
    #[derive(Facet, Debug)]
    struct Args {
        #[facet(args::named)]
        port: u16,
    }

    let args: Args = figue::from_slice(&["--port", "1", "--port", "2"]).unwrap();
    assert_eq!(args.port, 2);
}

it is an error when a non-array parameter is specified multiple times, which aligns with my expectations

❯ cargo test -p figue --test main test_duplicate_non_multiple_named_last_value_wins -- --nocapture
    Blocking waiting for file lock on build directory
   Compiling figue v1.0.0 (G:\Programming\Repos\figue\crates\figue)
    Finished `test` profile [unoptimized + debuginfo] target(s) in 9.42s
     Running tests\main.rs (target\debug\deps\main-8104c8a25b6a3ae2.exe)

running 1 test
Error: unexpected token: got sequence start, expected scalar value at port      
   ╭─[ <cli>:1:8 ]

running 1 test
Error: unexpected token: got sequence start, expected scalar value at port
   ╭─[ <cli>:1:8 ]
   │
 1 │ --port 1 --port 2
   │        ┬
   │        ╰── unexpected token: got sequence start, expected scalar value at port
───╯

error: test failed, to rerun pass `-p figue --test main`

updating my changes to ensure this remains the case

When a non-multiple named argument is provided more than once, report an explicit duplicate-argument diagnostic instead of coercing values into a sequence or relying on deserialization errors. Keep multi-value argument accumulation unchanged and add an integration test for repeated --port.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread crates/figue/src/layers/cli.rs
Comment thread crates/figue/src/layers/cli.rs
Report duplicate non-multiple argument errors at a stable span covering the repeated flag and its value (rather than the current token pointer). Update integration coverage to assert the duplicate --port diagnostic anchors at the second flag.
Keep span/provenance on ConfigValue::Array wrappers when inserting multiple args, including scalar-to-array rewrites and appends. This improves provenance tracking and keeps diagnostics anchored to meaningful CLI spans.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@TeamDman TeamDman closed this Mar 5, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants