Fix open declaration insertion in .fsx scripts after #r/#load directives (#16271)#19879
Fix open declaration insertion in .fsx scripts after #r/#load directives (#16271)#19879T-Gro wants to merge 15 commits into
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…aration (issue #16271) 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
|
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
T-Gro
left a comment
There was a problem hiding this comment.
Review of PR #19879 — Fix open declaration insertion in .fsx scripts
Overall the approach is sound: intercept after the normal insertion-point logic, scan for leading hash directives, and bump the position past them. A few issues to address:
Bug: Missing .fsscript extension handling
The PR only checks:
parsedInput.FileName.EndsWith(".fsx", StringComparison.OrdinalIgnoreCase)But F# scripts can also use the .fsscript extension (see CompilerConfig.fs:44 — FSharpScriptFileSuffixes = [".fsscript"; ".fsx"]). The fix should either:
- Also check
.fsscript, or - Reuse the existing
IsScript/FSharpScriptFileSuffixesinfrastructure (preferred for consistency)
Questionable elif branch (untested)
elif lastHashLine = 0 && isAnonModule && ctx.Pos.Line > 1 then
{ ScopeKind = ScopeKind.TopModule; Pos = mkPos 1 0 }This forces insertion at line 1 for .fsx files without hash directives when the normal logic picks a later line. But none of the tests exercise this branch (the test No r directives in fsx inserts at top has ctx.Pos.Line = 1 already, so it goes through else). This branch could interfere with correct open-grouping behavior when existing opens appear below a header comment:
// My script header
open System
let x = System.IO.File.ReadAllText "a"Here the normal logic would correctly group the new open with the existing open System (line 2), but this branch would override it to line 1 (before the comment). Please either add a test proving this branch is needed, or remove it.
Test precision
The core test assertions use Assert.True(insertionPoint.Line >= 3) which is quite loose. If the logic were to (incorrectly) return line 100, the test would still pass. Consider using exact equality or at least a tighter range:
Assert.Equal(3, insertionPoint.Line) // Exactly after the last #r on line 2Minor: Release notes in 9.0.300.md
This PR targets main (which maps to 11.0.100). The entry in 9.0.300.md should probably only be added when/if this is backported to that servicing branch, unless repo conventions say otherwise.
The main fix logic is correct and well-motivated. Addressing the .fsscript gap and clarifying the elif branch would complete this nicely.
…recise assertions - Use ParsedImplFileInput.IsScript instead of .fsx extension check, supporting both .fsx and .fsscript (FSharpScriptFileSuffixes) - Remove untested elif branch that could interfere with open-grouping - Use Assert.Equal for exact insertion point assertions - Add test for .fsscript extension - Remove release notes from 9.0.300.md (PR targets main/11.0.100) - Update 11.0.100.md to mention .fsscript Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The elif branch (lastHashLine = 0 && ctx.Pos.Line > 1) is needed to ensure opens in .fsx scripts without #r/#load directives are placed at the top (line 1) rather than being grouped near code usage. The existing test 'No r directives in fsx inserts at top' exercises this branch. Added a multiline variant to further prove coverage. Removed the isAnonModule check since IsScript already implies this. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The elif branch (no hash directives, script file) previously forced insertion to line 1 unconditionally when ctx.Pos.Line > 1. This broke open-grouping when existing opens appear below a header comment. Now the branch only fires when there are no existing open declarations, preserving correct grouping behavior while still ensuring bare scripts without opens/hash-directives get insertions at line 1. Added test exercising the reviewer's scenario (header comment + open System). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…ed-top branch - Only #r/#load (not #I/#time/#help/#nowarn) drive open placement in scripts (abonie). - Remove the elif that forced line 1; it broke 'module Foo' scripts and relied on testing FindNearest without AdjustInsertionPoint. Rely on AdjustInsertionPoint and clamp to max(lastHashLine+1, ctx.Pos.Line) under HashDirective scope so opens never move above the directives. - Tests assert the real post-AdjustInsertionPoint line; add regression tests for named module, non-reference directives, and #I-before-#r. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Slow-review found that the script open-insertion fix matched only #r/#load, not #reference (the documented long-form alias of #r). For #reference the scan never matched, so the open fell back to the top-of-file path and was placed above the directive -> invalid script (FS0039 at dotnet fsi). Add "reference" to the directive check and a regression test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Compact FindNearestPointToInsertOpenDeclaration: bind the script impl once and use impl.Contents directly instead of re-matching parsedInput; clearer names (isReferenceDirective / lastLeadingReferenceEndLine / lastReferenceLine). - Add tests for behaviors verified during review: directives separated by comments/blank lines, a non-reference directive (#I) interleaved between references, and CRLF endings. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace the hand-rolled recursion with List.takeWhile (leading hash directives) + List.choose + List.fold max, mirroring the isDefHash/takeWhile idiom in fsi.fs. Fold the directive-ident set into one local predicate and trim the comments to the two non-obvious points (the ordering rule and the HashDirective/AdjustInsertionPoint pass-through). No behavioural change; 15/15 tests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Duck-review (Nearest insertion + nested module) found that forcing column 0 and the HashDirective scope unconditionally would push a grouped open to column 0 inside an indented module, breaking its offside (FS0058). main places it correctly. Only remap the TopModule scope (which AdjustInsertionPoint would otherwise snap to line 1, above the directives) to the pass-through HashDirective scope; keep the original scope and column for the other scopes, just bumping the line past the directives. Add a Nearest nested-module regression test; the helper now also returns the column. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Description
Fixes #16271
In .fsx scripts, #r and #load directives must precede any open declarations. Previously, the \FindNearestPointToInsertOpenDeclaration\ logic did not account for this constraint and could suggest inserting an \open\ before existing #r/#load\ directives, resulting in invalid script files.
Changes
Testing