Diagnostics: warn if a with-operation updates all values

[0x53A]

Diagnostics: warn if a with-operation updates all values

I propose we ... warn if a with-operation updates all values.

Consider the following snippet:

type MyRecord = { A : int list ; B : string list }
let combine a b = { a with A = a.A @ b.A ; B = a.B @ b.B }

combine { A = [ 1 ] ; B = [ "2" ] }  { A = [ 1 ] ; B = [ "2" ] }

If I now update the type MyRecord and add a third field, the compiler will guide me to adapt the two constructors, but will NOT guide me to adapt combine.

type MyRecord = { A : int list ; B : string list ; C : char list }
let combine a b = { a with A = a.A @ b.A ; B = a.B @ b.B } // <- C is missing

combine { A = [ 1 ] ; B = [ "2" ] ; C = [] }  { A = [ 1 ] ; B = [ "2" ] ; C = [ '3' ] }

Using a with doesn't really make sense in this situation, because all values are updated anyway, but I have often seen this pattern in the wild! So I would want this warning to make later refactorings easier. Note that this warning would not be issued after adding the third field, because by that time, the with would no longer update all fields.

The existing way of approaching this problem in F# is ...

Pros and Cons

The advantages of making this adjustment to F# are ...

Makes it easier to find places to adapt after adding a record field.

The disadvantages of making this adjustment to F# are ...

Perfectly fine code may produce this diagnostic, but the fix is easy and will improve the code base.
Any additional warning may break code that currently compiles if warn-as-errors is enabled. I think it would be worth it.

Extra information

Estimated cost (XS, S, M, L, XL, XXL): XS-S

Related suggestions: (put links to related suggestions here)

Affidavit (please submit!)

Please tick this by placing a cross in the box:

• This is not a question (e.g. like one you might ask on stackoverflow) and I have searched stackoverflow for discussions of this issue
• I have searched both open and closed suggestions on this site and believe this is not a duplicate
• This is not something which has obviously "already been decided" in previous versions of F#. If you're questioning a fundamental design decision that has obviously already been taken (e.g. "Make F# untyped") then please don't submit it.

Please tick all that apply:

• This is not a breaking change to the F# language design
• Any additional warning may break code that currently compiles if warn-as-errors is enabled. I think it would be worth it.
• I or my company would be willing to help implement and/or test this

[0x53A]

Note: I strongly assume that this is the root cause of fsprojects/Paket#2687, so this does actually cause bugs to slip-through.

[matthid]

@0x53A note that fsprojects/Paket#2687 has nothing to do with your suggestion. I was only asking if there might be another bug (but not fsprojects/Paket#2687 itself).

I like the suggestion and think it is sound. However I also think that people use the with syntax to guide type inference...

[dsyme]

Seems reasonable, even for the compiler (rather than FSharpLint)

[dsyme]

Note adding this diagnostic doesn't need an RFC

[matthid]

That was a fast approved, now we only need a PR ;)

[Rickasaurus]

We use the pattern { MyType.Default with ... } quite a lot and I think this change would be rather annoying for us. Of course it's ideal from a safety perspective to always make a new record by hand, but that can get very aggravating when dealing with 10+ member records, especially in tests.

[0x53A]

This warning would only trigger if you updated all 10+ fields in the same with expression, and then MyType.Default would not really do anything anyway. It would not affect the normal usage case where you only update a few fields.

To remove the warning, you would only have to remove MyType.Default with

[dsyme]

@Rickasaurus Yes, I think if you're replacing all the fields then you'd always want a warning to that effect

[Rickasaurus]

So, when adding a field to a large record update we now may need to make a change far up in the file to avoid a warning. Something additional to worry about in a merge that's really just noise.

Would make for a nice suggestion refactoring in an IDE for sure.

[dsyme]

So, when adding a field to a large record update we now may need to make a change far up in the file to avoid a warning. Something additional to worry about in a merge that's really just noise.

@Rickasaurus Only if you were actually not making a record update at all, but actually replacing all values in the record. Why wouldn't you want a warning when all the information in the original record in {a with ... } is effectively being lost/replaced? We give warnings in F# when other information is thrown away

[Rickasaurus]

I don't think we've ever had an issue stem from this.

Let me paint out the scenario that worries me. We have a lot of record-only nuget packages for communicating with our databases. Let's say some field gets removed because we decide we want to do this thing a different way in the database and it wasn't used in most places anyway. Now we see meaningless warnings in all 50 some odd downstream projects and tests where we were using MyType.Default with for convenience. Suddenly we have a big cleanup effort because we like warning-free builds.

[vasily-kirichenko]

@Rickasaurus So you don't want to clean up your code?..

[Rickasaurus]

I don't consider this particularly dirty. In some ways it's cleaner as it keeps the diffs minimal.

I'm not going to freak out or anything if you all decide to go ahead with this anyway. I just wanted to weigh in with our perspective as a team with a huge number of projects to manage.