-
Notifications
You must be signed in to change notification settings - Fork 831
[FS-1031] Allow implementing the same interface at different generic instantiations in the same type #2867
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
Conversation
107c007 to
fb91410
Compare
6a674bc to
ae93ccd
Compare
|
Thanks @0x53A! It would be great to see this feature merged. |
|
Me too ;D There is a little bit of clean-up missing, I mainly paused because at that time there seemed to be a feature-freeze. @cartermp when is the cut-off date for F#4.2? Or to be more precise, what is the earliest date this could be merged? I didn't want to have to rebase it for months, that's why I stopped. |
There's no specific cut off as yet.
I'm afraid that's just what needs to be done to keep language additions in the pipe. That said, you have almost no conflicts so a merge+update should be easy enough |
In general I agree, but if it is clear that it can't be merged, then it doesn't really make sense. Anyway, regarding this specific PR: I will plan some time this weekend to get it to a reviewable state. |
|
I agree with @dsyme and I don't have any cutoff in mind, either. At some point, as we keep collecting bug fixes in F# 4.1, we may want to rev, but I don't feel that way right now. Certainly not without a few features like this. |
|
@0x53A should I update this for you? I'm used to this stuff and so it's not a big effort for me 😄 |
|
understood @0x53A. You can always ping me and ask me for keeping this up-to-date. |
|
thanks @realvictorprm for the offer, it was not that the effort of doing it once was so much, more that the effort of doing it every other day for a few months was something I didn't bother to do ;-) |
|
I try to reduce this time with adding features @0x53A 😛 😄 |
src/fsharp/PostInferenceChecks.fs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked around for any suspiciously named methods (isObjectExpression and so on), but couldn't find anything. Help appreciated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Afaik this code path is only hit with classes, not OEs, but not sure if that is guaranteed
|
I would appreciate a review and additional positive as well as negative unit tests, especially combinations of measures and aliases and generics. |
|
@0x53A, do we have an RFC for this? I have trouble finding it. In fsharp/fslang-design#185 you link to a possible RFC FS-1031, but this link is dead (it goes into your own repo) and I don't seem to be able to find the "official" one, if any exists. Or does the language suggestion #545 in the original post count as RFC? I'm curious to the expected width of this submission, whether it puts extra strain on the type inference system (i.e., performance-wise), whether it influences type-inferenced signatures in quick-info tooltips or FSI feedback and whether there are certain restrictions we should be aware of. Also of interest is this subtle bug introduced in VS2015, also related to generic type inheritance, according to Don's comment there, the bug is now a feature (i.e., no rollback will be done), meaning that code snippet should stay the same (and perhaps be made into a unit test). As many out there, I have been bitten by this restriction in the past, so I very much hope to see it merged into master! |
|
@abelbraaksma https://github.com/fsharp/fslang-design/blob/master/RFCs/FS-1031-Allow%20implementing%20the%20same%20interface%20at%20different%20generic%20instantiations%20in%20the%20same%20type.md The scope of this RFC and PR is as small as possible - it just fixes the naming collision, and adapts the check which previously prohibited this. There are certainly other additions which would round this feature out (e.g. see fsharp/fslang-design#185 (comment)), but that would - if approved by don - be implemented independent of this. |
|
The recent simplification commits -- introduced a bug should produce this output: actually produces this output: |
* Update dependencies from https://github.com/dotnet/arcade build 20200626.2 (dotnet#9577) Microsoft.DotNet.Arcade.Sdk From Version 1.0.0-beta.20302.3 -> To Version 1.0.0-beta.20326.2 Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> * Improve perf for String.filter up to 3x (dotnet#9509) * Improve perf for String.filter 2-2.5x * Cleanup: remove "foo" etc in tests * Add tests for new execution path for LOH in String.filter * Change test string * String map performance improvement (dotnet#9470) * Simplify and improve perf of String.length * Improve performance of String.map * Revert "Simplify and improve perf of String.length" * Resolves dotnet#9470 (comment) * Lingering space * Change `String` to use `new` to clarify use of ctor * Add some better tests for String.map, add side-effect test * Add tests to ensure the mapping function is called a deterministically amount of times * Fix typo * Remove "foo" from String.map tests * Perf: String.replicate from O(n) to O(log(n)), up to 12x speed improvement (dotnet#9512) * Turn String.replicate from O(n) into O(log(n)) * Cleanup String.replicate tests by removing usages of "foo" * String.replicate: add tests for missing cases, and for the new O(log(n)) cut-off points * Improve String.replicate algorithm further * Add tests for String.replicate covering all lines/branches of algo * Fix accidental comment Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: Abel Braaksma <abel.online@xs4all.nl>
…ev16.7 Merge master to release/dev16.7
…e2.fs (dotnet#9516) (dotnet#9587) * Re-enabling tests from OperatorsModule1/2.fs (compile errors) * Fix compile errors in OperatorsModule1/2.fs, fix tests. Note tanh test comment. * Fix `tanh` test, ensure stable result on x86 vs x64 runtimes * Stop using exception AssertionException, so that test window shows useful info * Whitespace cleanup and redundant code removal * Cleanup spelling etc * Re-enabling int, int16, int32, int64, nativeint, incr, nullArg etc tests * Special-case floating-point assertion messages for higher precision output * Fix/update/add tests (some still failing) * Separate Checked tests, add & fix others, differentiate framework/bitness for some tests * Add branch for .NET Native (ignore cos test) * Resorting to comparing floats with a delta using Assert.AreNearEqual * Add some more tests Co-authored-by: Abel Braaksma <abel.online@xs4all.nl>
…e2.fs (dotnet#9516) (dotnet#9590) * Re-enabling tests from OperatorsModule1/2.fs (compile errors) * Fix compile errors in OperatorsModule1/2.fs, fix tests. Note tanh test comment. * Fix `tanh` test, ensure stable result on x86 vs x64 runtimes * Stop using exception AssertionException, so that test window shows useful info * Whitespace cleanup and redundant code removal * Cleanup spelling etc * Re-enabling int, int16, int32, int64, nativeint, incr, nullArg etc tests * Special-case floating-point assertion messages for higher precision output * Fix/update/add tests (some still failing) * Separate Checked tests, add & fix others, differentiate framework/bitness for some tests * Add branch for .NET Native (ignore cos test) * Resorting to comparing floats with a delta using Assert.AreNearEqual * Add some more tests Co-authored-by: Abel Braaksma <abel.online@xs4all.nl>
…ev16.7 Merge master to release/dev16.7
|
Thank you for taking care of this! |
|
Wow, amazing stuff! 👍 |
|
@0x53A , thank you for getting it done in the first place 👍 |
Suggestion
Details: under discussion
RFC: merged
This now compiles:
This also compiles:
This does NOT compile, and is an explicit non-goal in the RFC:
This is now pretty near finished.
Build is green
one unit test is partially commented out,
open questionwhether it should be supported. resolved: not supportedImplementation needs a little bit of tidying up
I want to change the error message, here is the one from C#:

REVIEW: can this exception ever be hit? https://github.com/Microsoft/visualfsharp/pull/2867/files/01ae6c0526354e38fcc958d1d187761733a58b1e#diff-5b9ab9dd9d7133aaf23add1048742031R1085