Skip to content
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

Nested Record Field Copy and Update #14821

Merged
merged 41 commits into from
Mar 20, 2023
Merged
Show file tree
Hide file tree
Changes from 36 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
f746733
Nested record copy and update
kerams Feb 25, 2023
c3b5c0b
Add completions support
kerams Feb 26, 2023
76e1c4e
Add initial support for anonymous records
kerams Feb 27, 2023
2f843bb
Extend anonymous record support
kerams Feb 27, 2023
c9a2d29
Format
kerams Feb 27, 2023
7af7cbc
Improve Intellisense for anonymous records
kerams Feb 28, 2023
04cff96
Guard nested copy-and-update with a languafe feature
kerams Feb 28, 2023
2a5024f
Fix tests
kerams Feb 28, 2023
5da0738
Merge branch 'main' of https://github.com/dotnet/fsharp into nest
kerams Feb 28, 2023
f145126
Refactor
kerams Feb 28, 2023
0081cea
Refactor
kerams Feb 28, 2023
f973574
Format
kerams Feb 28, 2023
670f5c5
Fix resolution priority
kerams Feb 28, 2023
71abfae
Fix and refactor
kerams Mar 1, 2023
496d369
Merge branch 'main' of https://github.com/dotnet/fsharp into nest
kerams Mar 1, 2023
d2eced1
Refactor
kerams Mar 1, 2023
39ea7c6
Add first batch of tests
kerams Mar 1, 2023
95236e7
Add field qualification test
kerams Mar 2, 2023
f9938ac
Fix updates on recursive records
kerams Mar 2, 2023
d79c431
Add more tests
kerams Mar 2, 2023
09fa90d
Add more tests
kerams Mar 2, 2023
684c454
Improve Intellisense
kerams Mar 3, 2023
28b3631
Format
kerams Mar 3, 2023
c23718d
Fix surface area
kerams Mar 3, 2023
ce76288
Merge branch 'main' of https://github.com/dotnet/fsharp into nest
kerams Mar 3, 2023
08f44b3
Refactor
kerams Mar 3, 2023
139833c
Add diagnostic tests
kerams Mar 4, 2023
cf26fbb
Deduplicate items in nested field update tooltips
kerams Mar 4, 2023
679155e
Refactor
kerams Mar 7, 2023
0f49fb5
Merge branch 'main' of https://github.com/dotnet/fsharp into nest
kerams Mar 7, 2023
6cccd10
Refactor
kerams Mar 11, 2023
4eb5c90
Merge branch 'main' of https://github.com/dotnet/fsharp into nest
kerams Mar 11, 2023
6f6997a
Refactor
kerams Mar 13, 2023
b6894c5
Address comments
kerams Mar 14, 2023
ce4b1b7
Merge branch 'main' of https://github.com/dotnet/fsharp into nest
kerams Mar 14, 2023
0868ffc
Ooops
kerams Mar 14, 2023
a3067c5
Merge branch 'main' of https://github.com/dotnet/fsharp into nest
kerams Mar 15, 2023
4fc7c3c
Address comments
kerams Mar 15, 2023
151f3d8
Merge branch 'main' of https://github.com/dotnet/fsharp into nest
kerams Mar 17, 2023
a8cae6f
Add test with dotted field
kerams Mar 17, 2023
ec113d4
Merge branch 'main' into nest
T-Gro Mar 20, 2023
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
186 changes: 153 additions & 33 deletions src/Compiler/Checking/CheckExpressions.fs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ open FSharp.Compiler
open FSharp.Compiler.AbstractIL.IL
open FSharp.Compiler.AccessibilityLogic
open FSharp.Compiler.AttributeChecking
open FSharp.Compiler.CompilerGlobalState
open FSharp.Compiler.CheckBasics
open FSharp.Compiler.ConstraintSolver
open FSharp.Compiler.DiagnosticsLogger
Expand Down Expand Up @@ -1852,6 +1851,110 @@ let BuildFieldMap (cenv: cenv) env isPartial ty (flds: ((Ident list * Ident) * '
| _ -> error(Error(FSComp.SR.tcRecordFieldInconsistentTypes(), m)))
tinst, tcref, fldsmap, List.rev rfldsList

/// Merges updates to nested record fields on the same level in record copy-and-update.
///
/// `TransformAstForNestedUpdates` expands `{ x with A.B = 10; A.C = "" }`
///
/// into
///
/// { x with
/// A = { x.A with B = 10 };
/// A = { x.A with C = "" }
/// }
///
/// which we here convert to
///
/// { x with A = { x.A with B = 10; C = "" } }
let GroupUpdatesToNestedFields (fields: ((Ident list * Ident) * SynExpr option) list) =
vzarytovskii marked this conversation as resolved.
Show resolved Hide resolved
let rec groupIfNested res xs =
match xs with
| [] -> res
| x :: [] -> x :: res
| x :: y :: ys ->
match x, y with
| (lidwid, Some (SynExpr.Record (baseInfo, copyInfo, aFlds, m))), (_, Some (SynExpr.Record (recordFields = bFlds))) ->
let reducedRecd = (lidwid, Some(SynExpr.Record (baseInfo, copyInfo, aFlds @ bFlds, m)))
groupIfNested (reducedRecd :: res) ys
| (lidwid, Some (SynExpr.AnonRecd (isStruct, copyInfo, aFlds, m, trivia))), (_, Some (SynExpr.AnonRecd (recordFields = bFlds))) ->
let reducedRecd = (lidwid, Some(SynExpr.AnonRecd (isStruct, copyInfo, aFlds @ bFlds, m, trivia)))
groupIfNested (reducedRecd :: res) ys
| _ -> groupIfNested (x :: res) (y :: ys)

fields
|> List.groupBy (fun ((_, field), _) -> field.idText)
|> List.collect (fun (_, fields) ->
if fields.Length < 2 then
fields
else
groupIfNested [] fields)

/// Expands a long identifier into nested copy-and-update expressions.
///
/// `{ x with A.B = 0 }` becomes `{ x with A = { x.A with B = 0 } }`
let TransformAstForNestedUpdates cenv env overallTy (lid: LongIdent) exprBeingAssigned withExpr =
let recdExprCopyInfo ids withExpr id =
let upToId origSepRng id lidwd =
let rec buildLid res (id: Ident) =
function
| [] -> res
| (h: Ident) :: t ->
// Mark these hidden field accesses as synthetic so that they don't make it
// into the name resolution sink.
let h = ident(h.idText, h.idRange.MakeSynthetic())
if equals h.idRange id.idRange then h :: res else buildLid (h :: res) id t

let calcLidSeparatorRanges origSepRng lid =
match lid with
| [] | [_] -> [ origSepRng ]
| _ :: t -> origSepRng :: List.map (fun (s: Ident, e: Ident) -> mkRange s.idRange.FileName s.idRange.End e.idRange.Start) t

let lid = buildLid [] id lidwd |> List.rev

(lid, List.pairwise lid |> calcLidSeparatorRanges origSepRng)

let totalRange (origId: Ident) (id: Ident) =
mkRange origId.idRange.FileName origId.idRange.End id.idRange.Start

let rangeOfBlockSeperator (id: Ident) =
let idEnd = id.idRange.End
let blockSeperatorStartCol = idEnd.Column
let blockSeperatorEndCol = blockSeperatorStartCol + 4
let blockSeperatorStartPos = mkPos idEnd.Line blockSeperatorStartCol
let blockSeporatorEndPos = mkPos idEnd.Line blockSeperatorEndCol

mkRange id.idRange.FileName blockSeperatorStartPos blockSeporatorEndPos

match withExpr with
| SynExpr.Ident origId, (sepRange, _) ->
let lid, rng = upToId sepRange id (origId :: ids)
Some (SynExpr.LongIdent (false, LongIdentWithDots(lid, rng), None, totalRange origId id), (rangeOfBlockSeperator id, None))
| _ -> None

let rec synExprRecd copyInfo (id: Ident) fields exprBeingAssigned =
match fields with
| [] -> failwith "unreachable"
vzarytovskii marked this conversation as resolved.
Show resolved Hide resolved
| (fieldId, anonInfo) :: rest ->
let nestedField = if rest.IsEmpty then exprBeingAssigned else synExprRecd copyInfo fieldId rest exprBeingAssigned
let m = id.idRange.MakeSynthetic()

match anonInfo with
| Some { AnonRecdTypeInfo.TupInfo = TupInfo.Const isStruct } ->
let fields = [ LongIdentWithDots ([ fieldId ], []), None, nestedField ]
SynExpr.AnonRecd(isStruct, copyInfo id, fields, m, { OpeningBraceRange = range0 })
| _ ->
let fields = [ SynExprRecordField((LongIdentWithDots ([ fieldId ], []), true), None, Some nestedField, None) ]
SynExpr.Record(None, copyInfo id, fields, m)

let access, fields = ResolveNestedField cenv.tcSink cenv.nameResolver env.eNameResEnv env.eAccessRights overallTy lid

match access, fields with
| _, [] -> failwith "unreachable"
| accessIds, [ (fieldId, _) ] -> (accessIds, fieldId), Some exprBeingAssigned
| accessIds, (fieldId, _) :: rest ->
checkLanguageFeatureAndRecover cenv.g.langVersion LanguageFeature.NestedCopyAndUpdate (rangeOfLid lid)

(accessIds, fieldId), Some (synExprRecd (recdExprCopyInfo (fields |> List.map fst) withExpr) fieldId rest exprBeingAssigned)

let rec ApplyUnionCaseOrExn (makerForUnionCase, makerForExnTag) m (cenv: cenv) env overallTy item =
let g = cenv.g
let ad = env.eAccessRights
Expand Down Expand Up @@ -6568,7 +6671,9 @@ and TcRecordConstruction (cenv: cenv) (overallTy: TType) env tpenv withExprInfoO
if not (Zset.subset ns2 ns1) then
error(MissingFields(Zset.elements (Zset.diff ns2 ns1), m))
| _ ->
if oldFldsList.IsEmpty then
// `TransformAstForNestedUpdates` creates record constructions with synthetic ranges.
// Don't emit the warning for nested field updates, because it does not really make sense.
if oldFldsList.IsEmpty && not m.IsSynthetic then
let enabledByLangFeature = g.langVersion.SupportsFeature LanguageFeature.WarningWhenCopyAndUpdateRecordChangesAllFields
warning(ErrorEnabledWithLanguageFeature(FSComp.SR.tcCopyAndUpdateRecordChangesAllFields(fullDisplayTextOfTyconRef tcref), m, enabledByLangFeature))

Expand Down Expand Up @@ -7294,37 +7399,40 @@ and TcAssertExpr cenv overallTy env (m: range) tpenv x =

TcExpr cenv overallTy env tpenv callDiagnosticsExpr

and TcRecdExpr cenv (overallTy: TType) env tpenv (inherits, withExprOpt, synRecdFields, mWholeExpr) =

and TcRecdExpr cenv overallTy env tpenv (inherits, withExprOpt, synRecdFields, mWholeExpr) =
let g = cenv.g

let requiresCtor = (GetCtorShapeCounter env = 1) // Get special expression forms for constructors
let haveCtor = Option.isSome inherits

let withExprOpt, tpenv =
match withExprOpt with
| None -> None, tpenv
| Some (origExpr, _) ->
match inherits with
| Some (_, _, mInherits, _, _) -> error(Error(FSComp.SR.tcInvalidRecordConstruction(), mInherits))
| None ->
let withExpr, tpenv = TcExpr cenv (MustEqual overallTy) env tpenv origExpr
Some withExpr, tpenv
let withExprOptChecked, tpenv =
match withExprOpt with
| None -> None, tpenv
| Some (origExpr, _) ->
match inherits with
| Some (_, _, mInherits, _, _) -> error(Error(FSComp.SR.tcInvalidRecordConstruction(), mInherits))
| None ->
let withExpr, tpenv = TcExpr cenv (MustEqual overallTy) env tpenv origExpr
Some withExpr, tpenv

let hasOrigExpr = withExprOpt.IsSome
let hasOrigExpr = withExprOptChecked.IsSome

let fldsList =
let flds =
[
synRecdFields
|> List.map (fun (SynExprRecordField (fieldName = (synLongId, isOk); expr = exprBeingAssigned)) ->
// if we met at least one field that is not syntactically correct - raise ReportedError to transfer control to the recovery routine
for SynExprRecordField(fieldName=(synLongId, isOk); expr=v) in synRecdFields do
if not isOk then
// raising ReportedError None transfers control to the closest errorRecovery point but do not make any records into log
// we assume that parse errors were already reported
raise (ReportedError None)
if not isOk then
// raising ReportedError None transfers control to the closest errorRecovery point but do not make any records into log
// we assume that parse errors were already reported
raise (ReportedError None)

match withExprOpt, synLongId.LongIdent, exprBeingAssigned with
| _, [ id ], _ -> ([], id), exprBeingAssigned
| Some withExpr, lid, Some exprBeingAssigned -> TransformAstForNestedUpdates cenv env overallTy lid exprBeingAssigned withExpr
| _ -> List.frontAndBack synLongId.LongIdent, exprBeingAssigned)

yield (List.frontAndBack synLongId.LongIdent, v)
]
let flds = if hasOrigExpr then GroupUpdatesToNestedFields flds else flds

match flds with
| [] -> []
Expand All @@ -7339,7 +7447,7 @@ and TcRecdExpr cenv (overallTy: TType) env tpenv (inherits, withExprOpt, synRecd
| None -> () ]

let withExprInfoOpt =
match withExprOpt with
match withExprOptChecked with
| None -> None
| Some withExpr ->
let withExprAddrVal, withExprAddrValExpr = mkCompGenLocal mWholeExpr "inputRecord" (if isStructTy g overallTy then mkByrefTy g overallTy else overallTy)
Expand Down Expand Up @@ -7393,22 +7501,22 @@ and TcAnonRecdExpr cenv (overallTy: TType) env tpenv (isStruct, optOrigSynExpr,

// Check for duplicate field IDs
unsortedFieldIdsAndSynExprsGiven
|> List.countBy (fun (fId, _, _) -> fId.idText)
|> List.countBy (fun (fId, _, _) -> textOfLid fId.LongIdent)
|> List.iter (fun (label, count) ->
if count > 1 then error (Error (FSComp.SR.tcAnonRecdDuplicateFieldId(label), mWholeExpr)))

match optOrigSynExpr with
| None ->
TcNewAnonRecdExpr cenv overallTy env tpenv (isStruct, unsortedFieldIdsAndSynExprsGiven, mWholeExpr)

| Some (origExpr, _) ->
TcCopyAndUpdateAnonRecdExpr cenv overallTy env tpenv (isStruct, origExpr, unsortedFieldIdsAndSynExprsGiven, mWholeExpr)
| Some orig ->
TcCopyAndUpdateAnonRecdExpr cenv overallTy env tpenv (isStruct, orig, unsortedFieldIdsAndSynExprsGiven, mWholeExpr)

and TcNewAnonRecdExpr cenv (overallTy: TType) env tpenv (isStruct, unsortedFieldIdsAndSynExprsGiven, mWholeExpr) =

let g = cenv.g
let unsortedFieldSynExprsGiven = unsortedFieldIdsAndSynExprsGiven |> List.map (fun (_, _, fieldExpr) -> fieldExpr)
let unsortedFieldIds = unsortedFieldIdsAndSynExprsGiven |> List.map (fun (fieldId, _, _) -> fieldId) |> List.toArray
let unsortedFieldIds = unsortedFieldIdsAndSynExprsGiven |> List.map (fun (synLongIdent, _, _) -> synLongIdent.LongIdent[0]) |> List.toArray
let anonInfo, sortedFieldTys = UnifyAnonRecdTypeAndInferCharacteristics env.eContextInfo cenv env.DisplayEnv mWholeExpr overallTy isStruct unsortedFieldIds

// Sort into canonical order
Expand All @@ -7421,9 +7529,10 @@ and TcNewAnonRecdExpr cenv (overallTy: TType) env tpenv (isStruct, unsortedField
let sigma = sortedIndexedArgs |> List.map fst |> List.toArray
let sortedFieldExprs = sortedIndexedArgs |> List.map snd

sortedFieldExprs |> List.iteri (fun j (fieldId, _, _) ->
let item = Item.AnonRecdField(anonInfo, sortedFieldTys, j, fieldId.idRange)
CallNameResolutionSink cenv.tcSink (fieldId.idRange, env.NameEnv, item, emptyTyparInst, ItemOccurence.Use, env.eAccessRights))
sortedFieldExprs |> List.iteri (fun j (synLongIdent, _, _) ->
let m = rangeOfLid synLongIdent.LongIdent
let item = Item.AnonRecdField(anonInfo, sortedFieldTys, j, m)
CallNameResolutionSink cenv.tcSink (m, env.NameEnv, item, emptyTyparInst, ItemOccurence.Use, env.eAccessRights))

let unsortedFieldTys =
sortedFieldTys
Expand All @@ -7437,7 +7546,7 @@ and TcNewAnonRecdExpr cenv (overallTy: TType) env tpenv (isStruct, unsortedField

mkAnonRecd g mWholeExpr anonInfo unsortedFieldIds unsortedCheckedArgs unsortedFieldTys, tpenv

and TcCopyAndUpdateAnonRecdExpr cenv (overallTy: TType) env tpenv (isStruct, origExpr, unsortedFieldIdsAndSynExprsGiven, mWholeExpr) =
and TcCopyAndUpdateAnonRecdExpr cenv (overallTy: TType) env tpenv (isStruct, (origExpr, blockSeparator), unsortedFieldIdsAndSynExprsGiven, mWholeExpr) =
// The fairly complex case '{| origExpr with X = 1; Y = 2 |}'
// The origExpr may be either a record or anonymous record.
// The origExpr may be either a struct or not.
Expand All @@ -7448,7 +7557,6 @@ and TcCopyAndUpdateAnonRecdExpr cenv (overallTy: TType) env tpenv (isStruct, ori
// Unlike in the case of record type copy-and-update {| a with X = 1 |} does not force a.X to exist or have had type 'int'

let g = cenv.g
let unsortedFieldSynExprsGiven = unsortedFieldIdsAndSynExprsGiven |> List.map (fun (_, _, e) -> e)
let origExprTy = NewInferenceType g
let origExprChecked, tpenv = TcExpr cenv (MustEqual origExprTy) env tpenv origExpr
let oldv, oldve = mkCompGenLocal mWholeExpr "inputRecord" origExprTy
Expand All @@ -7457,6 +7565,18 @@ and TcCopyAndUpdateAnonRecdExpr cenv (overallTy: TType) env tpenv (isStruct, ori
if not (isAppTy g origExprTy || isAnonRecdTy g origExprTy) then
error (Error (FSComp.SR.tcCopyAndUpdateNeedsRecordType(), mOrigExpr))

// Expand expressions with respect to potential nesting
let unsortedFieldIdsAndSynExprsGiven =
unsortedFieldIdsAndSynExprsGiven
|> List.map (fun (synLongIdent, _, exprBeingAssigned) ->
match synLongIdent.LongIdent with
| [] -> error(Error(FSComp.SR.nrUnexpectedEmptyLongId(), mWholeExpr))
| [ id ] -> ([], id), Some exprBeingAssigned
| lid -> TransformAstForNestedUpdates cenv env origExprTy lid exprBeingAssigned (origExpr, blockSeparator))
|> GroupUpdatesToNestedFields

let unsortedFieldSynExprsGiven = unsortedFieldIdsAndSynExprsGiven |> List.choose snd

let origExprIsStruct =
match tryDestAnonRecdTy g origExprTy with
| ValueSome (anonInfo, _) -> evalTupInfoIsStruct anonInfo.TupInfo
Expand All @@ -7472,7 +7592,7 @@ and TcCopyAndUpdateAnonRecdExpr cenv (overallTy: TType) env tpenv (isStruct, ori
/// - Choice2Of2 for a binding coming from the original expression
let unsortedIdAndExprsAll =
[|
for id, _, e in unsortedFieldIdsAndSynExprsGiven do
for (_, id), e in unsortedFieldIdsAndSynExprsGiven do
yield (id, Choice1Of2 e)
match tryDestAnonRecdTy g origExprTy with
| ValueSome (anonInfo, tinst) ->
Expand Down
2 changes: 1 addition & 1 deletion src/Compiler/Checking/CheckExpressions.fsi
Original file line number Diff line number Diff line change
Expand Up @@ -892,7 +892,7 @@ val BuildFieldMap:
env: TcEnv ->
isPartial: bool ->
ty: TType ->
((Ident list * Ident) * 'T) list ->
flds: ((Ident list * Ident) * 'T) list ->
m: range ->
TypeInst * TyconRef * Map<string, 'T> * (string * 'T) list

Expand Down
Loading