-
Notifications
You must be signed in to change notification settings - Fork 831
Fix incorrect open statement placement for modules with multiline attributes
#19066
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…ttributes Co-authored-by: T-Gro <46543583+T-Gro@users.noreply.github.com>
Co-authored-by: T-Gro <46543583+T-Gro@users.noreply.github.com>
open statement placement for modules with multiline attributes
|
@copilot : ////////1. Plain crashing Failed FSharp.Editor.Tests.CodeFixes.AddOpenOnTopOnTests.Fixes FS0039 for missing opens - multiple first lines are empty [583 ms] ////////2. Missed expectations: Actual: Some({ Message = "open System" Stack Trace: |
Co-authored-by: T-Gro <46543583+T-Gro@users.noreply.github.com>
Fixed in d8f4a1a. The initial implementation only handled nested modules with attributes, but not top-level modules. Updated |
|
@copilot : Another failure. notice bad placement of the open. Failed FSharp.Editor.Tests.CodeFixes.AddOpenOnTopOnTests.Fixes FS0039 for missing opens - explicit namespace [266 ms] Actual: Some({ Message = "open System" Stack Trace: |
Co-authored-by: T-Gro <46543583+T-Gro@users.noreply.github.com>
Fixed in cd2861c. The namespace case requires using |
❗ Release notes required
Warning No PR link found in some release notes, please consider adding it.
|
❗ Release notes requiredCaution No release notes found for the changed paths (see table below). Please make sure to add an entry with an informative description of the change as well as link to this pull request, issue and language suggestion if applicable. Release notes for this repository are based on Keep A Changelog format. The following format is recommended for this repository:
If you believe that release notes are not necessary for this PR, please add NO_RELEASE_NOTES label to the pull request. You can open this PR in browser to add release notes: open in github.dev
|
|
@copilot You forgot to add release notes. vsintegration/src |
Co-authored-by: T-Gro <46543583+T-Gro@users.noreply.github.com>
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
❌ Build / Test Failures💥 Issues
🏗️ Jobs
🧪 Tests (1)
|
…TopLevel Co-authored-by: T-Gro <46543583+T-Gro@users.noreply.github.com>
Fixed test expectation in f2d08ad. The test in |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
Fix incorrect
openstatement placement for modules with multiline attributesPlan
startLine - 1behaviorChanges Made
walkSynModuleDeclto usetrivia.ModuleKeyword.StartLinefor nested moduleswalkSynModuleOrNamespaceto usetrivia.LeadingKeywordrange for top-level modules and namespacesopenat namespace level (grouping with existing opens) whenAlwaysPlaceOpensAtTopLevel = trueFix Details
The initial fix only handled nested modules, causing test failures. Second iteration fixed top-level modules but broke namespace handling. The namespace case requires
startLine - 1instead of using the keyword line directly. Test expectation was corrected to match the behavior ofAlwaysPlaceOpensAtTopLevel = true, which places opens at the namespace level when there are existing opens there (grouping behavior).Fixes #18671
Original prompt
Fix incorrect
openstatement placement for modules with multiline attributesProblem
When using the "Add open" code fix on modules with multiline attributes, the
openstatement is incorrectly inserted inside the attribute block instead of after the module declaration.Current Buggy Behavior:
Expected Behavior:
Root Cause
The compiler service in
src/Compiler/Service/ServiceParsedInputOps.fsreturns incorrect insertion context for modules with multiline attributes. TheAddOpenCodeFixProviderhas a workaround hack to patch this, but it only handles single-line attributes.Solution - Two-Part Fix
Part 1: Fix Root Cause in Compiler Service
File:
src/Compiler/Service/ServiceParsedInputOps.fsLocation: Around line 2461 in the
walkSynModuleDeclfunctionCurrent Code:
Replace With:
Explanation: The issue is that
range.StartLineincludes attributes. By usingtrivia.ModuleKeyword.StartLine, we get the exact line where themodulekeyword appears, skipping all attributes regardless of how many lines they span.Part 2: Simplify AddOpenCodeFixProvider (Remove Workaround)
File:
vsintegration/src/FSharp.Editor/CodeFixes/AddOpenCodeFixProvider.fsLocation: Lines 33-88, the
getOpenDeclarationfunctionCurrent Code (with hack comments):