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 9 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). |
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