Lint suggests "not (isNull x)" over "x <> null"

[mydogisbox]

Description

The linter suggests replacing a null check (x <> null) with the more complicated not (isNull x). I'm not coming up with any cases where I think a null check would be better as not (isNull x).

Repro steps

Please provide the steps required to reproduce the problem

1. Add a <> null check to your F# code

2. View linter error

Expected behavior

The linter doesn't warn on <> null checks.

Actual behavior

The linter warns on <> null checks.

Known workarounds

Disable the linter.

Related information

• Operating system
• Branch
• .NET Runtime, CoreCLR or Mono Version
• Performance information, links to performance testing scripts

[abelbraaksma]

I believe the motivation is that the operator can be overloaded or overridden, therefore it's generally better to use isNull and not << isNull and variants which are unambiguous and will always create the same optimized opcode. A similar rule exists in C# FxCop for things like if(x != null).

I don't like the verbosity either, so I just have isNotNull: 'a -> bool for the rare cases I need it (rarely ever, it's F# after all).

[Krzysztof-Cieslak]

Null checks are difficult. And not using <> null is a good suggestion: https://latkin.org/blog/2015/05/18/null-checking-considerations-in-f-its-harder-than-you-think/

[mydogisbox]

I can see why that could be a reasonable lint in some cases. That being said, performance isn't always an important consideration, and if we want to protect against bad actors overloading/overriding operators we wouldn't be able to use them at all.

My suggestion would be to make this an opt-in lint (although I don't see that as an option for lints currently), rather than a default lint.

[abelbraaksma]

The problem is that it's a pitfall and more often than not a programming mistake and can lead to subtle and not so subtle errors.

I can't judge per the opt in vs opt out (my preference th the latter, the whole reason people use Lint is to be warned about such cases that aren't obvious). Since opt out is already possible (unless I'm mistaken, it's been a while, but I assume the attributes to switch off specific warnings still exist), why not just use that?

[mydogisbox]

At least in VSCode there isn't any obvious way to turn off individual lints:

The problem is that it's a pitfall and more often than not a programming mistake and can lead to subtle and not so subtle errors.

Again, by that reasoning, all operators are to be avoided, but as long as I can trivially turn it off I don't care that much.

[Krzysztof-Cieslak]

VSCode/Ionide is using FSharpLint config file - http://fsprojects.github.io/FSharpLint/#Configuration-Files

[mydogisbox]

I guess that's really an issue with the ionide integration then since its on by default but then I have to go dig around in non-obvious config files to configure it. If I feel like the linting is worth it maybe I'll go create an issue there.

[jgardella]

So this is one of the hints defined in the default configuration file. You can disable it by adding an fsharplint.json config file; you can base it on the default here, and in your case you would want to remove these lines.

In my opinion I think it is a sensible hint to have by default, given the same reasons @abelbraaksma gave. But we are currently re-evaluating the default configuration as part of the v1 release and welcome any comments on it (#436). Additionally, it may be useful to add justification in the docs for any hints we have in the default configuration.

@Krzysztof-Cieslak it may be useful to add some additional info in the Ionide config about how FSharpLint can be configured, likely with a link to the page you provided above (although probably makes sense to wait on adding that until after v1, as the docs pages will move around a bit).

[abelbraaksma]

Again, by that reasoning, all operators are to be avoided, but as long as I can trivially turn it off I don't care that much.

I don't follow your logic. Checking for null is fundamentally different than structural equality or custom equality. This is true in most languages. In C#, you will be warned when you do if x == null. In SQL you can't get anywhere unless you do IS NULL or IS NOT NULL, in JavaScript you must use === and not ==, in Java they introduced @NotNull as a code contract (but you can use == and !=).

Since null means the absence of data, it should be treated specially. Historically, this was best done in F# with match x with null -> .... Using = or <> was, also historically, never a good idea. Since a null-check is essentially a unary operation and not a binary, they could have gone with a special operator. I don't know the exact reasoning behind introducing isNull and not a new operator, but you can trivially create your own: let (!!) x = isNull x.

Note that the MS docs specifically do not suggest to use = or <>: https://docs.microsoft.com/en-us/dotnet/fsharp/language-reference/values/null-values (that document predates the isNull introduction and should be updated).

Of course, you're free to use whatever coding style you prefer, but as you've seen in the referenced blog post, using = or <> is a recipe for disaster (see also: https://github.com/dotnet/fsharp/issues/3108). It may be unfortunate that F# doesn't have a better way of dealing with it, but since null is very, very rare in F# code, I think it matters little and is just not high on the priority list to try to make the already very complex structural equality code (inside the compiler) even more complex.

[abelbraaksma]

@jgardella, I thought it was also possible to switch of specific messages just like FxCop, i.e. with SuppressMessageAttribute?

It definitely used to work like that: https://stackoverflow.com/questions/42948899/how-to-turn-off-ionide-lint-warnings

[jgardella]

@abelbraaksma Yes it is, but we changed from using attributes to using comments for that. But you cannot disable a specific hint, you can only disable all Hints. In this case you could add something like // fsharplint:disable Hints.

I had suggested removing the hint from the config itself as then you can disable just that hint, and it sounded like that's what was wanted. But I agree with you that this is useful hint and probably best to keep it on by default.

[asik]

Null checks are difficult. And not using <> null is a good suggestion: https://latkin.org/blog/2015/05/18/null-checking-considerations-in-f-its-harder-than-you-think/

I wonder if this is still true today. Lots of work went into optimization in both F# and the JIT since then. ~~I tried it quickly on SharpLab (link) and the generated code was properly inlined.~~

Well, nevermind, it`s still bad.