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

Move array/string slicing/indexing methods to type extensions #796

Open
nelson-wu opened this issue Oct 18, 2019 · 9 comments

Comments

@nelson-wu
Copy link

@nelson-wu nelson-wu commented Oct 18, 2019

I propose we move slicing/indexing methods for array/string to type extensions. The current way slicing is wired up results in weird behavior overloading GetSlice on arrays, where you can define an overload only if it has a different amount of arguments from what we already have in Core.

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

typechecker.fs:6340


    let attemptArrayString = 
        if isArray || isString then 

            let indexOpPath = ["Microsoft";"FSharp";"Core";"LanguagePrimitives";"IntrinsicFunctions"]
            let sliceOpPath = ["Microsoft";"FSharp";"Core";"Operators";"OperatorIntrinsics"]
            let info = 
                match isString, isArray, wholeExpr with 
                | false, true, SynExpr.DotIndexedGet (_, [SynIndexerArg.One(SynExpr.Tuple (false, ([_;_] as idxs), _, _))], _, _)           -> Some (indexOpPath, "GetArray2D", idxs)
                | false, true, SynExpr.DotIndexedGet (_, [SynIndexerArg.One(SynExpr.Tuple (false, ([_;_;_] as idxs), _, _))], _, _)         -> Some (indexOpPath, "GetArray3D", idxs)
                | false, true, SynExpr.DotIndexedGet (_, [SynIndexerArg.One(SynExpr.Tuple (false, ([_;_;_;_] as idxs), _, _))], _, _)       -> Some (indexOpPath, "GetArray4D", idxs)
                | false, true, SynExpr.DotIndexedGet (_, [SynIndexerArg.One idx], _, _)                                          -> Some (indexOpPath, "GetArray", [idx])
                | false, true, SynExpr.DotIndexedSet (_, [SynIndexerArg.One(SynExpr.Tuple (false, ([_;_] as idxs), _, _))], e3, _, _, _)     -> Some (indexOpPath, "SetArray2D", (idxs @ [e3]))
                | false, true, SynExpr.DotIndexedSet (_, [SynIndexerArg.One(SynExpr.Tuple (false, ([_;_;_] as idxs), _, _))], e3, _, _, _)   -> Some (indexOpPath, "SetArray3D", (idxs @ [e3]))
                | false, true, SynExpr.DotIndexedSet (_, [SynIndexerArg.One(SynExpr.Tuple (false, ([_;_;_;_] as idxs), _, _))], e3, _, _, _) -> Some (indexOpPath, "SetArray4D", (idxs @ [e3]))
                | false, true, SynExpr.DotIndexedSet (_, [SynIndexerArg.One _], e3, _, _, _)                                       -> Some (indexOpPath, "SetArray", (GetIndexArgs indexArgs @ [e3]))
                | true, false, SynExpr.DotIndexedGet (_, [SynIndexerArg.Two _], _, _)                                            -> Some (sliceOpPath, "GetStringSlice", GetIndexArgs indexArgs)
                | true, false, SynExpr.DotIndexedGet (_, [SynIndexerArg.One _], _, _)                                            -> Some (indexOpPath, "GetString", GetIndexArgs indexArgs)
                | false, true, SynExpr.DotIndexedGet (_, [SynIndexerArg.Two _], _, _)                                            -> Some (sliceOpPath, "GetArraySlice", GetIndexArgs indexArgs)
                | false, true, SynExpr.DotIndexedGet (_, [SynIndexerArg.One _;SynIndexerArg.Two _], _, _)                        -> Some (sliceOpPath, "GetArraySlice2DFixed1", GetIndexArgs indexArgs)
                | false, true, SynExpr.DotIndexedGet (_, [SynIndexerArg.Two _;SynIndexerArg.One _], _, _)                        -> Some (sliceOpPath, "GetArraySlice2DFixed2", GetIndexArgs indexArgs)
                | false, true, SynExpr.DotIndexedGet (_, [SynIndexerArg.Two _;SynIndexerArg.Two _], _, _)                        -> Some (sliceOpPath, "GetArraySlice2D", GetIndexArgs indexArgs)
                | false, true, SynExpr.DotIndexedGet (_, [SynIndexerArg.Two _;SynIndexerArg.Two _;SynIndexerArg.Two _], _, _)    -> Some (sliceOpPath, "GetArraySlice3D", GetIndexArgs indexArgs)
                | false, true, SynExpr.DotIndexedGet (_, [SynIndexerArg.Two _;SynIndexerArg.Two _;SynIndexerArg.Two _;SynIndexerArg.Two _], _, _)      -> Some (sliceOpPath, "GetArraySlice4D", GetIndexArgs indexArgs)
                | false, true, SynExpr.DotIndexedSet (_, [SynIndexerArg.Two _], e3, _, _, _)                                                             -> Some (sliceOpPath, "SetArraySlice", (GetIndexArgs indexArgs @ [e3]))
                | false, true, SynExpr.DotIndexedSet (_, [SynIndexerArg.Two _;SynIndexerArg.Two _], e3, _, _, _)                                         -> Some (sliceOpPath, "SetArraySlice2D", (GetIndexArgs indexArgs @ [e3]))
                | false, true, SynExpr.DotIndexedSet (_, [SynIndexerArg.One _;SynIndexerArg.Two _], e3, _, _, _)                                         -> Some (sliceOpPath, "SetArraySlice2DFixed1", (GetIndexArgs indexArgs @ [e3]))
                | false, true, SynExpr.DotIndexedSet (_, [SynIndexerArg.Two _;SynIndexerArg.One _], e3, _, _, _)                                         -> Some (sliceOpPath, "SetArraySlice2DFixed2", (GetIndexArgs indexArgs @ [e3]))
                | false, true, SynExpr.DotIndexedSet (_, [SynIndexerArg.Two _;SynIndexerArg.Two _;SynIndexerArg.Two _], e3, _, _, _)                     -> Some (sliceOpPath, "SetArraySlice3D", (GetIndexArgs indexArgs @ [e3]))
                | false, true, SynExpr.DotIndexedSet (_, [SynIndexerArg.Two _;SynIndexerArg.Two _;SynIndexerArg.Two _;SynIndexerArg.Two _], e3, _, _, _) -> Some (sliceOpPath, "SetArraySlice4D", (GetIndexArgs indexArgs @ [e3]))
                | _ -> None // error(Error(FSComp.SR.tcInvalidIndexerExpression(), mWholeExpr))
            match info with 
            | None -> None
            | Some (path, functionName, indexArgs) -> 
                let operPath = mkSynLidGet mDot path (CompileOpName functionName)
                let f, fty, tpenv = TcExprOfUnknownType cenv env tpenv operPath
                let domainTy, resultTy = UnifyFunctionType (Some mWholeExpr) cenv env.DisplayEnv mWholeExpr fty
                UnifyTypes cenv env mWholeExpr domainTy e1ty 
                let f', resultTy = buildApp cenv (MakeApplicableExprNoFlex cenv f) resultTy e1' mWholeExpr
                let delayed = List.foldBack (fun idx acc -> DelayedApp(ExprAtomicFlag.Atomic, idx, mWholeExpr) :: acc) indexArgs delayed // atomic, otherwise no ar.[1] <- xyz
                Some (PropagateThenTcDelayed cenv overallTy env tpenv mWholeExpr f' resultTy ExprAtomicFlag.Atomic delayed )
        else None

The F# 4.1 spec states:
image

The spec is misleading here, as from the code snipped above it is evident there is no GetSlice extension on array/string. Instead in the typechecker the syntax is analyzed and the correct concrete slice implementation is directly picked, based on how many arguments are in the slice.

Pros and Cons

The advantages of making this adjustment to F# are:

  • We would be closer to spec
  • It would allow all slicing to have consistent behavior
  • It would get rid of a ugly piece of code
  • It would allow normal overloading/shadowing behavior for slices

The disadvantages of making this adjustment to F# are ...

  • People would be able to overload the default indexer for arrays/strings
  • People would be able to overload the default GetSlice for arrays/strings
  • We'd have to expose a new module in Core that needs to be autoloaded

Extra information

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

Related suggestions: (put links to related suggestions here)
dotnet/fsharp#7742

Affidavit (please submit!)

Please tick this by placing a cross in the box:

  • [x ] This is not a question (e.g. like one you might ask on stackoverflow) and I have searched stackoverflow for discussions of this issue
  • [ x] I have searched both open and closed suggestions on this site and believe this is not a duplicate
  • [x ] 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:

  • [x ] This is not a breaking change to the F# language design
  • [x ] I or my company would be willing to help implement and/or test this
@cartermp

This comment has been minimized.

Copy link
Member

@cartermp cartermp commented Oct 18, 2019

@dsyme thoughts? In order to handle slicing properly for 3D and 4D arrays, we'd have to use type extensions. So that would create a world where there are type extensions for 3D and 4D arrays, which can be shadowed, but not for 1D and 2D arrays. This would also bring the implementation in line with what the spec says.

@dsyme

This comment has been minimized.

Copy link
Collaborator

@dsyme dsyme commented Oct 22, 2019

What are the binary compat ramifications?

@dsyme

This comment has been minimized.

Copy link
Collaborator

@dsyme dsyme commented Oct 22, 2019

(I'm fine with this in principle if binary and source compat is maintained)

@cartermp

This comment has been minimized.

Copy link
Member

@cartermp cartermp commented Oct 22, 2019

Binary and source compat is a requirement here, yes.

@dsyme

This comment has been minimized.

Copy link
Collaborator

@dsyme dsyme commented Oct 22, 2019

As I mentioned in the implementation PR, quotations of array indexing may have an issue here, e.g.

 <@ (Array2D.zeroCreate<int> 3 4).[0..1,*] @>;;

reveals GetArraySlice2D

  Call (None, GetArraySlice2D, ...)

So in theory this is a breaking change, and I wouldn't be at all surprised if there were quotation processing libraries that looked for GetArraySlice2D....

@dsyme

This comment has been minimized.

Copy link
Collaborator

@dsyme dsyme commented Oct 22, 2019

(I presume a fix for that could be inserted in the stage of the compiler that makes quotations)

@nelson-wu

This comment has been minimized.

Copy link
Author

@nelson-wu nelson-wu commented Oct 22, 2019

@gusty

This comment has been minimized.

Copy link

@gusty gusty commented Oct 23, 2019

The problem in keeping them as normal methods is that any extension will be ignored.

See dotnet/fsharp#3692 which mentions specifically this problem.

@nelson-wu

This comment has been minimized.

Copy link
Author

@nelson-wu nelson-wu commented Oct 24, 2019

^ Yeah that's a concern with the current implementation. Right now any user can define their own type extension that implements GetSlice, but if it has the same amount of arguments as a GetSlice predefined in Core, it will be ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.