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

Warning FS3560 triggered upon nested update even if other fields of nested record are copied #16995

Closed
reinux opened this issue Apr 5, 2024 · 5 comments
Labels
Milestone

Comments

@reinux
Copy link

reinux commented Apr 5, 2024

Repro steps

type Q = { x: int; y: string }
type T = { q: Q }
let a = { q = { x = 5; y = "abc" } }
let b = { a with q.x = 5 }

Expected behavior
No warning

Actual behavior

"This copy-and-update record expression changes all fields of reecord type 'T'. Consider using the record construction syntax instead.

Known workarounds
<NoWarn>FS3560</NoWarn> in .fsproj

Related information

  • Operating system: Linux (NixOS Unstable)
  • .NET Runtime kind: .NET Core
  • Editing Tools: Rider 2023.3.4
@github-actions github-actions bot added this to the Backlog milestone Apr 5, 2024
@edgarfgp
Copy link
Contributor

edgarfgp commented Apr 6, 2024

I'm not able to reproduce this on main with withLangVersion80 or withLangVersionPreview.

Edit: Adding some extra tests for this could be useful

Edit 2: Looks like this warning is disabled by default(It was not like that in the past).

Anyway. Yeah it can be reproduced. See:

[<Fact>]
let ``Warning FS3560 triggered upon nested update even if other fields of nested record are copied`` () =
        Fsx """
type Q = { x: int; y: string }
type T = { q: Q }
let a = { q = { x = 5; y = "abc" } }
let b = { a with q.x = 5 }
        """
        |> withLangVersion80
        |> withOptions ["--warnon:FS3560"]
        |> typecheck
        |> shouldFail
        |> withDiagnostics [
            (Warning 3560, Line 5, Col 9, Line 5, Col 27, "This copy-and-update record expression changes all fields of record type 'Test.T'. Consider using the record construction syntax instead.")
        ]

@kerams
Copy link
Contributor

kerams commented Apr 7, 2024

Why do you think this is a bug?

@brianrourkeboll
Copy link
Contributor

brianrourkeboll commented Apr 8, 2024

Why do you think this is a bug?

Yeah, I guess technically the message is correct in that

{ a with q.x = 5 }

could be

{ q = { a.q with x = 5 } }

But I can also see why someone would view copying-and-updating the two nested records in this case as a single copy-and-update operation (now that that's possible), rather than as an outer construction operation and an inner copy-and-update operation.

@abonie
Copy link
Member

abonie commented Apr 8, 2024

We should remove this warning and do what the warning says during code-gen or optimization

@abonie
Copy link
Member

abonie commented Apr 8, 2024

Closing, since we plan to address it in a different manner. Btw this warning is disabled by default as Edgar said (except in some old version of the SDK)

@abonie abonie closed this as completed Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

No branches or pull requests

5 participants