Open Static Classes - Updates #7405
Conversation
|
Relevant tests that likely need updating: https://github.com/dotnet/fsharp/pull/6807/files#diff-7ddc16298f74f66b9a43a440b355f771R692 |
This comment has been minimized.
This comment has been minimized.
src/fsharp/NameResolution.fs
Outdated
| (minfosNew, minfosCurrent) | ||
| ||> List.fold (fun minfos minfo1 -> | ||
| // Shadow methods with the same signature. | ||
| if minfos |> List.exists (fun minfo2 -> MethInfosEquivByNameAndSig Erasure.EraseAll true g amap m minfo1 minfo2) then |
There was a problem hiding this comment.
This looks quadratic in the number of method overloads? I suspect it's hard to escape that without massively over-complicating the code though. Other things in the compiler are also quadratic (though I think it's less common to be quadratic when consuming a construct.)
There was a problem hiding this comment.
We should consider whether we should be using EraseAll here - e.g. should units of measure be significant when determining whether one overload shadows another? EraseAll means they are not. I think they should be significant - because the only reason we erase UoM when forbidding identical method signatures is that .NET IL doesn't permit two identical method signatures within the same class. But it does allow them across different classes.
Either way we will definitely need test cases for this case
There was a problem hiding this comment.
I think I can make a slight tweak to the overloads complexity; I notice I keep searching on the list that keeps getting additions which isn't necessary.
In regards to EraseAll. Because you cannot do overloading with UoM normally in classes, should we now allow it in this case? I was worried about consistency. But, it would be extremely powerful to allow it. I'll set it back to EraseNone; it should be pretty interesting to see overloading with UoM. I can imagine reasonable use cases where it would be helpful. :)
There was a problem hiding this comment.
It's a tricky choice - each way sort of seems wrong. Could you try the switch and explore some test cases?
dsyme
left a comment
dsyme
left a comment
There was a problem hiding this comment.
Good work - I left some comments
src/fsharp/NameResolution.fs
Outdated
| (minfosNew, minfosCurrent) | ||
| ||> List.fold (fun minfos minfo1 -> | ||
| // Shadow methods with the same signature. | ||
| if minfos |> List.exists (fun minfo2 -> MethInfosEquivByNameAndSig Erasure.EraseAll true g amap m minfo1 minfo2) then |
There was a problem hiding this comment.
We should consider whether we should be using EraseAll here - e.g. should units of measure be significant when determining whether one overload shadows another? EraseAll means they are not. I think they should be significant - because the only reason we erase UoM when forbidding identical method signatures is that .NET IL doesn't permit two identical method signatures within the same class. But it does allow them across different classes.
Either way we will definitely need test cases for this case
| x.Add(k, Item.MethodGroup (k, minfos, None)) | ||
|
|
||
| // Methods will hide properties, events, and fields with the same name. | ||
| | Item.MethodGroup _, Item.Property _ |
There was a problem hiding this comment.
This seems fine, will need test cases and RFC update
There was a problem hiding this comment.
Yup, for sure. This is to mimic opening multiple static classes in C# and what their behavior is.
TIHan
changed the title
|
|
||
| let getMethItems () = | ||
| [ let methGroups = | ||
| AllMethInfosOfTypeInScope ResultCollectionSettings.AllResults infoReader nenv None AccessorDomain.AccessibleFromSomeFSharpCode PreferOverrides m ty |
|
@TIHan I've forgotten exactly what's in this branch but we recorded these items today
|
OK this part would have to be undone per our design discussion today |
|
Closing in favor of this: #9513 |
3 participants
This places the open static classes in preview to allow some bake time for the feature and make sure we cover all design scenarios.
This also fixes the ability for multiple open static classes that have the same method name to be overloads of each other. Resolving this issue: #7400
@cartermp has a design in his head and so he will update the open static classes RFC. So, we will continue to update the design there.