Parser: Update OBLOCKSEP token passing #18921

edgarfgp · 2025-09-19T14:08:43Z

Description

Continuation of #18899

Checklist

Test cases added
Release notes entry updated

github-actions · 2025-09-19T14:09:28Z

⚠️ Release notes required, but author opted out

Warning

Author opted out of release notes, check is disabled for this pull request.
cc @dotnet/fsharp-team-msft

auduchinok

There are many incorrect nodes in the test tree dumps. @edgarfgp Could you please fix the implementation, so the tests produce correct data?

tests/service/data/SyntaxTree/Pattern/Named field 05.fs.bsl

tests/service/data/SyntaxTree/Expression/Record - Anon 12.fs.bsl

tests/service/data/SyntaxTree/Expression/Record - Anon 06.fs.bsl

src/Compiler/pars.fsy

src/Compiler/SyntaxTree/SyntaxTree.fs

… into capture-block-separators

auduchinok

This is really good now. It's now possible to rely on the separator info when processing the tree, and all the cases with fake tree nodes are cleared up. Thanks @edgarfgp!

src/Compiler/pars.fsy

src/Compiler/Service/ServiceParseTreeWalk.fs

edgarfgp · 2025-09-21T16:42:21Z

vsintegration/tests/UnitTests/LegacyLanguageService/Tests.LanguageService.Completion.fs

                "let r = {AAA = 5}"
                "let b = {r  with }"
            ]
-        AssertCtrlSpaceCompleteContains code "{r " [] ["AAA"]


Humm. This will enabled completion before the with keyword. To fix this we can do some range calculations in ServiceParseTreeWalk.fs and update the completion context for records or capture the with range in the tree and then use it.

auduchinok · 2025-09-22T08:22:29Z

src/Compiler/pars.fsy

-       (Some($1, BlockSeparator.Offside(rhs parseState 2, None)), l) }
+       (Some($1, $5), l) }


This change looks suspicious. Previously we would capture the with range and store it as a separator. That didn't look correct because of how the separator field was reused for a different thing, but it could be the place where the code completion relied on. We can add a new field to the trivia and store the with range there.

This is where we've started assuming that with was an offside separator: https://github.com/dotnet/fsharp/pull/18899/files#diff-e4c6a418b5f63918e99d3b27f209771a966fb6c7dbadf25cf5d3bb4b7b7d7bd7L5746 and then in this PR we've stopped keeping all non-semi and non-comma ones. A simpler possible fix is to rollback the copyInfo changes in the records and keep a with range there to use it from the code completion.

Why is this comment marked as resolved? We still capture a different thing here, compared to pre-#18899, don't we?

Yes, everything that was BlockSeparator.Offside now should be None.

But isn't it exactly the thing that the code completion relied on?

It clearly wasn't a separator in the new terminology but it was named that way before this terminology was introduced in the tree. It captured the with keyword range and other places could use it, so it was a different kind of separator between the expression and the field updates, and that's where the name came from.

It just was a mistake to start storing with range as if it was a semicolon/comma in the recent PRs, which happened because of the unfortunate naming clash.

So your point is that we should not capture this as separator at all in this case ?

My point is it seems it was captured for a reason before and now it's lost. Even though it was also called a separator, it served a different purpose: it wasn't an offside separator, but the range of with keyword (that now we can store in the record expression trivia), and it seems it's exactly what the code completion could rely on.

auduchinok · 2025-09-22T08:39:34Z

tests/FSharp.Compiler.Service.Tests/EditorTests.fs

+let notHasRecordField (fieldName:string) (symbolUses: FSharpSymbolUse list) =
+    symbolUses
+    |> List.exists (fun symbolUse ->
+        match symbolUse.Symbol with
+        | :? FSharpField as field -> field.DisplayName = fieldName
+        | _ -> false
+    )
+    |> fun exists -> Assert.False(exists, $"Field {fieldName} should not be found.")


Please use hasRecordField and not.

src/Compiler/pars.fsy

src/Compiler/Service/ServiceParsedInputOps.fs

auduchinok · 2025-09-22T09:19:06Z

src/Compiler/Checking/CheckRecordSyntaxHelpers.fs

+        let rangeOfBlockSeparator (id: Ident) =
+            let idEnd = id.idRange.End
+            let blockSeparatorStartCol = idEnd.Column
+            let blockSeparatorEndCol = blockSeparatorStartCol + 4


Where does 4 magic constant come from? If we assume that a separator is ; or , then it can't be 4 characters long.

If this is a with keyword range, it should not be calculated from the id, it's job of the parser to preserve it correctly, like it was before.

This was how it was originally. See https://github.com/dotnet/fsharp/pull/18899/files#diff-1e790bba80026c421a964cb1d8be0297aff37a0a3dbe7cbf954b54f6e3544e40L93-L100. But I think in this case we do not have a separator we can use range0 ?

auduchinok · 2025-09-24T07:25:14Z

src/Compiler/pars.fsy

     { let l = List.rev $4
       let l = rebindRanges $3 l $5
-       (Some($1, $5), l) }
+       (Some($1, None), l) }


The captured info is still different here.

No. this should be None as previously was BlockSeparator.Offside

… into capture-block-separators

auduchinok · 2025-09-25T09:52:31Z

src/Compiler/SyntaxTree/SyntaxTree.fs

-        copyInfo: (SynExpr * BlockSeparator) option *
+        copyInfo: (SynExpr * BlockSeparator option) option *


This place still looks very wrong. We've stored the with keyword range before #18899 here, and now we store an optional semicolon or comma. Why do we expect a ;/, separator instead of the with in the tree? Where do we expect them to come from? Do we allow some new syntax? Or is it just dead code now? Why don't we update all the service code accordingly and try to process semicolons in places where they can never be at all where previously we processed with keywords?

auduchinok · 2025-09-25T10:50:37Z

This PR seems to add lots of new logic on top of mistakes made in #18899, most importantly treating with as if it was a an implicit semicolon, and with each commit we only diverge more from the original implementation.

The previous implementation would call with a separator in record expressions and capture its range, so the tooling could use it. With the current implementation only ; and , are considered separators. The problem is the old code which checked with range in record expressions now ignores with completely and tries to process semicolons that can't even be there. This leads us to regressions in features like code completion.

@edgarfgp @T-Gro I propose we revert #18899, so this work could be started from scratch with better understanding of what the original code did instead of covering the mistakes with more new code. I believe that would allow us to have less diff as well. We can also use blame.ignoreRevsFile to hide #18899 and the revert from git blame

If not reverting, let's make a fresh branch starting from pre-#18899 and modeling the tree with the minimal diff, clearly stating what we consider a separator and updating the service code accordingly, and only then process with subsequent changes.

edgarfgp · 2025-09-25T12:17:16Z

@auduchinok Yeah let’s revert the previous PR and leave things as were before. Feel free to raise a new PR with your suggested changes.

github-project-automation bot added this to F# Compiler and Tooling Sep 19, 2025

github-project-automation bot moved this to New in F# Compiler and Tooling Sep 19, 2025

edgarfgp requested a review from auduchinok September 19, 2025 14:09

edgarfgp added the NO_RELEASE_NOTES Label for pull requests which signals, that user opted-out of providing release notes label Sep 19, 2025

auduchinok suggested changes Sep 19, 2025

View reviewed changes

github-project-automation bot moved this from New to In Progress in F# Compiler and Tooling Sep 19, 2025

Parser: Update OBLOCKSEP token passing

9f9c0ec

edgarfgp force-pushed the capture-block-separators branch from 6eea60c to 9f9c0ec Compare September 19, 2025 21:13

edgarfgp added 7 commits September 19, 2025 23:57

baselines

8347ba2

Reduce diff

3e1a3c7

Merge branch 'main' into capture-block-separators

02d2984

Reduce diff

9139f3e

Merge branch 'capture-block-separators' of github.com:edgarfgp/fsharp…

489cf4f

… into capture-block-separators

reduce diff

74db5c8

Remove position from BlockSeparator

fcc7b41

edgarfgp force-pushed the capture-block-separators branch from a2f929b to fcc7b41 Compare September 20, 2025 14:18

auduchinok reviewed Sep 20, 2025

View reviewed changes

src/Compiler/pars.fsy Outdated Show resolved Hide resolved

src/Compiler/pars.fsy Outdated Show resolved Hide resolved

src/Compiler/Service/ServiceParseTreeWalk.fs Outdated Show resolved Hide resolved

src/Compiler/Service/ServiceParseTreeWalk.fs Outdated Show resolved Hide resolved

edgarfgp added 4 commits September 20, 2025 20:10

Use a more accurate separator range

ae13b59

Use nested match

c9be8b7

extracting common logic in diveIntoRecordLikeExpr

fb776ab

Fix editor test

8828dad

edgarfgp force-pushed the capture-block-separators branch 2 times, most recently from d19dc1c to 8315a15 Compare September 21, 2025 16:37

edgarfgp commented Sep 21, 2025

View reviewed changes

edgarfgp force-pushed the capture-block-separators branch from 8315a15 to 7a291f2 Compare September 22, 2025 08:36

auduchinok reviewed Sep 22, 2025

View reviewed changes

fix test: Disallow completion before with keyword

aa7bd57

edgarfgp force-pushed the capture-block-separators branch from 7a291f2 to aa7bd57 Compare September 23, 2025 07:40

edgarfgp added 2 commits September 23, 2025 16:51

Update pars.fsy

04d0c5b

rollback the behavior to pre-dotnet#18899

ffdaa6e

edgarfgp force-pushed the capture-block-separators branch from acefecd to ffdaa6e Compare September 23, 2025 21:11

auduchinok reviewed Sep 24, 2025

View reviewed changes

edgarfgp added 5 commits September 24, 2025 15:16

Merge branch 'main' into capture-block-separators

7d89c7b

Avoid capturing {x with} as separator

37b3cf1

Merge branch 'capture-block-separators' of github.com:edgarfgp/fsharp…

d138a23

… into capture-block-separators

capture with keyword using trivia

1ff5bed

format

53809e7

auduchinok suggested changes Sep 25, 2025

View reviewed changes

edgarfgp closed this Sep 25, 2025

github-project-automation bot moved this from In Progress to Done in F# Compiler and Tooling Sep 25, 2025

		(Some($1, BlockSeparator.Offside(rhs parseState 2, None)), l) }
		(Some($1, $5), l) }

		copyInfo: (SynExpr * BlockSeparator) option *
		copyInfo: (SynExpr * BlockSeparator option) option *

Parser: Update OBLOCKSEP token passing #18921

Parser: Update OBLOCKSEP token passing #18921

Uh oh!

Conversation

edgarfgp commented Sep 19, 2025

Description

Checklist

Uh oh!

github-actions bot commented Sep 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ Release notes required, but author opted out

Uh oh!

auduchinok left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

auduchinok left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

auduchinok Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edgarfgp Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

auduchinok Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

auduchinok Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

auduchinok commented Sep 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

edgarfgp commented Sep 25, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

github-actions bot commented Sep 19, 2025 •

edited

Loading

auduchinok left a comment •

edited

Loading

auduchinok Sep 23, 2025 •

edited

Loading

edgarfgp Sep 24, 2025 •

edited

Loading

auduchinok Sep 24, 2025 •

edited

Loading

auduchinok Sep 24, 2025 •

edited

Loading

auduchinok commented Sep 25, 2025 •

edited

Loading