Skip to content

Warn when record update expression changes all fields#14673

Merged
vzarytovskii merged 9 commits into
dotnet:mainfrom
kerams:warn
Feb 1, 2023
Merged

Warn when record update expression changes all fields#14673
vzarytovskii merged 9 commits into
dotnet:mainfrom
kerams:warn

Conversation

@kerams

@kerams kerams commented Jan 29, 2023

Copy link
Copy Markdown
Contributor

Implements fsharp/fslang-suggestions#603.

There has been some opposition to the suggestion. Should this maybe be off by default or part of warning level 4?

@kerams kerams requested a review from a team as a code owner January 29, 2023 12:57
@kerams

kerams commented Jan 29, 2023

Copy link
Copy Markdown
Contributor Author

Build failures stem from this diagnostic being triggered when compiling FSharp.Core:

/// Produce a new execution context for a composite async
member _.WithContinuations(cont, econt) =
    AsyncActivation<'U> // <------------- ok, 2 fields, construction syntax
        {
            cont = cont
            aux = { contents.aux with econt = econt }
        }

/// Produce a new execution context for a composite async
member ctxt.WithContinuations(cont, econt, ccont) =
    AsyncActivation<'T>
        { contents with // <------------- warning, 2 fields, update syntax
            cont = cont
            aux =
                { ctxt.aux with
                    econt = econt
                    ccont = ccont
                }
        }

@vzarytovskii

Copy link
Copy Markdown
Member

Can you please put it off by default in current versions and on under preview version? This way, I guess, we can have it off now, but optionally enabled if users want it, and on by default in preview/8?

Comment thread src/Compiler/Checking/CheckExpressions.fs Outdated
@psfinaki

Copy link
Copy Markdown
Contributor

@kerams you are really doing an amazing job with these PRs, thanks a lot. Considering the level of the warning, IMO can be whatever since yeah we don't expect this to happen often to be anyhow noisy.

@vzarytovskii

Copy link
Copy Markdown
Member

@kerams you are really doing an amazing job with these PRs, thanks a lot. Considering the level of the warning, IMO can be whatever since yeah we don't expect this to happen often to be anyhow noisy.

It should not be enabled by default for current language versions, a rule of thumb is usually "if it will break anyone's compilation simply by upgrading to a minor SDK, then it go under language flag"

@T-Gro T-Gro left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the new approach via the exception type - it can be also reused for other scenarios where we want the same ( combination of langfeature + still allowing explicit opt-in for older versions).

Good.

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

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

5 participants