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

[RFC FS-1043] Extension members visible to trait constraints #6805

Draft
wants to merge 279 commits into
base: main
Choose a base branch
from

Conversation

dsyme
Copy link
Contributor

@dsyme dsyme commented May 22, 2019

Continuation of #6286 from a feature branch

RFC https://github.com/fsharp/fslang-design/blob/master/RFCs/FS-1043-extension-members-for-operators-and-srtp-constraints.md

This is work by @TobyShaw and myself to implement RFC FS-1043. This PR brings #3582 up-to-date with master

Design points:

  • determine any necessary/desired changes to SRTP resolution

Things to test

Related bugs to investigate:

@dsyme dsyme changed the title [WIP, RFC FS-1043] Extension members visible to trait constraints [RFC FS-1043] Extension members visible to trait constraints Jul 5, 2019
@abelbraaksma
Copy link
Contributor

I'd love to see this in, but I'm wondering about the rfc status. It appears empty, and points to the previous PR. Is there another location that has a summary of scope and features?

@cartermp
Copy link
Contributor

@abelbraaksma this won't be merged until an RFC is written. The current one has no design.

@abelbraaksma
Copy link
Contributor

Thanks, I'd love to help, but I may not know enough of the internals to write a quality rfc pr

@realvictorprm
Copy link
Contributor

Anything we can do about the RFC?

@realvictorprm
Copy link
Contributor

@dsyme is it possible that this will be merged for F# 5 after you were able to fix some bugs with the SRTP's?

@dsyme
Copy link
Contributor Author

dsyme commented Jan 27, 2020

Anything we can do about the RFC?

A sketch draft of the RFC is complete. It's not actually a very complicated change, but there are some subtle points in the interaction with other elements of logic in the compiler

@dsyme
Copy link
Contributor Author

dsyme commented Jan 27, 2020

@dsyme is it possible that this will be merged for F# 5 after you were able to fix some bugs with the SRTP's?

A key thing is to continue to improve testing, for

  1. Common scenarios of expected positive usage (cases where this construct makes code simpler and better). Roughly speaking this is where instances are provided to augment existing types with witnesses for existing SRTP-constraints implied by FSharp.Core (mainly arithmetic operators).

  2. Cases related to the technical interactions described in the RFC and any other technical interactions we expect

  3. Cases covering the "whackier edge cases" where we have previously observed subtle change in SRTP resolutions.

Note I'm not particularly interested in this change supporting the SRTP-laden code that does (largely pointless) over-abstraction of code in the name of "maximal sharing" or simulating category theory - this tends to serve little actual value and makes code highly obscure, and SRTP was never designed for these purposes. Indeed if possible I'd prefer to disallow or deprecate that kind of code.

@smoothdeveloper
Copy link
Contributor

@dsyme

over-abstraction of code in the name of "maximal sharing" or simulating category theory

I can share the feeling, but at the same time, the type of code you are refering to is coming to existence (even in a coming PR to fsharp core) because F# currently lacks the constructs enabling such code to be written with more adequate idioms than SRTP.

see usage of ^TaskLike and Priority1, 2 and 3 in this code from the Task Support PR:

interface IPriority1
interface IPriority2
interface IPriority3

As for the usefulness of SRTP, it is very useful and critical, here is a single SRTP function that I've defined and is critical in my day work codebase:

let inline CreateCommand (createCommand: unit -> SqlCommand) : 'a =
  let dummyCommand = createCommand()
  let cx = dummyCommand.Connection
  let tx = dummyCommand.Transaction
  let timeout = dummyCommand.CommandTimeout
  let command = (^a : (new : SqlConnection * SqlTransaction * int -> ^a) (cx, tx, timeout)) // SRTP abstracting over
  // possibly some more stuff
  command

(NB: I don't use SRTP constructs casually in my work codebase)

This is to work around design choices made in a TP library where I don't have control on constructor and definitely don't want useless boiler plate code to show up at every instanciation of hundreds of disparate types.

The normal "legit" ways to handle this with the vanilla OO polymorphism just don't work (especially for type constructors), the usage of reflection comes with lots of issues, and the zero cost abstraction and safety that SRTP enables works strongly to support the great aspects of F# design, even if for SRTP, it is better burried deep down in the implementation of a library.

My point is to show that between SRTP enabling critical abstractions (like numeric operators you mentionned or the function I was showing) and what is over abstraction, it really depends the context.

Other languages like C++ and Rust have similar features (with worse or better syntax / idioms) that enable compile time monomorphization, and F# having some support for it is a great thing, despite all the bad rep of SRTP syntax.

AFAIU, C++ will someday having the concept of concepts, and Rust uses traits, those approach enable better user story in terms of error messages, which in case of C++ template code and F# SRTP, tend to produce error messages that aren't conveying the likely cause of an issue.

All that being said, this very PR, AFAIU, is going to enable haskell type class like approach where the instance can be defined outside of the type or the abstraction, so I won't be surprised if this turns out into new idioms developing until there is support for "Extend everything" from C# land, or new and more elaborated type constructs landing in the CLR.

@dsyme
Copy link
Contributor Author

dsyme commented Jan 27, 2020

As for the usefulness of SRTP, it is very useful and critical, here is a single SRTP function that I've defined and is critical in my day work codebase...

Yes, I'm aware of complex new SRTP constraints being used to capture the TaskLike pattern in RFC FS-1072 #6811 - this is something I'm not totally happy about and likely wouldn't have authored, but has been brought through the initial desire to base the work on TaskBuilder.fs.

Regarding this code:

let inline CreateCommand (createCommand: unit -> SqlCommand) : 'a =
  let dummyCommand = createCommand()
  let cx = dummyCommand.Connection
  let tx = dummyCommand.Transaction
  let timeout = dummyCommand.CommandTimeout
  let command = (^a : (new : SqlConnection * SqlTransaction * int -> ^a) (cx, tx, timeout)) // SRTP abstracting over
  // possibly some more stuff
  command

Could you show some callsites? I presume they need type annotations, e.g.

let x : SomeCommandType = CreateCommand (fun () -> ...)

But how bad is it if you pass the constructor in explicitly?

let x = CreateCommand  SomeCommandType (fun () -> ...)

Anyway, this RFC is not removing the ability to write user-defined SRTP constraints, I'm just trying to scope intended use cases for the whole mechanism if we need to make tradeoffs for compat etc. It would be a separate RFC to make some adjustment (e.g. add a warning for any user-defined SRTP constraint, or add a warning for user-defined two-type-parameter SRTP constraints, or something). I'm far from deciding what the principles for anything like this would be.

@dsyme
Copy link
Contributor Author

dsyme commented Jan 27, 2020

Right now I'm trying to determine when/if this whole RFC is a breaking change.

  1. I'm assuming it must be a breaking change, because additional methods are taken into account in the overload resolution used in SRTP constraint resolution. That must surely cause it to fail where it would have succeeded before.

  2. However, all the new methods are extension methods, which are lower priority in overload resolution

  3. Also, it's hardly the worse kind of breaking change, because it's a lot like an addition to the .NET libraries causing overload resolution to fail. We don't really consider that a breaking change (partly because this is measured differently for C# and F#, as they have different sensitivities to adding overloads).

  4. Also, it's not the worst breaking change because we give warnings when users try to add extension members for operators like +

  5. Also, nearly all SRTP constraints are on static members, and C# code can't introduce extension members that are static - just instance ones. So for C# extension members to cause a problem to arise we'd have to have F# code introducing SRTP constraints on instance members

Still, I'm sure this must be a breaking change. I would actually appreciate help trying to construct a case where it is if anyone has a spare evening :)

@dsyme
Copy link
Contributor Author

dsyme commented Jan 27, 2020

Added notes on the compat concerns here: fsharp/fslang-design#430

@dsyme
Copy link
Contributor Author

dsyme commented Jan 27, 2020

I started to add some compatibility testing here: f3901d0

@dsyme
Copy link
Contributor Author

dsyme commented Jan 27, 2020

I would like some contributions of the following if you can help...

Please list the best examples (if any) that you know of for these:

  1. Cases where a .NET Framework type (other than a primitive) doesn't support a particular method corresponding to an existing SRTP constraint used in FSharp.Core. For example, is missing op_Addition or some other operator or method (Sin, Cos, Zero, One, etc.)

  2. Cases of fresh SRTP code (i.e. using user-defined SRTP constraint calls) that captures common existing patterns in .NET libraries like TryGetValue or the like (and especially cases where it would be highly awkward to pass the witnesses to these patterns manually).

  3. Cases of fresh SRTP code can define reasonable and simple new patterns that you might like to retrofit on to a range of existing or new .NET or F# types. For example, patterns you might reasonably try to retrofit a pattern over all collections, or all code to undergo JSON serialization etc. (I'm especially interested in cases where it would be highly awkward to pass the witnesses to these patterns manually).

I'm after examples of good SRTP code capturing useful patterns, used in a way that makes code simpler end to end (convince me!), if this feature were supported.

@smoothdeveloper
Copy link
Contributor

Could you show some callsites? I presume they need type annotations, e.g.

Right, it looks like this

let command : MyCommandType = CreateCommand createCommand

But how bad is it if you pass the constructor in explicitly?

let command = CreateCommandWithCtor createCommand (fun (a,b,c) -> new MyCommandType(a,b,c))

In my case I find it would be prohibitive to fallback to write this (or rather discourage the use of function I used as example), the only work around is reflection (not sure that even works with erased provided types I'm using here) which would offer no compile time guarantees as to which type argument is passed.

There is also the risk, by doing what is shown in that SRTP-less code, that if I need to call another constructor later on, I have to go over all those lambdas and shuffle things, that is many callsites, instead of just changing that single infrastructure function; also prohibitive.

With SRTP approach, I enforce only proper types are constructed and the invariants in my CreateCommand with practically zero noise and zero cost beside compile time verification of the constraint.

This is for me, compelling use of SRTP, so I don't have to review code which would be missusing or missing critical setters calls on the object and the invariants baked in the function, the syntax to use the function is actually the smallest thing a developer has to write with nothing competing as more convenient which could be faulty / needs more time reviewing.

(This is code using http://fsprojects.github.io/FSharp.Data.SqlClient/)

Thanks a lot for the clarifications and inviting to contribute valuable SRTP samples!

@robkuz
Copy link

robkuz commented Jan 27, 2020

Sorry I have no code here - but I will do some blog posts when this becomes available - comparing the different approaches.

I have a deeply nested object graph (8 - 10 nested types before the whole thing becomes recursive) that I need to (de)JSONify - however I have usually between 4-5 representations of JSON for the same data structure.

The most common approach for this would be to encode each JSON representation and the corresponding types as Json-types . however this would yield an explosion of similar types and the corresponding FromType and ToType methods/functions.
The benefit is of course that I could use Newtonsofts auto serializer. However that is not wanted as I need finegrained (non-automatic) conversions and I need to be able to read older versions of JSON without breaking the system. This is really important as we are handling tax-data and everybody is very sensitive to schema changes.

At the moment I have settled for a solution like the new System.Text.Json from .net core. That is I build a big registry of JsonReaders and JsonWriters with a lookup of typeof<_> and some additional key. It works and it is stable but basically all type safety was thrown out of the window. Therefore all calling code needs to permantly check if we got an error (usually because I converter couldnt be found).
when in reality the compiler should be perfectly able to determine if for a given type (and that includes nested types in an object graph) there are converters in scoper or not.

The code that exists atm is roughly 7KLOC and I am sure I could strip away 30% if not 50% if I had something akin to TCs.
The downside would be some potentially scary error messages by the compiler (usually something like a screen full of overloaded messages that do not fit). However having used and abused SRTPs (and also extension methods in general) for the last 3 1/2 years I dont see that as a real problem. After 2-3 times one encounters such intimidating messages one gets an intuition what to do about it.
plus nobody is forced to use it. Which is great.

@cartermp
Copy link
Contributor

IMO the level of breaking change here is worth taking:

  • It makes a material, positive change towards making F# more expressive for DSLs and scenarios where you do arithmetic on custom types
  • It is roughly the same as .NET adding a new overload, as was one with Span<'T> when introduced across the entire framework

@edgarfgp
Copy link
Contributor

edgarfgp commented Jan 1, 2023

Would be good to try to fix the merge conflicts before the diff gets too big .

@laenas
Copy link
Contributor

laenas commented Feb 13, 2023

Just noting that there's some consequential overlap with fsharp/fslang-suggestions#1039 here - since it expands the ability to incidentally share names with properties, causing potential resolution issues.

@vzarytovskii
Copy link
Member

This pretty much awaits on C#'s decision on extensions, so we don't do work twice in regards to interop.

@serefarikan
Copy link

@vzarytovskii just curious, can you briefly explain which decision you're referring to on the c# side?

@vzarytovskii
Copy link
Member

@vzarytovskii just curious, can you briefly explain which decision you're referring to on the c# side?

When an how will C# implement extensions. So interop will be less painful and we can design around their feature too.

@vzarytovskii
Copy link
Member

Otherwise if we implement it now, we may have to do support for their extensions again, and a. Inch of stuff may overlap. Going just to be double the work.

@serefarikan
Copy link

Thanks! I think my confusion comes from thinking "c# already supports extension methods?" 😄 You seem to be referring to something more advanced on the C# side (which they'll partially take from F# probably...) to which you'll have to align with again. Did I get it right?

@vzarytovskii
Copy link
Member

Thanks! I think my confusion comes from thinking "c# already supports extension methods?" 😄 You seem to be referring to something more advanced on the C# side (which they'll partially take from F# probably...) to which you'll have to align with again. Did I get it right?

Yeah, it's a new feature - extensions.
Last spec I've seen is here: https://github.com/dotnet/csharplang/blob/main/proposals/extensions.md

I am not sure if there will be lots of overlap, but might be a bunch.

I guess I should've phrased my initial comment differently: we aren't blocked by C# feature, but whoever will continue working on it should keep in mind C# spec and make a decision/suggestion whether we want to wait for their implementation or not.

@serefarikan
Copy link

That explains it @vzarytovskii , much appreciated, thanks!

@edgarfgp
Copy link
Contributor

I think would be good to keep this branch without conflicts. So whoever continues this will have a chance :)

@gusty
Copy link
Contributor

gusty commented Oct 31, 2023

Agreed with @edgarfgp also I had a look at C# extensions, thanks @vzarytovskii for the link I wasn't aware of that proposal at all, and it looks like it's not overlapping with this because it's about type extensions (as opposed to static member extensions) and it's not about SRTPs, so I don't see where a conflict/overlap might arise.

@vzarytovskii
Copy link
Member

Agreed with @edgarfgp also I had a look at C# extensions, thanks @vzarytovskii for the link I wasn't aware of that proposal at all, and it looks like it's not overlapping with this because it's about type extensions (as opposed to static member extensions) and it's not about SRTPs, so I don't see where a conflict/overlap might arise.

Oh it likely will, since we'll need to be supporting SRTPs via new extensions.

@gusty
Copy link
Contributor

gusty commented Oct 31, 2023

I see what you mean, yes, but it seems to me that once you get SRTPs to work with static members, making them working on type extensions shouldn't be that complicated, specially considering that type extension should also support interfaces, IWSAMs which BTW can be used as solutions for SRTPs, ... just my 2 cents.

@vzarytovskii
Copy link
Member

I think would be good to keep this branch without conflicts. So whoever continues this will have a chance :)

It will be additional housekeeping for something we don't know when we will continue. I.e someone will need to go and explicitly look at it every week (day?, couple of days?), go and merge it with understanding of the change.

I would say the usual approach would be easier - re-apply changes on top of main once we work on it. History shows that's the easiest approach since implementation changes oftentimes (like now - we will be needing to take care of iwsams too probably, and later we will need to be supporting extensions in srtps too).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: New
Development

Successfully merging this pull request may close these issues.