Uh oh!
There was an error while loading. Please reload this page.
Stop silently ignoring --arg-file-path for Option<T> contract parameters - #2680
Stop silently ignoring --arg-file-path for Option<T> contract parameters#2680Galmanus wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes argument parsing for Option<T> contract inputs so an explicitly provided --<arg>-file-path is honored (instead of falling back to None/Void), and adds regression tests for the behavior.
Changes:
- Reorders
parse_single_argumentbranches to prioritize--<arg>-file-pathover theOption<T>fallback. - Adds regression tests covering optional + required args read from file path and optional omission defaulting to
Void.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| let mut file = tempfile::NamedTempFile::new().unwrap(); | ||
| write!(file, "127").unwrap(); | ||
| let path = file.path().to_str().unwrap().to_string(); |
There was a problem hiding this comment.
Thanks for flagging this, but I don't think these tests are flaky. NamedTempFile delegates its Write impl to a bare std::fs::File, which is unbuffered in userspace (there's no BufWriter here) — write! goes straight through write_all/write(2) to the kernel, and write!(...).unwrap() only returns Ok once every byte has been accepted by the kernel. parse_file_argument then reads the same path back with std::fs::read_to_string in the same process, so the read is served from the page cache and sees the full contents (standard POSIX read-after-write coherency). No userspace buffer sits between the two.
For the same reason, adding file.flush() wouldn't change anything: File's flush is a no-op (there's nothing to flush). sync_all() would force durability to physical disk, but that only matters for crash-safety, not for reading the bytes back within the running test. The buffering concern would apply if the write went through a BufWriter, which isn't the case here. I'm happy to add an explicit flush if the team wants it as documentation of intent, but functionally it's a no-op, so I'd lean toward keeping the tests as-is.
In `parse_single_argument` the fallback order was: direct arg -> `Option<T>` => `Void` -> `--<arg>-file-path`. For optional parameters the `Void` branch always won, so an explicitly provided `--<arg>-file-path` was silently discarded and the function invoked with `None`. Reorder the fallbacks to check the file arg before the `Option` default. `--<arg>` and `--<arg>-file-path` remain mutually exclusive via clap's `conflicts_with`, so no new ambiguity is introduced. Fixesstellar#2432
2eed32d to
c663dc6Compare| } else if matches!(input.type_, ScSpecTypeDef::Option(_)) { | ||
| parsed_args.push(ScVal::Void); | ||
| Ok(()) | ||
| } else if let Some(arg_path) = matches_.get_one::<PathBuf>(&fmt_arg_file_name(&name)) { |
Fixes#2432.
In
parse_single_argumentthe fallback order was: direct arg →Option<T>⇒Void→--<arg>-file-path. For optional parameters theVoidbranch always won, so an explicitly provided--<arg>-file-pathwas silently discarded and the function invoked withNone.Silently dropping an explicitly-provided argument is the worst failure mode here: the invocation still succeeds, just with the wrong value.
Fix
Reorder the fallbacks: direct arg → file path →
Option<T>⇒Void→ missing-argument error.--<arg>and--<arg>-file-pathremain mutually exclusive via clap'sconflicts_with, so no new ambiguity is introduced, and the behavior for required parameters and for omitted optional parameters is unchanged.Tests
optional_arg_reads_value_from_file_path— failed withleft: [Void], right: [U32(127)]before the fix.optional_arg_omitted_defaults_to_void— omitted optional still yieldsVoid.required_arg_reads_value_from_file_path— required-param file path unchanged.