-
Notifications
You must be signed in to change notification settings - Fork 826
Refactor #line processing - keep the original positions in the AST #18699
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
❗ Release notes required
|
This is ready for review. See updated PR description. |
@T-Gro |
Sorry for not getting around to responding sooner. We probably do with FSAC QuickFixes or possibly some analyzers but with all changes to FCS, we adapt. It might be enlightening to see what tests fail with a build of this against FSAC or FSharp.Analyzers.SDK/Ionide-Analyzers/GResearch's analyzers. |
Good to hear. Thanks for looking into it. I will try to run the tests and report back. |
Here is what I found out. Good news is that goto-symbol is working correctly with the FCS of this PR and the fix in FSAC's I also ran
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors #line directive processing to fix issue #18553. It changes the system to keep original positions in the AST while storing line directives separately, providing a new ApplyLineDirectives()
method for when transformed positions are needed.
Key changes:
- Original ranges are preserved in the AST, with line directives stored in a separate table
- The
__LINE__
and__SOURCE_FILE__
intrinsics now show original positions instead of transformed ones - Debug information and diagnostics continue to use transformed positions via
ApplyLineDirectives()
Reviewed Changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
tests/fsharpqa/Source/Conformance/SpecialAttributesAndTypes/Imported/CallerInfo/*.fs | Update test expectations to reflect that CallerLineNumber now uses original line numbers |
tests/fsharp/core/syntax/test.fsx | Remove extensive line directive tests that no longer apply to the new behavior |
tests/ILVerify/*.bsl | Update IL verification baselines to reflect line number changes in generated code |
tests/FSharp.Compiler.Service.Tests/ProjectAnalysisTests.fs | Update test to use ApplyLineDirectives() for transformed ranges |
tests/FSharp.Compiler.Service.Tests/*.bsl | Add ApplyLineDirectives method to surface area |
tests/FSharp.Compiler.ComponentTests/CompilerDirectives/*.fs | Update/add tests for new line directive behavior |
src/Compiler/lex.fsl | Replace WarnScopes.RegisterLineDirective with LineDirectiveStore.SaveLineDirective |
src/Compiler/Utilities/range.* | Add ApplyLineDirectives method and LineDirectives module for storing directives |
src/Compiler/SyntaxTree/WarnScopes.* | Remove line directive tracking and mapping logic |
src/Compiler/SyntaxTree/*.fs | Add LineDirectiveStore module and update position handling |
src/Compiler/Facilities/prim-lexing.* | Remove OriginalLine and ApplyLineDirective from Position type |
src/Compiler/Driver/*.fs | Update to call ApplyLineDirectives for diagnostics and add line directive storage |
src/Compiler/Checking/*.fs | Apply line directives for debug info and runtime diagnostics |
docs/release-notes/ | Document the breaking change |
Description
Fixes #18553.
Breaking change.
This PR is non-breaking for two of the three #line use cases mentioned in #18553.
It keeps the original ranges in the AST, stores the #line directives in a table and provides a method
range.ApplyLineDirectives()
. This method is applied in the following placesCompilerDiagnostics.FormatDiagnosticLocation
)FSharpDiagnostics.CreateFromException
)This should make sure that all debugging information (in .pdb) and all diagnostics stay unchanged.
The third use case, symbol positions for IDE editors, is more tricky.
FSharpSymbol
has no range field or property that I could apply the transformation to, but just contains (and makes public) the AST items with their original ranges.So, I propose we leave it like this and call
ApplyLineDirectives
in the appropriate places in FSAC / the editors.This will also enable better line directive support in the editors (for some use cases you need the original ranges).
For Ionide / FSAC I found that calling
ApplyLineDirectives
in a single place (in FSAC'sfcsRangeToLspLocation
) will most probably be sufficient to support the current functionality.I don't know what it means for the other editors.
Any thoughts or recommendations here? (@TheAngryByrd @auduchinok @abonie )
List of breaking changes
__LINE__
and__SOURCE_FILE__
show original locations nowFSharpSymbol
points to its original range)Checklist