dotnet / fsharp

The F# compiler, F# core library, F# language service, and F# tooling integration for Visual Studio
https://dotnet.microsoft.com/languages/fsharp
MIT License
3.89k stars 784 forks source link

Type annotation adjacency warning doesn't consider operator names #774

Open frankshearar opened 8 years ago

frankshearar commented 8 years ago

In this snippet,

type Digit<'a> = 'a list
type FingerTree<'a> =
    | Empty
    | Single of 'a
    | Deep of Digit<'a> * FingerTree<Node<'a>> * Digit<'a>

let rec (<<)<'a> (a: 'a) (tree: FingerTree<'a>): FingerTree<'a> =
    match tree with
    | Empty -> Single a
    | Single b -> Deep ([a], Empty, [b])
    | Deep ([b; c; d; e], m, suffix) -> Deep ([a; b], (Node3 (c, d, e)) << m, suffix)
    | Deep (prefix, m, suffix) -> Deep (List.append [a] prefix, m, suffix)

the compiler warns that "Type parameters must be placed directly adjacent to the type name". Presumably that's because the method name is surrounded by ()s.

(I need the <'a> because the function takes a FingerTree<'a> but the recursion takes a FingerTree<Node<'a>>.)

If I make the operator a function, instead --

    let rec addToFront<'a> (a: 'a) (tree: FingerTree<'a>): FingerTree<'a> =
    match tree with
    | Empty -> Single a
    | Single b -> Deep ([a], Empty, [b])
    | Deep ([b; c; d; e], m, suffix) -> Deep ([a; b], addToFront (Node3 (c, d, e)) m, suffix)
    | Deep (prefix, m, suffix) -> Deep (List.append [a] prefix, m, suffix)

-- then everything works.

drvink commented 8 years ago

I ran into this previously too; it's the same as issue #396. The solution is to do this instead:

let rec ``<<``<'a> blahblah =
frankshearar commented 8 years ago

Hm, it does remove the squigglies... but then I'm defining a function, not an operator. I can't say `1 << myTree``. It's not a massive train smash: it just seems like an unnecessary wart.

drvink commented 8 years ago

That is true--it doesn't really solve the problem the way you (and I) wanted--but agreed; if it can be fixed without breaking existing code, it would be nice, since it certainly seems unintentional and not particularly desirable that it doesn't work as one would expect. (sorry for the noise, just my two cents!)

PatrickMcDonald commented 8 years ago

As a workaround, would something like the following work?

let rec addToFront<'a> (a: 'a) (tree: FingerTree<'a>): FingerTree<'a> =
    match tree with
    | Empty -> Single a
    | Single b -> Deep ([a], Empty, [b])
    | Deep ([b; c; d; e], m, suffix) -> Deep ([a; b], addToFront (Node3 (c, d, e)) m, suffix)
    | Deep (prefix, m, suffix) -> Deep (List.append [a] prefix, m, suffix)

let (<<) = addToFront
dsyme commented 8 years ago

You likely don't need the <'a> at all - it can be inferred. You could also use let op_LessLess<'a> ...

frankshearar commented 8 years ago

@PatrickMcDonald That's a clever idea, and does indeed work. Thanks!

@dsyme It can't be inferred because the recursive call is Finger<Node<'a>>, so type inference complains that "The resulting type would be infinite when unifying ''a' and 'Node<'a>'". But using op_LessLess<'a> does do the trick, with a small wrinkle:

let rec op_LessLess<'a> (a: 'a) (tree: FingerTree<'a>): FingerTree<'a> =
    match tree with
    | Empty -> Single a
    | Single b -> Deep ([a], Empty, [b])
    | Deep ([b; c; d; e], m, suffix) -> Deep ([a; b], op_LessLess (Node3 (c, d, e)) m, suffix)
    | Deep (prefix, m, suffix) -> Deep (List.append [a] prefix, m, suffix)

The wrinkle is that the recursive call has to use op_LessLess not <<, or the compiler thinks I'm calling the default << operator. But making the parser special-case the equality of "op_LessLess" with "<<" doesn't seem like a win to me.