Skip to content
Closed
Show file tree
Hide file tree
Changes from all 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
58 changes: 46 additions & 12 deletions src/Compiler/Checking/CheckRecordSyntaxHelpers.fs
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,9 @@ let GroupUpdatesToNestedFields (fields: ((Ident list * Ident) * SynExpr option)
| [ x ] -> x :: res
| x :: y :: ys ->
match x, y with
| (lidwid, Some(SynExpr.Record(baseInfo, copyInfo, fields1, m))), (_, Some(SynExpr.Record(recordFields = fields2))) ->
| (lidwid, Some(SynExpr.Record(baseInfo, copyInfo, fields1, m, trivia))), (_, Some(SynExpr.Record(recordFields = fields2))) ->
let reducedRecd =
(lidwid, Some(SynExpr.Record(baseInfo, copyInfo, fields1 @ fields2, m)))
(lidwid, Some(SynExpr.Record(baseInfo, copyInfo, fields1 @ fields2, m, trivia)))

groupIfNested res (reducedRecd :: ys)
| (lidwid, Some(SynExpr.AnonRecd(isStruct, copyInfo, fields1, m, trivia))), (_, Some(SynExpr.AnonRecd(recordFields = fields2))) ->
Expand Down 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,22 @@ 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
WithKeyword = None
}
)
| _ ->
let fields =
[
Expand All @@ -133,7 +158,16 @@ let TransformAstForNestedUpdates (cenv: TcFileState) (env: TcEnv) overallTy (lid
)
]

SynExpr.Record(None, copyInfo outerFieldId, fields, outerFieldId.idRange)
SynExpr.Record(
None,
copyInfo outerFieldId,
fields,
outerFieldId.idRange,
{
OpeningBraceRange = outerFieldId.idRange
WithKeyword = None
}
)

let access, fields =
ResolveNestedField cenv.tcSink cenv.nameResolver env.eNameResEnv env.eAccessRights overallTy lid
Expand All @@ -153,7 +187,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
10 changes: 5 additions & 5 deletions src/Compiler/Checking/Expressions/CheckExpressions.fs
Original file line number Diff line number Diff line change
Expand Up @@ -5929,14 +5929,14 @@ and TcExprUndelayed (cenv: cenv) (overallTy: OverallTy) env tpenv (synExpr: SynE
let binds = unionBindingAndMembers binds members
TcExprObjectExpr cenv overallTy env tpenv (synObjTy, argopt, binds, extraImpls, mNewExpr, m)

| SynExpr.Record (inherits, withExprOpt, synRecdFields, mWholeExpr) ->
| SynExpr.Record (inherits, withExprOpt, synRecdFields, mWholeExpr, _trivia) ->
match withExprOpt with
| None
| Some(SynExpr.Ident _, _) ->
TcNonControlFlowExpr env <| fun env ->
TcExprRecord cenv overallTy env tpenv (inherits, withExprOpt, synRecdFields, mWholeExpr)
| Some withExpr ->
BindOriginalRecdExpr withExpr (fun withExpr -> SynExpr.Record (inherits, withExpr, synRecdFields, mWholeExpr))
BindOriginalRecdExpr withExpr (fun withExpr -> SynExpr.Record (inherits, withExpr, synRecdFields, mWholeExpr, { OpeningBraceRange = mWholeExpr; WithKeyword = None }))
|> TcExpr cenv overallTy env tpenv

| SynExpr.While (spWhile, synGuardExpr, synBodyExpr, m) ->
Expand Down Expand Up @@ -8288,7 +8288,7 @@ and Propagate (cenv: cenv) (overallTy: OverallTy) (env: TcEnv) tpenv (expr: Appl

// async { }
// seq { }
| SynExpr.Record (None, None, [], _) when g.langVersion.SupportsFeature LanguageFeature.EmptyBodiedComputationExpressions -> ()
| SynExpr.Record (None, None, [], _, _) when g.langVersion.SupportsFeature LanguageFeature.EmptyBodiedComputationExpressions -> ()

// expr[idx]
// expr[idx1, idx2]
Expand Down Expand Up @@ -8588,7 +8588,7 @@ and TcApplicationThen (cenv: cenv) (overallTy: OverallTy) env tpenv mExprAndArg
// seq { comp }
// seq { }
| SynExpr.ComputationExpr (false, comp, m)
| SynExpr.Record (None, None, EmptyFieldListAsUnit comp, m) when
| SynExpr.Record (None, None, EmptyFieldListAsUnit comp, m, _) when
(match leftExpr with
| ApplicableExpr(expr=Expr.Op(TOp.Coerce, _, [SeqExpr g], _)) -> true
| _ -> false) ->
Expand Down Expand Up @@ -8637,7 +8637,7 @@ and TcApplicationThen (cenv: cenv) (overallTy: OverallTy) env tpenv mExprAndArg
// leftExpr { comp }
// leftExpr { }
| SynExpr.ComputationExpr (false, comp, _m)
| SynExpr.Record (None, None, EmptyFieldListAsUnit comp, _m) ->
| SynExpr.Record (None, None, EmptyFieldListAsUnit comp, _m, _) ->
let bodyOfCompExpr, tpenv = cenv.TcComputationExpression cenv env overallTy tpenv (mLeftExpr, leftExpr.Expr, exprTy, comp)
TcDelayed cenv overallTy env tpenv mExprAndArg (MakeApplicableExprNoFlex cenv bodyOfCompExpr) (tyOfExpr g bodyOfCompExpr) ExprAtomicFlag.NonAtomic delayed

Expand Down
2 changes: 1 addition & 1 deletion src/Compiler/Service/FSharpParseFileResults.fs
Original file line number Diff line number Diff line change
Expand Up @@ -629,7 +629,7 @@ type FSharpParseFileResults(diagnostics: FSharpDiagnostic[], input: ParsedInput,
| SynExpr.ArrayOrList(_, exprs, _)
| SynExpr.Tuple(_, exprs, _, _) -> yield! walkExprs exprs

| SynExpr.Record(_, copyExprOpt, fs, _) ->
| SynExpr.Record(_, copyExprOpt, fs, _, _) ->
match copyExprOpt with
| Some(e, _) -> yield! walkExpr true e
| None -> ()
Expand Down
2 changes: 1 addition & 1 deletion src/Compiler/Service/ServiceInterfaceStubGenerator.fs
Original file line number Diff line number Diff line change
Expand Up @@ -849,7 +849,7 @@ module InterfaceStubGenerator =
| SynExpr.Tuple(_, synExprList, _, _range)
| SynExpr.ArrayOrList(_, synExprList, _range) -> List.tryPick walkExpr synExprList

| SynExpr.Record(_inheritOpt, _copyOpt, fields, _range) ->
| SynExpr.Record(_inheritOpt, _copyOpt, fields, _range, _) ->
List.tryPick (fun (SynExprRecordField(expr = e)) -> Option.bind walkExpr e) fields

| SynExpr.New(_, _synType, synExpr, _range) -> walkExpr synExpr
Expand Down
86 changes: 45 additions & 41 deletions src/Compiler/Service/ServiceParseTreeWalk.fs
Original file line number Diff line number Diff line change
Expand Up @@ -400,6 +400,42 @@ module SyntaxTraversal =
let traverseSynType = traverseSynType path
let traversePat = traversePat path

let diveIntoRecordLikeExpr path (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 ->
// FIXME: handle caret-after-WITH when no separator token
match expr with
| SynExpr.Ident id ->
let idEnd = id.idRange.End
let withStartPos = mkPos idEnd.Line idEnd.Column
let withEndPos = mkPos idEnd.Line (idEnd.Column + 4)
let withRange = withStartEnd withStartPos withEndPos id.idRange

yield
dive () withRange (fun () ->
if posGeq pos withEndPos 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 +480,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 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 +490,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 +521,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 copyOpt

let copyOpt = Option.map fst copyOpt

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

]
Expand Down
6 changes: 3 additions & 3 deletions src/Compiler/Service/ServiceParsedInputOps.fs
Original file line number Diff line number Diff line change
Expand Up @@ -818,7 +818,7 @@ module ParsedInput =

| SynExpr.MatchLambda(matchClauses = clauses) -> List.tryPick walkClause clauses

| SynExpr.Record(_, _, fields, r) ->
| SynExpr.Record(_, _, fields, r, _) ->
ifPosInRange r (fun _ ->
fields
|> List.tryPick (fun (SynExprRecordField(expr = e)) -> e |> Option.bind (walkExprWithKind parentKind)))
Expand Down Expand Up @@ -1475,7 +1475,7 @@ module ParsedInput =
| PartOfParameterList pos precedingArgument args -> Some(CompletionContext.ParameterList args)
| _ -> defaultTraverse expr

| SynExpr.Record(None, None, [], _) -> Some(CompletionContext.RecordField RecordContext.Empty)
| SynExpr.Record(None, None, [], _, _) -> Some(CompletionContext.RecordField RecordContext.Empty)

// Unchecked.defaultof<str$>
| SynExpr.TypeApp(typeArgsRange = range) when rangeContainsPos range pos -> Some CompletionContext.Type
Expand All @@ -1501,7 +1501,7 @@ module ParsedInput =
| SyntaxNode.SynExpr _ :: SyntaxNode.SynBinding _ :: SyntaxNode.SynMemberDefn _ :: SyntaxNode.SynTypeDefn(SynTypeDefn(
typeInfo = SynComponentInfo(longId = [ id ]))) :: _ -> RecordContext.Constructor(id.idText)

| SyntaxNode.SynExpr(SynExpr.Record(None, _, fields, _)) :: _ ->
| SyntaxNode.SynExpr(SynExpr.Record(None, _, fields, _, _)) :: _ ->
let isFirstField =
match field, fields with
| Some contextLid, SynExprRecordField(fieldName = lid, _) :: _ -> contextLid.Range = lid.Range
Expand Down
2 changes: 1 addition & 1 deletion src/Compiler/Service/ServiceStructure.fs
Original file line number Diff line number Diff line change
Expand Up @@ -430,7 +430,7 @@ module Structure =

| SynExpr.Paren(e, _, _, _) -> parseExpr e

| SynExpr.Record(recCtor, recCopy, recordFields, r) ->
| SynExpr.Record(recCtor, recCopy, recordFields, r, _) ->
match recCtor with
| Some(_, ctorArgs, _, _, _) -> parseExpr ctorArgs
| _ -> ()
Expand Down
23 changes: 8 additions & 15 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,18 +541,19 @@ 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
trivia: SynExprRecdTrivia

| ArrayOrList of isArray: bool * exprs: SynExpr list * range: range

| 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
range: range *
trivia: SynExprRecdTrivia

| New of isProtected: bool * targetType: SynType * expr: SynExpr * range: range

Expand Down
Loading
Loading