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

Allow open type on RequireQualifiedAccess DUs/records #1127

Closed
5 tasks done
NinoFloris opened this issue Apr 5, 2022 · 9 comments
Closed
5 tasks done

Allow open type on RequireQualifiedAccess DUs/records #1127

NinoFloris opened this issue Apr 5, 2022 · 9 comments

Comments

@NinoFloris
Copy link

NinoFloris commented Apr 5, 2022

I propose we allow DUs/records annotated with RequireQualifiedAccess to have their symbols brought into scope with open type. Modules annotated with RQA (RequireQualifiedAccess) would still stay exempt from this relaxation.

This would need some discussion as to what open type would bring into scope for DUs and records, just the cases/labels? Or potentially less desirable overall - but more consistent with current open type semantics - also their static members?

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

Today you choose to accept the symbol pollution or you apply RQA to forever disallow anybody bringing these symbols into scope again. Neither are 'always' the best way to go.

Pros and Cons

The advantages of making this adjustment to F# are:

  • Additional, easily auditable, flexibility when using types annotated with RQA.
  • Increased orthogonality for open type. I tried using it on a DU somewhat expecting/hoping it would already work today.

The disadvantages of making this adjustment to F# are:

  • Increased complexity if we end up with inconsistent semantics for open type, pending discussion.
  • Some people may want to prevent even open type from bringing RQA'ed symbols into scope, please add a comment if so.

Extra information

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

Related suggestions: #1090

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
  • I or my company would be willing to help implement and/or test this

For Readers

If you would like to see this issue implemented, please click the 👍 emoji on this issue. These counts are used to generally order the suggestions by engagement.

@cartermp
Copy link
Member

cartermp commented Apr 5, 2022

Or potentially less desirable overall - but more consistent with current open type semantics - also their static members?

If we were to do this then I think we'd bring the labels into scope, like today without RQA.

I think this is probable also in the XS category, since this was explicitly designed for and would probably just involve removing an if check in a few places, like so: https://github.com/dotnet/fsharp/blob/main/src/fsharp/NameResolution.fs#L1200-L1224

@cartermp
Copy link
Member

cartermp commented Apr 5, 2022

I think I'm in favor of this. I guess it's just a question of which should "win", but I believe it makes more sense to let open type win because it's an opt-in mechanism and so it doesn't really violate the intent of the attribute.

@LyndonGingerich
Copy link

The precision control would be very handy, but it may not offset the added conceptual complexity. It would seemingly make most sense either to have RQA by default and be able to turn it off (for example, with open type) or to have RQA disabled by default with the ability to enable it (for example, with the tag, as in the current implementation). Having RQA disabled by default, with the option of adding the attribute, and then later the choice of opening the type to disable it again, seems overcomplicated.

@realparadyne
Copy link

If it were the same person making those choices all within the same project, it might be overcomplicated. But if a library author makes the RQA choice because it makes sense for 'most users' and then someone else (with no control over that library) could really benefit from overriding it for a special case then it seems straightforward instead to have 'open RQAType' as a way to force it despite the RQA.

@cartermp
Copy link
Member

@realparadyne Yes, this is largely why I'm in favor of this change. And TBH I don't think this is terribly complicated either. I can think of many ways you can utilize existing, single features that are much more complicated than this.

@auduchinok
Copy link

auduchinok commented Apr 11, 2022

If approved, let's make this behaviour different from open type where static members are imported, to reduce complexity? It could be something like this:

[<RequireQualifiedAccess>]
type Record =
    { Field: int }

    static member Prop = 1

open Record      // imports `Field`
open type Record // imports `Prop`

@cartermp
Copy link
Member

@auduchinok I think that would be even more confusing, since open type already exposes DU cases without RQA

//[<RequireQualifiedAccess>]
type DU =
    | ACase of int

    static member Prop = 1


open type DU // imports `Prop`


ACase |> ignore
Prop |> ignore

Requiring something different from open type when RQA is active would be too difficult to keep track of IMO. I'd much rather just bring this into open type.

@l3m
Copy link

l3m commented Apr 11, 2022

I feel it might increase the cognitive load when reading real-world code. Right now, RQA is simple and consistent: you need to use the type name before the DU-case.

Too many exceptions and special cases add up. To me, this seems like something that people would be dissuaded from using in coding conventions.

@dsyme
Copy link
Collaborator

dsyme commented Jun 16, 2022

Having RQA disabled by default, with the option of adding the attribute, and then later the choice of opening the type to disable it again, seems overcomplicated.

I agree with this. RQA means what it says, opting in to it then suppressing it seems too layered.

I will close this as a result

@dsyme dsyme closed this as completed Jun 16, 2022
@dsyme dsyme reopened this Jun 16, 2022
@dsyme dsyme closed this as not planned Won't fix, can't repro, duplicate, stale Jun 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants