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

Diagnostics: warn if a with-operation updates all values #603

Open
0x53A opened this Issue Aug 29, 2017 · 15 comments

Comments

Projects
None yet
5 participants
@0x53A
Contributor

0x53A commented Aug 29, 2017

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

This comment has been minimized.

Show comment
Hide comment
@0x53A

0x53A Aug 29, 2017

Contributor

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

Contributor

0x53A commented Aug 29, 2017

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

@matthid

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Aug 29, 2017

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

matthid commented Aug 29, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Aug 29, 2017

Collaborator

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

Collaborator

dsyme commented Aug 29, 2017

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

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Aug 29, 2017

Collaborator

Note adding this diagnostic doesn't need an RFC

Collaborator

dsyme commented Aug 29, 2017

Note adding this diagnostic doesn't need an RFC

@matthid

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Aug 29, 2017

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

matthid commented Aug 29, 2017

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

@Rickasaurus

This comment has been minimized.

Show comment
Hide comment
@Rickasaurus

Rickasaurus Aug 29, 2017

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.

Rickasaurus commented Aug 29, 2017

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 comment has been minimized.

Show comment
Hide comment
@0x53A

0x53A Aug 30, 2017

Contributor

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

Contributor

0x53A commented Aug 30, 2017

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

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Aug 30, 2017

Collaborator

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

Collaborator

dsyme commented Aug 30, 2017

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

@Rickasaurus

This comment has been minimized.

Show comment
Hide comment
@Rickasaurus

Rickasaurus Aug 31, 2017

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.

Rickasaurus commented Aug 31, 2017

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

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Aug 31, 2017

Collaborator

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

Collaborator

dsyme commented Aug 31, 2017

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

This comment has been minimized.

Show comment
Hide comment
@Rickasaurus

Rickasaurus Sep 1, 2017

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.

Rickasaurus commented Sep 1, 2017

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

This comment has been minimized.

Show comment
Hide comment
@vasily-kirichenko

vasily-kirichenko Sep 1, 2017

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

vasily-kirichenko commented Sep 1, 2017

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

@Rickasaurus

This comment has been minimized.

Show comment
Hide comment
@Rickasaurus

Rickasaurus Sep 1, 2017

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.

Rickasaurus commented Sep 1, 2017

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.

@0x53A

This comment has been minimized.

Show comment
Hide comment
@0x53A

0x53A Sep 1, 2017

Contributor

Even if this is added, you could just suppress the warning globally and go back to the previous status quo.

Contributor

0x53A commented Sep 1, 2017

Even if this is added, you could just suppress the warning globally and go back to the previous status quo.

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Sep 1, 2017

Collaborator

Yes,you'd just have to suppress the warning :)

Collaborator

dsyme commented Sep 1, 2017

Yes,you'd just have to suppress the warning :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment