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

A code refactoring to convert bool to a Yes|No DU #15566

Open
Tracked by #15561
psfinaki opened this issue Jul 5, 2023 · 3 comments
Open
Tracked by #15561

A code refactoring to convert bool to a Yes|No DU #15566

psfinaki opened this issue Jul 5, 2023 · 3 comments
Labels
Area-LangService-CodeRefactorings Suggested but not necessary code actions. Screwdrivers in VS. Feature Request
Milestone

Comments

@psfinaki
Copy link
Member

psfinaki commented Jul 5, 2023

Update: actually turns out there are more ideas. See my original idea below and even more original ideas of kerams even more below.


Convert

type T = T of bool

to

[<RequireQualifiedAccess>]
type T = 
   | Yes
   | No

A true functional fix :)

Originally posted by @kerams in #15561 (comment)

@github-actions github-actions bot added this to the Backlog milestone Jul 5, 2023
@psfinaki psfinaki added the Area-LangService-CodeRefactorings Suggested but not necessary code actions. Screwdrivers in VS. label Jul 5, 2023
@kerams
Copy link
Contributor

kerams commented Jul 6, 2023

No, no, I meant

let CheckIWSAM (cenv: cenv) (env: TcEnv) checkConstraints (checkIwsam: bool) m tcref =
    let g = cenv.g
    let ty = generalizedTyconRef g tcref

    if checkIwsam && isInterfaceTy g ty && checkConstraints = CheckCxs then
        let meths = AllMethInfosOfTypeInScope ResultCollectionSettings.AllResults cenv.infoReader env.NameEnv None env.eAccessRights IgnoreOverrides m ty
        ()

let TcLongIdentType kindOpt (cenv: cenv) newOk checkConstraints occ iwsam env tpenv synLongId =
    let (SynLongIdent(tc, _, _)) = synLongId
    let m = synLongId.Range
    let ad = env.eAccessRights

    CheckIWSAM cenv env checkConstraints true m tcref

->

[<RequireQualifiedAccess>]
type CheckIwsam =
    | Yes
    | No

let CheckIWSAM (cenv: cenv) (env: TcEnv) checkConstraints (checkIwsam: CheckIwsam) m tcref =
    let g = cenv.g
    let ty = generalizedTyconRef g tcref

    if checkIwsam = CheckIwsam.Yes && isInterfaceTy g ty && checkConstraints = CheckCxs then
        let meths = AllMethInfosOfTypeInScope ResultCollectionSettings.AllResults cenv.infoReader env.NameEnv None env.eAccessRights IgnoreOverrides m ty
        ()

let TcLongIdentType kindOpt (cenv: cenv) newOk checkConstraints occ iwsam env tpenv synLongId =
    let (SynLongIdent(tc, _, _)) = synLongId
    let m = synLongId.Range
    let ad = env.eAccessRights

    CheckIWSAM cenv env checkConstraints CheckIwsam.Yes m tcref

This is obviously much more complicated as we'd need to rewrite the definition site and all call sites.

@vzarytovskii
Copy link
Member

Convert

type T = T of bool

to

[<RequireQualifiedAccess>]
type T = 
   | Yes
   | No

A true functional fix :)

Originally posted by @kerams in #15561 (comment)

Thats not very useful. It will be much more useful to remove boolean parameter and use newly introduced DU though.

However, it's a common pattern in compiler, but I rarely see it elsewhere, so not sure about overall usefulness.

Update: @kerams beat me to it.

@psfinaki
Copy link
Member Author

psfinaki commented Jul 6, 2023

Ohhhh I see. Alright, sorry for the confusion then. :)

Well I personally generally dislike whatever booleans wherever since they hinder readability on a calling side (bold statement I know). So probably both ideas could be worthy.

I'd wait for more people to chime in here with their opinions before splitting the ticket and jumping on it. Anyway, thanks for good examples up there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-LangService-CodeRefactorings Suggested but not necessary code actions. Screwdrivers in VS. Feature Request
Projects
None yet
Development

No branches or pull requests

4 participants