Skip to content

Parser: Update OBLOCKSEP token passing #18921

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.

Sign up for GitHub

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

Closed
edgarfgp wants to merge 20 commits into from
Closed

Parser: Update OBLOCKSEP token passing #18921

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
9f9c0ec
Parser: Update OBLOCKSEP token passing
edgarfgp Sep 19, 2025
8347ba2
baselines
edgarfgp Sep 19, 2025
3e1a3c7
Reduce diff
edgarfgp Sep 19, 2025
02d2984
Merge branch 'main' into capture-block-separators
edgarfgp Sep 19, 2025
9139f3e
Reduce diff
edgarfgp Sep 19, 2025
489cf4f
Merge branch 'capture-block-separators' of github.com:edgarfgp/fsharp…
edgarfgp Sep 20, 2025
74db5c8
reduce diff
edgarfgp Sep 20, 2025
fcc7b41
Remove position from BlockSeparator
edgarfgp Sep 20, 2025
ae13b59
Use a more accurate separator range
edgarfgp Sep 20, 2025
c9be8b7
Use nested match
edgarfgp Sep 20, 2025
fb776ab
extracting common logic in diveIntoRecordLikeExpr
edgarfgp Sep 20, 2025
8828dad
Fix editor test
edgarfgp Sep 20, 2025
aa7bd57
fix test: Disallow completion before with keyword
edgarfgp Sep 21, 2025
04d0c5b
Update pars.fsy
edgarfgp Sep 23, 2025
ffdaa6e
rollback the behavior to pre-#18899
edgarfgp Sep 23, 2025
7d89c7b
Merge branch 'main' into capture-block-separators
edgarfgp Sep 24, 2025
37b3cf1
Avoid capturing {x with} as separator
edgarfgp Sep 24, 2025
d138a23
Merge branch 'capture-block-separators' of github.com:edgarfgp/fsharp…
edgarfgp Sep 24, 2025
1ff5bed
capture with keyword using trivia
edgarfgp Sep 25, 2025
53809e7
format
edgarfgp Sep 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 25 additions & 9 deletions src/Compiler/Checking/CheckRecordSyntaxHelpers.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,14 +90,24 @@ let TransformAstForNestedUpdates (cenv: TcFileState) (env: TcEnv) overallTy (lid
let totalRange (origId: Ident) (id: Ident) =
withStartEnd origId.idRange.End id.idRange.Start origId.idRange

match withExpr with
| SynExpr.Ident origId, (blockSep: BlockSeparator) ->
let lid, rng = upToId blockSep.Range id (origId :: ids)
let rangeOfBlockSeparator (id: Ident) =
let idEnd = id.idRange.End
let blockSeparatorStartCol = idEnd.Column
let blockSeparatorEndCol = blockSeparatorStartCol + 4
Copy link
Member

@auduchinok auduchinok Sep 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@edgarfgp edgarfgp Sep 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

let blockSeparatorStartPos = mkPos idEnd.Line blockSeparatorStartCol
let blockSeparatorEndPos = mkPos idEnd.Line blockSeparatorEndCol

withStartEnd blockSeparatorStartPos blockSeparatorEndPos id.idRange

Some(
SynExpr.LongIdent(false, LongIdentWithDots(lid, rng), None, totalRange origId id),
BlockSeparator.Offside(blockSep.Range, None)
)
match withExpr with
| SynExpr.Ident origId, (blockSep: BlockSeparator option) ->
let mBlockSep =
match blockSep with
| Some sep -> sep.Range
| None -> rangeOfBlockSeparator origId

let lid, rng = upToId mBlockSep id (origId :: ids)
Some(SynExpr.LongIdent(false, LongIdentWithDots(lid, rng), None, totalRange origId id), blockSep)
| _ -> None

let rec synExprRecd copyInfo (outerFieldId: Ident) innerFields exprBeingAssigned =
Expand All @@ -120,7 +130,13 @@ let TransformAstForNestedUpdates (cenv: TcFileState) (env: TcEnv) overallTy (lid
AnonRecdTypeInfo.TupInfo = TupInfo.Const isStruct
}) ->
let fields = [ LongIdentWithDots([ fieldId ], []), None, nestedField ]
SynExpr.AnonRecd(isStruct, copyInfo outerFieldId, fields, outerFieldId.idRange, { OpeningBraceRange = range0 })
// Pass through optional separator for anonymous records
let copyInfoAnon =
match copyInfo outerFieldId with
| Some(exprWhenWith, sepOpt) -> Some(exprWhenWith, sepOpt)
| None -> None

SynExpr.AnonRecd(isStruct, copyInfoAnon, fields, outerFieldId.idRange, { OpeningBraceRange = range0 })
| _ ->
let fields =
[
Expand Down Expand Up @@ -153,7 +169,7 @@ let TransformAstForNestedUpdates (cenv: TcFileState) (env: TcEnv) overallTy (lid

/// When the original expression in copy-and-update is more complex than `{ x with ... }`, like `{ f () with ... }`,
/// we bind it first, so that it's not evaluated multiple times during a nested update
let BindOriginalRecdExpr (withExpr: SynExpr * BlockSeparator) mkRecdExpr =
let BindOriginalRecdExpr (withExpr: SynExpr * BlockSeparator option) mkRecdExpr =
let originalExpr, blockSep = withExpr
let mOrigExprSynth = originalExpr.Range.MakeSynthetic()
let id = mkSynId mOrigExprSynth "bind@"
Expand Down
6 changes: 4 additions & 2 deletions src/Compiler/Checking/CheckRecordSyntaxHelpers.fsi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,10 @@ val TransformAstForNestedUpdates:
overallTy: TType ->
lid: LongIdent ->
exprBeingAssigned: SynExpr ->
withExpr: SynExpr * BlockSeparator ->
withExpr: SynExpr * BlockSeparator option ->
(Ident list * Ident) * SynExpr option

val BindOriginalRecdExpr:
withExpr: SynExpr * BlockSeparator -> mkRecdExpr: ((SynExpr * BlockSeparator) option -> SynExpr) -> SynExpr
withExpr: SynExpr * BlockSeparator option ->
mkRecdExpr: ((SynExpr * BlockSeparator option) option -> SynExpr) ->
SynExpr
32 changes: 15 additions & 17 deletions src/Compiler/Service/ServiceParseTreeWalk.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -445,7 +445,7 @@ module SyntaxTraversal =
| SynExpr.AnonRecd(copyInfo = copyOpt; recordFields = fields) ->
[
match copyOpt with
| Some(expr, blockSep) ->
| Some(expr, Some blockSep) ->
yield dive expr expr.Range traverseSynExpr

yield
Expand All @@ -456,6 +456,9 @@ module SyntaxTraversal =
visitor.VisitRecordField(path, Some expr, None)
else
None)
| Some(expr, None) ->
// No explicit separator (implicit OBLOCKSEP). Still dive into expr.
yield dive expr expr.Range traverseSynExpr
| _ -> ()

for field, _, x in fields do
Expand All @@ -466,19 +469,11 @@ module SyntaxTraversal =

| SynExpr.Record(baseInfo = inheritOpt; copyInfo = copyOpt; recordFields = fields) ->
[
let diveIntoSeparator offsideColumn scPosOpt copyOpt =
match scPosOpt with
| Some scPos ->
if posGeq pos scPos then
visitor.VisitRecordField(path, copyOpt, None) // empty field after the inherits
else
None
| None ->
//semicolon position is not available - use offside rule
if pos.Column = offsideColumn then
visitor.VisitRecordField(path, copyOpt, None) // empty field after the inherits
else
None
let diveIntoSeparator scPos copyOpt =
if posGeq pos scPos then
visitor.VisitRecordField(path, copyOpt, None) // empty field after the inherits
else
None

match inheritOpt with
| Some(_ty, expr, _range, sepOpt, inheritRange) ->
Expand All @@ -505,12 +500,12 @@ module SyntaxTraversal =
// inherit A()
// $
// field1 = 5
diveIntoSeparator inheritRange.StartColumn blockSep.Position None)
diveIntoSeparator blockSep.Range.End None)
| None -> ()
| _ -> ()

match copyOpt with
| Some(expr, blockSep) ->
| Some(expr, Some blockSep) ->
yield dive expr expr.Range traverseSynExpr

yield
Expand All @@ -521,6 +516,9 @@ module SyntaxTraversal =
visitor.VisitRecordField(path, Some expr, None)
else
None)
| Some(expr, None) ->
// No explicit separator (implicit OBLOCKSEP). Still dive into expr.
yield dive expr expr.Range traverseSynExpr
| _ -> ()

let copyOpt = Option.map fst copyOpt
Expand Down Expand Up @@ -563,7 +561,7 @@ module SyntaxTraversal =
// field1 = 5
// $
// field2 = 5
diveIntoSeparator offsideColumn blockSep.Position copyOpt)
diveIntoSeparator blockSep.Range.End copyOpt)
| _ -> ()

]
Expand Down
18 changes: 5 additions & 13 deletions src/Compiler/SyntaxTree/SyntaxTree.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,21 +308,13 @@ type SeqExprOnly = SeqExprOnly of bool

[<NoEquality; NoComparison; RequireQualifiedAccess>]
type BlockSeparator =
| Semicolon of range: range * position: pos option
| Comma of range: range * position: pos option
| Offside of range: range * position: pos option
| Semicolon of range: range
| Comma of range: range

member this.Range =
match this with
| Semicolon(range = m)
| Comma(range = m)
| Offside(range = m) -> m

member this.Position =
match this with
| Semicolon(position = p)
| Comma(position = p)
| Offside(position = p) -> p
| Comma(range = m) -> m

type RecordFieldName = SynLongIdent * bool

Expand Down Expand Up @@ -549,7 +541,7 @@ type SynExpr =

| AnonRecd of
isStruct: bool *
copyInfo: (SynExpr * BlockSeparator) option *
copyInfo: (SynExpr * BlockSeparator option) option *
recordFields: (SynLongIdent * range option * SynExpr) list *
range: range *
trivia: SynExprAnonRecdTrivia
Expand All @@ -558,7 +550,7 @@ type SynExpr =

| Record of
baseInfo: (SynType * SynExpr * range * BlockSeparator option * range) option *
copyInfo: (SynExpr * BlockSeparator) option *
copyInfo: (SynExpr * BlockSeparator option) option *
Comment on lines -561 to +553
Copy link
Member

@auduchinok auduchinok Sep 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

recordFields: SynExprRecordField list *
range: range

Expand Down
17 changes: 4 additions & 13 deletions src/Compiler/SyntaxTree/SyntaxTree.fsi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,22 +359,13 @@ type SeqExprOnly =
type BlockSeparator =
/// A separator consisting of a semicolon ';'
/// range is the range of the semicolon
/// position is the position of the semicolon (if available)
| Semicolon of range: range * position: pos option
| Semicolon of range: range
/// A separator consisting of a comma ','
/// range is the range of the comma
/// position is the position of the comma (if available)
| Comma of range: range * position: pos option

// A separator consisting of a newline
/// range is the range of the newline
/// position is the position of the newline (if available)
| Offside of range: range * position: pos option
| Comma of range: range

member Range: range

member Position: pos option

/// Represents a record field name plus a flag indicating if given record field name is syntactically
/// correct and can be used in name resolution.
type RecordFieldName = SynLongIdent * bool
Expand Down Expand Up @@ -607,7 +598,7 @@ type SynExpr =
/// F# syntax: struct {| id1=e1; ...; idN=eN |}
| AnonRecd of
isStruct: bool *
copyInfo: (SynExpr * BlockSeparator) option *
copyInfo: (SynExpr * BlockSeparator option) option *
recordFields: (SynLongIdent * range option * SynExpr) list *
range: range *
trivia: SynExprAnonRecdTrivia
Expand All @@ -621,7 +612,7 @@ type SynExpr =
/// every field includes range of separator after the field (for tooling)
| Record of
baseInfo: (SynType * SynExpr * range * BlockSeparator option * range) option *
copyInfo: (SynExpr * BlockSeparator) option *
copyInfo: (SynExpr * BlockSeparator option) option *
recordFields: SynExprRecordField list *
range: range

Expand Down
35 changes: 20 additions & 15 deletions src/Compiler/pars.fsy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3668,14 +3668,15 @@ namePatPairs:
{ let (id: Ident), mEq, (pat: SynPat) = $1
let m = unionRanges id.idRange pat.Range
let lid = SynLongIdent([id], [], [None])
NamePatPairField(lid, mEq, m, pat, Some $2) :: $3 }
NamePatPairField(lid, mEq, m, pat, $2) :: $3 }

| namePatPair seps_block seps_block namePatPairs
{ reportParseErrorAt (rhs parseState 3) (FSComp.SR.parsExpectingPattern ())
{ let mSep = match $3 with | Some sep -> sep.Range | None -> rhs parseState 3
reportParseErrorAt mSep (FSComp.SR.parsExpectingPattern ())
let (id: Ident), mEq, (pat: SynPat) = $1
let m = unionRanges id.idRange pat.Range
let lid = SynLongIdent([id], [], [None])
NamePatPairField(lid, mEq, m, pat, Some $2) :: $4 }
NamePatPairField(lid, mEq, m, pat, $2) :: $4 }

namePatPair:
| ident EQUALS parenPattern
Expand Down Expand Up @@ -4016,13 +4017,13 @@ recordPatternElementsAux:
| recordPatternElement seps_block recordPatternElementsAux
{ let (lid: SynLongIdent), mEq, (pat: SynPat) = $1
let m = unionRanges lid.Range pat.Range
NamePatPairField(lid, mEq, m, pat, Some $2) :: $3 }
NamePatPairField(lid, mEq, m, pat, $2) :: $3 }

| recordPatternElement seps_block seps_block recordPatternElementsAux
{ reportParseErrorAt (rhs parseState 3) (FSComp.SR.parsExpectingPattern ())
let (lid: SynLongIdent), mEq, (pat: SynPat) = $1
let m = unionRanges lid.Range pat.Range
NamePatPairField(lid, mEq, m, pat, Some $2) :: $4 }
NamePatPairField(lid, mEq, m, pat, $2) :: $4 }

recordPatternElement:
| path EQUALS parenPattern
Expand Down Expand Up @@ -5709,7 +5710,7 @@ recdExprCore:
| appExpr
{ let mExpr = rhs parseState 1
reportParseErrorAt mExpr (FSComp.SR.parsFieldBinding ())
Some($1, BlockSeparator.Offside(mExpr.EndRange, None)), [] }
(Some($1, None), []) }

/*
handles cases when identifier can start from the underscore
Expand Down Expand Up @@ -5743,36 +5744,40 @@ recdExprCore:
| appExpr WITH recdBinding recdExprBindings opt_seps_block
{ let l = List.rev $4
let l = rebindRanges $3 l $5
(Some($1, BlockSeparator.Offside(rhs parseState 2, None)), l) }
(Some($1, $5), l) }
Copy link
Member

@auduchinok auduchinok Sep 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@auduchinok auduchinok Sep 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@auduchinok auduchinok Sep 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@edgarfgp edgarfgp Sep 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@auduchinok auduchinok Sep 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@auduchinok auduchinok Sep 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@edgarfgp edgarfgp Sep 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@auduchinok auduchinok Sep 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


| appExpr OWITH opt_seps_block OEND
{ (Some($1, BlockSeparator.Offside(rhs parseState 2, None)), []) }
{ (Some($1, $3), []) }

| appExpr OWITH recdBinding recdExprBindings opt_seps_block OEND
{ let l = List.rev $4
let l = rebindRanges $3 l $5
(Some($1, BlockSeparator.Offside(rhs parseState 2, None)), l) }
(Some($1, $5), l) }

opt_seps_block:
| seps_block
{ Some $1 }
{ $1 }

| /* EMPTY */
{ None }

seps_block:
| OBLOCKSEP
{ BlockSeparator.Offside((rhs parseState 1), None) }
{
// OBLOCKSEP is purely syntactic: do not produce a BlockSeparator value
// Treat it as absence of an explicit separator
None
}

| SEMICOLON
{ let m = (rhs parseState 1)
BlockSeparator.Semicolon(m, Some m.End) }
Some (BlockSeparator.Semicolon(m)) }

| SEMICOLON OBLOCKSEP
{ BlockSeparator.Semicolon((rhs2 parseState 1 2), Some (rhs parseState 1).End) }
{ Some (BlockSeparator.Semicolon((rhs2 parseState 1 2))) }

| OBLOCKSEP SEMICOLON
{ BlockSeparator.Semicolon((rhs2 parseState 1 2), Some (rhs parseState 2).End) }
{ Some (BlockSeparator.Semicolon((rhs2 parseState 1 2))) }


/* identifier can start from the underscore */
Expand All @@ -5787,7 +5792,7 @@ pathOrUnderscore :

recdExprBindings:
| recdExprBindings seps_block recdBinding
{ ($3, Some $2) :: $1 }
{ ($3, $2) :: $1 }

| /* EMPTY */
{ [] }
Expand Down
Loading
Loading