[WIP] Nested Record Field Copy and Update Expression

[williamtetlow]

fsharp/fslang-suggestions#379

TODOs

• Create RFC
• Fix implementation to enable updating more than 1 nested field
• Fix implementation to handle ambiguities between TypeName.FieldName & FieldName.FieldName
• Resolution of access to nested field that is type with [<RequireQualifiedAccess>] attribute
• Resolution of records accessed through namespace or module
• Handle type abbreviations
• Fix Build
• Add Tests
• Add negative tests
• Check all parts of nested field are declared in record
• Check the same field is not declared twice in a nested field update
• Fix Intellisense suggestions for nested record field access
• Fix incorrect highlighting of TypeNames used in nested record field access
• Further testing of Intellisense
• Collaboration with anonymous records feature
• Investigate other language features that should support nested paths

[msftclas]

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

❌ williamtetlow sign now
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[AviAvni]

This pr is working on first class lens I'm working with @williamtetlow as part of FSSF mentorship program

We already able to compile code like

type Street = { N: string }
type Address = { S: Street }
type Person = { A: Address; Age: int }
​
let person = { A = { S = { N = "Street 1" } }; Age = 30 }
​
let anotherPerson = { person with A = { person.A with S = { person.A.S with N = person.A.S.N + ", k.2" } } }
​
let anotherPerson1 = { person with A.S.N = person.A.S.N + ", k.2" }

The current situation "with" syntax able to update 1 level of fields of record
There are 2 ways to specify the field
FieldName
TypeName.FieldName

We added the ability to update more then 1 level like
FieldName.FieldName

The way we did it is by transform the ast in the type checker see the example above

This is still work in progress
Some of the things we still need to work on
Fix the implementation to enable to update more then 1 nested field
Fix the build
Add tests
Handle the TypeName.FieldName vs FieldName.FieldName possible ambiguity
Check that the all parts of nested field is declared in record
Check that intellisence work as expected
Check how this behave with anonymous records

We will be happy to get feedback

[forki]

cool stuff.

the current build error is already a good unit test ;-)

the fieldname is prefixed with a type

[forki]

something that also needs to be done (later when dust settles): for the standard case we have suggestions working. we should add that as well for nested records

[cartermp]

Modified the title to be WIP.

@williamtetlow and @AviAvni absolutely love seeing this. Can you add a checklist of TODOs to the initial text of this PR so that we can easily see what's left to cover? Thanks!

[dsyme]

@cartermp @williamtetlow @AviAvni We also need an RFC for this, and a language suggestion (I couldn't find one on q quick search though one may exist)

I have some concerns, in particular about whether nested paths should be supported in other places, e.g. named argument setters:

obj.Method(Prop1.NestedProp=3)
SomeClass(Prop1.NestedProp=3, Prop1.NestedProp2=6)

It feels to me like if we're going to support one kind of nested update, we should support two. And what about indexers?

{ mylist with Item(4).NestedProperty = 4 }
obj.Method(Item(4).NestedProperty=3)

This seems useful and orthogonal in the context of both immutable and mutable data.

Anyway, I'd like to discuss this properly from a design perspective, to make sure we are not just adding a feature that only applies to records when it can logically apply elsewhere too

[AviAvni]

@dsyme suggestion fsharp/fslang-suggestions#379
RFC I'll work on it later

[AviAvni]

@dsyme didn't understand to what

obj.Method(Prop1.NestedProp=3)

will compile to

[smoothdeveloper]

@AviAvni

let temp = obj.Method()
temp.Prop1.NestedProp <- 3

This is mostly used in constructors the same way C# has initializer syntax, but in F# it also works on plain methods, applying the setters on the return value.

[dsyme]

@AviAvni Currently

obj.Method(SettableProp1=x)

compiles to

let tmp = obj.Method()
tmp.SettableProp1 <- x
tmp

So

obj.Method(Prop1.SettableNestedProp1=x)

would be

let tmp = obj.Method()
tmp.Prop1.SettableNestedProp1 <- x
tmp

Likewise there would a question about whether long paths could be used in named and/or optional arguments, e.g. would this have any meaning:

member __.Method1(arg1 : SomeType)

obj.Method(arg1.Prop1=x)

Though it's not clear what the meaning should be - it's like we would want functional update on a default argument but that doesn't fit that well with how F# does default arguments today. I can see some use for this when option/argument spaces are very large (e.g. charting), e.g.

Chart.Plot(Colors.Background=Color.Red, Colors.Foreground=Color.Blue, Layout.Packing=4)

The overall point is that we want to maintain some degree of orthogonality (or at least the feeling of orthogonality) between record labels and named arguments...

[AviAvni]

@dsyme thanks we will work on it after finish with record things

[dsyme]

Note also we will need the same thing to work for anonymous records.. #4499. I just did the copy-and-update implementation for that today

[williamtetlow]

@dsyme How should we handle the case of nested access to a record field that is a type with the [<RequiredQualifiedAccess>] attribute?

Should we require the inclusion of the type/module that has been marked as required or allow nested access without this? (let b vs let c)

e.g.

[<RequireQualifiedAccess>]
type SomeType = { B : string; }

type AnotherType = { A : SomeType }

let a = { A = { SomeType.B = "a" } }

let b = { a with A.SomeType.B = "b" } 

let c = { a with A.B = "b" } 

[dsyme]

@dsyme How should we handle the case of nested access to a record field that is a type with the [] attribute?

No. The fact the field has type SomeType is enough. RequiredQualifiedAccess is really "require either qualified access or a type-directed name resolution`

[zpodlovics]

Will it work with struct records without introducing lot's of struct copy? eg.: a 6 level deep nested field access could create lot's of struct copy to access a simple field. Also the update order may matters a lot for struct records.

[cartermp]

@williamtetlow Fantastic progress so far, good work.

Could you also go through the RFC and update it with the behavioral changes you've made in this PR? Moving forward, we want the RFCs to be a canonical source for what, why, and a high-level how for all future changes to the language. This traceability is crucial for helping people understand behavior and help others on places such as StackOverflow.

Additionally, @zpodlovics has a good concern regarding struct records. Could this also be evaluated and written up in the RFC as something that is being accounted for?

Thank you!

[zpodlovics]

Right now the struct wrapper ctors are not optimized (https://github.com/dotnet/coreclr/issues/18542) and will require lot's of stack space and copy, especially with deep nesting: https://github.com/dotnet/coreclr/issues/18542#issuecomment-399535658

[williamtetlow]

RFC has been updated with behavioural changes and answer to @zpodlovics question about struct records. fsharp/fslang-design#333

The current implementation is expanding the simplified AST into the existing one and will have the same struct copy issue mentioned by @zpodlovics. However, as this issue is present in the original copy and update expression this would have to be fixed as well as updating the new implementation. This will be investigated further once the open tasks are completed

[realvictorprm]

Good job with this @williamtetlow 👍 After the whole SRTP topic is finished I want to help out 😃

[forki]

ping

[cartermp]

@williamtetlow @AviAvni Any progress on the remaining WIP items? We'll refrain from reviewing until you feel it's ready.

Now that Anonymous Records are merged and (soon) shipping, we'll want to make sure this can work with them too. I can update the RFC for that.

[SchlenkR]

Are record fields with union types somehow supported?

Like this:

type Response = {
    printHint: PrintHint option
}
and PrintHint = {
    requestPrintHint: RequestPrintHint option;
}
and RequestPrintHint = {
    printHeader: bool
}

Would it be possible to match cases in a given path of an update Expression and only update on match?

[vasily-kirichenko]

Why not Map.tryFind with match?

[vasily-kirichenko]

I don't think throwing a couple of chars saves a lot of space in this case. Why not buildForNestedFields?

[williamtetlow]

@cartermp there hasn't been any progress over the last few months, the intellisense issues are still outstanding and integration with anonymous records.

I was struggling to find a solution for the wrong suggestions being presented when you start going more than one field deep. e.g.

let anotherPerson1 = { person with A.S.

suggests S instead of N.

I will catch up with @AviAvni.

@vasily-kirichenko thanks for the feedback. I will take a look.

[realvictorprm]

Would be nice if we can get going here again. If I find time to do something again, I want to do something here too.

[KevinRansom]

@realvictorprm, it's been a long time since anyone looked at this? do you want to take it over or should I close it?

Thanks

Kevin

[realvictorprm]

I forgot about that thingy. I think this can be considered as long hanging fruit with it being already quite reachable because of the existing progress. I can't promise anything but I'll try to find some time this year to look into it and try to update it and get it working even more.

[KevinRansom]

@realvictorprm, I am going to close this as an orphaned PR. please make a branch in your fork if you want to revisit this PR. We would welcome a PR, when you have something ready for us to work with.

Thanks everyone for your attention to this contribution.

Kevin