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

Simplified parameter null validation code #1014

Closed
5 tasks done
Happypig375 opened this issue May 30, 2021 · 7 comments
Closed
5 tasks done

Simplified parameter null validation code #1014

Happypig375 opened this issue May 30, 2021 · 7 comments

Comments

@Happypig375
Copy link
Contributor

Happypig375 commented May 30, 2021

I propose we add a new function to simplify parameter null validation code.

checkNullArg arg

That's it!

Example usage:

let length (array: _[]) =
     checkNullArg array
     array.Length

Here will be the secret sauce in FSharp.Core:

[<AutoOpen; AbstractClass; Sealed>] 
type CheckNullArg =
    static member checkNullArg (argument, [<System.Runtime.CompilerServices.CallerArgumentExpression "argument";
                                            System.Runtime.InteropServices.Optional;
                                            System.Runtime.InteropServices.DefaultParameterValue "">] argumentName : string) =
        if isNull argument then nullArg argumentName

This will work by building on CallerArgumentExpression.

The existing way of approaching this problem in F# is defining this function yourself or writing the long code every time:

if isNull argument then nullArg(nameof argument)

Compared to the C# equivalent of this feature, this does not introduce new syntax (!), is much easier to understand (! after the argument name itself?), and also makes the intention really explicit without the ! being too easy to miss.

Pros and Cons

The advantages of making this adjustment to F# are

  1. Convenience
  2. Consistency with C#
  3. Reduction of boilerplate which hinders reading

The disadvantage of making this adjustment to F# is that this is one more way to do the same thing, but is considerably simpler.

Extra information

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

Related suggestions: CallerArgumentExpression

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

For Readers

If you would like to see this issue implemented, please click the 👍 emoji on this issue. These counts are used to generally order the suggestions by engagement.

@Happypig375
Copy link
Contributor Author

Happypig375 commented May 30, 2021

Open questions:

  • FSharp.Core calls this function checkNonNull without the symmetry with nullArg. Should this be the name instead? Alternatively for discoverability, we could name this nullArgCheck instead.
  • Should this be marked inline? The checkNonNull of FSharp.Core does this. However, marking this inline could increase code size.
  • Do we need the System.Runtime.InteropServices.DefaultParameterValue ""? Without this attribute, the argument will default to null instead. Will this interfere with nullable reference type analysis in the future?

@charlesroddie
Copy link

charlesroddie commented May 30, 2021

Similarly to #883 , this needs to be considered in the context of https://github.com/fsharp/fslang-design/blob/master/RFCs/FS-1060-nullable-reference-types.md .

Then your example becomes:

let length (array: 'a[]) =
    // don't need a null check here since the function conveys that it expects a non-null argument
    array.Length

// or

let length (array: ('a[])?) =
    match array with
    | Null -> nullArg ...
    | NonNull a -> a.Length

@Happypig375
Copy link
Contributor Author

@charlesroddie Not really. Like in C# with nullable reference types, the argument itself is nonnullable but it defends against users with nullable reference type analysis disabled.

@En3Tho
Copy link

En3Tho commented Aug 20, 2021

It would be nice if ActivePatterns could use this too.

Ideally :

let (|NotNull|) (obj, [<Optional; DefaultParameterValue(null); CallerArgumentExpression("argument")>] description) = ArgumentNullException.ThrowIfNull(obj, description); obj
let nullCheckingCode (NotNull parameter: string) = printfn $"{parameter}"

For now it doesn't work. It would be fun if ActivePatterns could support optional parameters like these.
Today NotNull pattern works with single parameter but there is no way to pass proper argument name there.
Something like this:

let (|NotNull|) description x = ArgumentNullException.ThrowIfNull(x, description); x
let nullCheckingCode (NotNull "parameter" parameter: string) (NotNull "parameter2" parameter2) = printfn $"{parameter + parameter2}"

Or without description.

@En3Tho
Copy link

En3Tho commented Aug 24, 2021

@dsyme Do you think it's possible or reasonable to do with active patterns? I think it's a completely different proposal (like supporting optional parameters in active patterns) but it would be nice if you could take a small look at example above and say if you like the idea at all

@dsyme
Copy link
Collaborator

dsyme commented Jun 14, 2022

@dsyme Do you think it's possible or reasonable to do with active patterns? I think it's a completely different proposal (like supporting optional parameters in active patterns) but it would be nice if you could take a small look at example above and say if you like the idea at all

I'm not really sold on doing either of these in FSharp.Core. I don't really mind how people program it up in user code assuming CallerArgumentExpression is avaialble.

@dsyme
Copy link
Collaborator

dsyme commented Apr 13, 2023

Closing per my comment above

@dsyme dsyme closed this as completed Apr 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants