Stop FSI from mutating script arguments after -- separator#19926
Conversation
RED phase: pin down the correct behaviour of fsi.CommandLineArgs when user arguments follow the -- separator. The abbreviated flags -d, -r, -I are currently colon-joined with their next token (e.g. '-- -d 5' becomes '-d:5' instead of staying as two tokens), which these tests assert against. Theory rows fail on current main; the baseline [<Fact>] passes and locks in the current treatment of non-abbreviated post-'--' args so the GREEN fix cannot regress them. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
#10819) Split argv at the first `--` and only post-process the compiler-args prefix; the suffix (including the `--` separator itself) reaches downstream handlers byte-for-byte. Keeping `--` in the suffix preserves two paths: * with a script preceding `--`, the script's OptionGeneral IsScript handler captures the suffix as script args (matching pre-fix behaviour for fsi.CommandLineArgs); * with no script (e.g. `dotnet fsi -- -d 5`), the `--` token reaches ParseCompilerOptions and fires its OptionRest recordExplicitArg handler so `-d` and `5` are recorded as explicit args instead of being parsed as compiler options. Updates the regression test theory to expect `--` in the captured tail and adds a new no-script Fact that locks the OptionRest path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Fantomas-applied formatting in src/Compiler/Interactive/fsi.fs (whitespace and pattern-match line collapse only; no behavioural change) and a single new bullet under ### Fixed in docs/release-notes/.FSharp.Compiler.Service/11.0.100.md referencing Issue #10819. Validation: focused regression filters (FsiCommandLineArgsTests, CommandLineArgs, FsiCliTests) green under -c Release on both net10.0 and net48. SurfaceAreaTest passes on net10.0 with no baseline update (net48 has zero tests for that filter - platform-conditional). Full FSharp.Compiler.ComponentTests and FSharp.Compiler.Service.Tests suites were run under -c Release as the documented alternative to './build.sh -c Release --testcoreclr'; the only failures observed were two well-known flaky ScriptOptionsTests nuget-resolution tests on net48 (unrelated to argument handling; confirmed flaky in isolation; net10.0 runs all 4364 tests green). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
❗ Release notes requiredYou can open this PR in browser to add release notes: open in github.dev
|
github-actions
Bot
added
the
AI-Tooling-Check-Bypassed
| // otherwise fsi.CommandLineArgs leaks the rewrite to scripts (see #10819). | ||
| // Split argv at the first `--`, post-process only the compiler-args prefix, | ||
| // and pass the suffix (including the `--` separator itself) through unmodified. | ||
| // Keeping `--` in the suffix preserves both downstream handlers: |
There was a problem hiding this comment.
A lot of this comment's contents is coupled to code that's not even in this file. In case of future changes downstream, it's very unlikely this comment gets updated as well. At least trim it down. The useful part of this comment is its first ~3 lines
There was a problem hiding this comment.
Trimmed the comment down to the self-contained lines (the rewrite rationale + what this split does), and removed the part describing the downstream OptionGeneral/OptionRest handlers that lived in other files. Pushed in 6e4d11f.
github-project-automation
Bot
moved this from New
to In Progress
in F# Compiler and Tooling
T-Gro
left a comment
T-Gro
left a comment
There was a problem hiding this comment.
🤖 This review was generated by AI (@expert-reviewer agent). Findings may contain inaccuracies — please verify independently.
Reviewed the fix for #10819. The core change is correct: PostProcessCompilerArgs (which colon-joins abbreviated flags like -d 5 → -d:5) is now applied only to the compiler-args prefix before the first --, while the suffix (including --) passes through unmodified. I verified the relevant edge cases — the no--- path preserves prior behaviour, only the first -- is treated as the separator, the boundary abbreviated flag no longer joins with --, and List.tail processedArgs stays safe because processedArgs is always non-empty. The added tests exercise both the script-present (IsScript/OptionGeneral) and no-script (OptionRest) handlers. No significant correctness, security, or performance issues found. One non-blocking API note inline.
| | Some idx -> | ||
| let prefix = argv[0 .. idx - 1] | ||
| let suffix = argv[idx..] | ||
| PostProcessCompilerArgs abbrevArgs prefix @ List.ofArray suffix |
There was a problem hiding this comment.
Non-blocking confirmation: this intentionally keeps the -- token itself in fsi.CommandLineArgs (the tests assert --,-d,5). The PR rationale is to match pre-fix behaviour, which is a reasonable, minimal-scope choice. Worth flagging that some scripts may expect -- to be stripped as a pure separator (as many CLI conventions do), so if a future change ever revisits this, it would be an observable behaviour shift for consumers of fsi.CommandLineArgs. No change requested here — just documenting the contract decision.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
2 participants
Fixes #10819
fsi.CommandLineArgswas colon-joining abbreviated flags (-d,-r,-I)with their next token when they appeared after
--. NowPostProcessCompilerArgssplits at the first
--and only post-processes the compiler-args prefix.