Skip to content
Closed
Show file tree
Hide file tree
Changes from 14 commits
Commits
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
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

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

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
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
80 changes: 39 additions & 41 deletions src/Compiler/Service/ServiceParseTreeWalk.fs
Original file line number Diff line number Diff line change
Expand Up @@ -400,6 +400,36 @@ module SyntaxTraversal =
let traverseSynType = traverseSynType path
let traversePat = traversePat path

let diveIntoRecordLikeExpr path (exprRange: range) (hasFields: bool) (copyOpt: (SynExpr * BlockSeparator option) option) =
[
match copyOpt with
| Some(expr, blockSep) ->
// Dive into the copy expression
yield dive expr expr.Range traverseSynExpr

// Dive into the WITH separator (if present) and handle caret-after-WITH
match blockSep with
| Some blockSep ->
yield
dive () blockSep.Range (fun () ->
if posGeq pos blockSep.Range.End then
// { x with $ }
visitor.VisitRecordField(path, Some expr, None)
else
None)
| None ->
// If there is a copy-with but no explicit separator and no fields yet,
// still offer field completions once the caret is after the copy expr.
if not hasFields then
yield
dive () exprRange (fun () ->
if posGeq pos expr.Range.End then
visitor.VisitRecordField(path, Some expr, None)
else
None)
| None -> ()
]

match e with
| SynExpr.LongIdentSet(expr = synExpr)
| SynExpr.DotGet(expr = synExpr)
Expand Down Expand Up @@ -444,19 +474,7 @@ module SyntaxTraversal =

| SynExpr.AnonRecd(copyInfo = copyOpt; recordFields = fields) ->
[
match copyOpt with
| Some(expr, blockSep) ->
yield dive expr expr.Range traverseSynExpr

yield
dive () blockSep.Range (fun () ->
if posGeq pos blockSep.Range.End then
// special case: caret is after WITH
// { x with $ }
visitor.VisitRecordField(path, Some expr, None)
else
None)
| _ -> ()
yield! diveIntoRecordLikeExpr path expr.Range (not (List.isEmpty fields)) copyOpt

for field, _, x in fields do
yield dive () field.Range (fun () -> visitor.VisitRecordField(path, copyOpt |> Option.map fst, Some field))
Expand All @@ -466,19 +484,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,23 +515,11 @@ module SyntaxTraversal =
// inherit A()
// $
// field1 = 5
diveIntoSeparator inheritRange.StartColumn blockSep.Position None)
diveIntoSeparator blockSep.Range.End None)
| None -> ()
| _ -> ()

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

yield
dive () blockSep.Range (fun () ->
if posGeq pos blockSep.Range.End then
// special case: caret is after WITH
// { x with $ }
visitor.VisitRecordField(path, Some expr, None)
else
None)
| _ -> ()
yield! diveIntoRecordLikeExpr path expr.Range (not (List.isEmpty fields)) copyOpt

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
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

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
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
37 changes: 22 additions & 15 deletions src/Compiler/pars.fsy
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,42 @@ 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, None), l) }
Copy link
Member

Choose a reason for hiding this comment

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

The captured info is still different here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


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

| 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, None), 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) }
{ let mSemi = (rhs parseState 1)
Some (BlockSeparator.Semicolon(mSemi)) }

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


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

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

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