Promote better awareness of partial functions for new users

[Rickasaurus]

I propose we look for a way to promote better awareness of partial functions (and methods/properties) for new users for F#. In observing many new F# programmers at my company I've found it's much too easy for them to reach for Option.Value, Option.get or List.head with no negative feedback until code review time.

• The most obvious solution would be to generate warnings on usage of the FSharp.Core partial combinators as this would align nicely with warnings on incomplete pattern matches.
• I'm very open to other suggestions but I'd really like it if whatever we choose is on by default.

The existing way of approaching this problem in F# is to try to inform up front and code review later.

Pros and Cons

The advantages of making this adjustment to F# are easier on-boarding of new team members.

The disadvantages of making this adjustment to F# are that it might be annoying for seasoned vets.

Extra information

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

Related suggestions: (put links to related suggestions here)

Affidavit (please submit!)

Please tick this by placing a cross in the box:

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

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

[dsyme]

Relevant twitter discussion: https://twitter.com/eiriktsarpalis/status/880505955838033920

[cartermp]

I'm definitely for the warnings + suggestions to use the module functions. Example:

> let x = Some "bananas";;
val x : string option = Some "bananas"

> x.Value;;

warning FSSOMEWARNING: The '.Value' property is not recommended.  The 'Option.get' function is preferred here, or explicit pattern matching on 'x'.

val it : string = "bananas"

[eiriktsarpalis]

@Rickasaurus I don't think totality is necessarily the best criterion for adding such warnings. Take for example Seq.length, which is partial since sequences can be infinite. I wouldn't think it reasonable to be warned every time I use that.

[cartermp]

I was thinking that warnings specifically for an option's .Value and a list's .Head were only up for consideration here. Perhaps there's more to identify here, though.

[eiriktsarpalis]

@cartermp I would try to emphasize pattern matching over Option.get. Novices should be encouraged to use pattern matching everywhere, and discover the naughty bits such as Option.get and Unchecked.defaultof on their own.

[cartermp]

I agree. Error message should be more like this then:

warning FSSOMEWARNING: The '.Value' property is not recommended.  Try pattern matching on 'x' instead.

I also think it's worth not emitting this error if it's guarded properly:

if x.IsSome then
    printfn "hey look I'm %A" x.Value
else
    printfn "I am nothing"

Though I suppose it's equally as valid to say that Option.get should be used there instead of .Value just to push people towards the module functions as per the original text of this issue.

[isaacabraham]

@cartermp Why should you warn on .Value but not Option.get? Both are just as bad as each other.

I also don't think that we need to go down the route of complex guarded property checks - doing an if / then check as above is (IMHO) not taking advantage of the type system and pattern matching, which is what we should be encouraging users to do. If anything, I'd like to see a specific warning (or lint) for if x.IsSome then which suggests to use pattern matching for expressions or Option.iter for side-effects.

[dsyme]

Some data

• 50 uses of .IsNone in the F# compiler
• 93 uses of .IsSome in the F# compiler
• 52 uses of Option.get in the F# compiler
• 64 uses of .Value in the F# compiler
• 97 uses of .Head in the F# compiler
• 29 uses of .Tail in the F# compiler

I'm not defending them :) But I probably wrote most of them, for better or worse, and I generally try fairly hard to avoid them (though not as hard as others, and not as hard as I should). The integrity of the code base has definitely suffered because of their encroachment

Note I have no beef with IsNone, IsSome, IsEmpty, nonNil etc.

[isaacabraham]

@dsyme I would agree that IsNone etc. etc. fall into a different category - the last four methods you mention above are unsafe whereas the first two aren't, but could indicate a code smell - perhaps those would be more of a linter-style warning.

What about array / list indexers? Theoretically those are also unsafe and could be replaced with tryItem.

[dsyme]

There are others too - List.last etc.

What about array / list indexers? Theoretically those are also unsafe and could be replaced with tryItem.

Certainly array indexers represent such a thoroughly well-established form of programming that it would be very hard to constrain that. And there are serious performance issues involved

Note that I'm more inclined to an opt-in mechanism, where policy can be applied later in enterprises. I can't justify forcing people to rewrite existing, working codebases to eliminate a new opt-out warning (or else opting out of the warning altogether)

Adding "informational" messages to the system may also be reasonable

[isaacabraham]

Agreed - I'm not suggesting breaking changes or warnings everywhere. And as you say, there's perf considerations (as well as the fact that array indexing is a well-known pattern).

Are you thinking along the lines of having essentially a switch on the compiler that turns on a "strict" mode, with more warnings etc.?

If so - you might want to be consistent across these calls - things that can't be proven as safe would give a warning?

[cartermp]

@isaacabraham I agree with most of your sentiments, but this is a bit strange:

doing an if / then check as above is (IMHO) not taking advantage of the type system and pattern matching, which is what we should be encouraging users to do

There are cases where something like function | Some x -> whatever | None -> () is used in-line somewhere, and I'd argue that the type system isn't really being used here either. The if x.IsSome then x.Value else something pattern is perfectly valid here, safe, and takes as much advantage of the type system as the pattern match. It's a bit of a fine line to walk, and certainly involves opinion, but if such a pattern is guarded properly . . . why warn? I don't find that to be consistent, especially since the code is just as safe as a pattern match.

[Rickasaurus]

RE: if x.IsSome then x.Value else something

The problem here as far as I understand it is it's very difficult to check if-else statements because the type system doesn't define that "it's one of these discrete things" in any meaningful way. You could maybe special case option types but how could you do this generally?

RE: Indexing

Most programmers are familiar with indexing (although most programmers make mistakes with indexing regularly). A language where indexing returned an option would be amazing as long as it handled operations on those types well, but I doubt F# will ever go there. (It's a shame it can be shadowed with a module like with the numeric stuff).

[isaacabraham]

@Rickasaurus isn't this is the sort of thing Typescript does - it checks to see how some value is being used to determine if it's "safe" to access (wasn't this also something being suggested for C# vNext - safe null checks?).

@cartermp I see the difference with those two as with if / then, you might know it's safe, but the compiler can't prove that through the types i.e. .Value is inherently unsafe. It's the same as saying if List.length > 0 then List.head etc. - you're using imperative statements to determine something rather than the type system preventing you for entering the incorrect logic etc..

[Rickasaurus]

@isaacabraham I'm not familiar with it, but I assume it's one of those things that only works some of the time and only with null. If it's broader than that it would be excellent to figure out if it could fit into F# :).

[ghost]

This almost should be a breaking change. As still rather a newbie, I've been meaning to find the right place to ask: if null and null checking were a "billion dollar mistake", why should there even be List.head and empty list checking? A warning seems an absolute minimum. (Okay that's a rhetorical question and people may have some good reasons. But it still seems to me that eg list operations and empty lists are exactly the same as null checking dangers.)

[ghost]

(Working with time series simulations it was natural to pattern match in recursive iterative functions. But it was too easy to forget or be complacent in the wind-down, when the series were "supposed to have been" validated already, and the end points were getting recorded.)

[cartermp]

@isaacabraham

I see the difference with those two as with if / then, you might know it's safe, but the compiler can't prove that through the types i.e. .Value is inherently unsafe.

Well, If that specific expression wasn't safe, then we'd have quite a crisis on our hands 😛.

An issue here would be large codebases which use this pattern and also have warnings turned into errors. Now we've forced them to either turn off that setting or rewrite parts of the codebase which would otherwise not need any changes. I'm just not too hot on putting a blanket warning on that, even though you're correct in that it mostly shouldn't get used.

[isaacabraham]

@cartermp you know what I mean when I say unsafe - there's no "requirement" on you to first do an IsSome check before accessing Value :-) Put it this way: -

(* compiles *)
let f (x:string option) =
    if x.IsSome then
        x.Value
    else "default"

(* still compiles - oops! *)
let f (x:string option) =
//    if x.IsSome then
           x.Value
//    else "default"

(* compiles *)
let f (x:string option) =
    match x with
    | Some x ->
        x
    | None ->
        "default"

(* does not compile - good *)
let f (x:string option) =
//    match x with
//    | Some x ->
        x
//    | None ->
//        "default"

[cartermp]

Right, I agree with everything you've said. But I'm just not too hot on a blanket warning here, especially since folks will build with warnings as errors here. Forcing them to go back and change code which otherwise would not have broken wouldn't make people happy. I'm still not opposed to the idea outright, but as someone who has to take the brunt of such a decision from people, I'm a bit hesitant to do so 😄.

[vasily-kirichenko]

What about adding such attributes to all partial functions in FSharp.Core?

[<Browsable(false)>]
[<Warning(12053, "use pattern matching instead")>]
member this.Value = ...

It will not appear in intellisense (and newbies will never discover it :) ) and it will emit a warning. Quick fixes may be added for all the functions.

[cartermp]

I think that's a really good idea @vasily-kirichenko

[isaacabraham]

@cartermp sure, I get that (and agree, more or less :-)). Where do you draw the line though? List.head can be safe, provided you do enough checks beforehand. Same as basically all the other ones we've seen mentioned :-)

[Pauan]

In my opinion any non-total function which has a good alternative should give the warning: List.head can be replaced with pattern matching, Map.find can be replaced with Map.tryFind, option.Value can be replaced with pattern matching, etc.

Whether it can be safe is irrelevant (anything can be safe if used correctly, including null), what matters is that it's easy to make mistakes, and the compiler will not help you prevent those mistakes. Having the compiler do the work of proving it safe is much better than having the human do the work of proving it safe.

I think things like option.IsSome should not give warnings, because they are total, correct, and completely safe. Yes it might indicate a code smell, but a program that uses option.IsSome and does not use option.Value is still correct and safe.

Array indexes can often be replaced with pattern matching or sequence operators. But there are some situations where there aren't any good alternatives for array indexing. So I'm on the fence for array indexes. I think having an Array.tryGet: array<'T> -> int -> option<'T> function would be a very good idea.

[Rickasaurus]

@vasily-kirichenko This is awesome. Even better if the attribute could be exposed for library writers to use. Does the warning attribute work in 3rd party libs?

[coolya]

Maybe one comment why I think this is pretty useful. I would consider myself a f# expert but I'm using it for quite a lot side projects. And I just spent about an hour looking for a `.head´ function call that caused my async code to crash in an edge case. Though the fact that async didn't preserver a usable stacktrace contributed to it taking so long.

But I would really like to have this warning it would have made it very easy to spot the issue. And especially new colleagues that I introduce to f# are using partial functions all over the place because there is little guidance to avoid them.

[jwosty]

Keep in mind that these correctness checks only apply to immutable code. E.g. this code isn't actually correct, even though it performs a check, so the proposed warning would still need to appear here:

member val Name with get, set = None

member this.GetName() =
    if Option.isSome this.Name then
        this.Name.Value // or Option.get, if you please
    else "<Unnamed>"

(Name could be changed from another thread in between the call to isSome and Value)