proposal: avoid_non_null_checks

[pq]

Name: avoid_non_null_checks

Description: Avoid using non-null-check operators.

Details: Code that uses non-null-check operators (!) is harder to understand and can lead to runtime exceptions. In general, you should avoid non-null-check operators by using null-aware operators or making local copies of potentially null elements.

Good:

class Buffer {
  String? contents;

  int length() {
    var c = contents;
    if (c != null) {
      return c.length;
    }
    return -1;
  }
}

class Buffer {
  String? contents;

  int length() => contents?.length ?? -1;
}

Bad:

class Buffer {
  String? contents;

  int length() {
    if (contents != null) {
      return contents!.length;
    }
    return -1;
  }
}

class Buffer {
  String? contents;

  int length() => contents != null ? contents!.length : -1;
}

An exception to this general rule is made for map lookups (which return a nullable type by default). Idiomatic Dart allows expressions like this:

Good:

String getEntry(String key) => myMap[key]!;

Alternatively, you might consider making a local copy and an explicit check:

Good:

String getEntry(String key) {
  var result =  myMap[key];
  if (result != null) return result;
  throw Exception('Result for $key unexpectedly null');
}

---

See discussion in #2562

[pq]

/fyi @duttaoindril

Feedback welcome.

/fyi @jakemac53 @natebosch @munificent @lrhn @eernstg @goderbauer @jefflim-google @davidmorgan

[jakemac53]

I think its fine to have a lint like this - but I probably wouldn't want it in the core/recommended sets. The null assertion operator can be safely used and it is a lot less verbose than some of the alternatives. It is also potentially more efficient than explicit null checks (in particular for dart2js, where I think they are mostly free, at least in the case of !., it just relies on JS null pointer exceptions).

[duttaoindril]

Hey @pq thanks for creating this proposal, I think this is a great idea. While I agree with @jakemac53 that it's much less verbose to use a !, I don't think it's safe to have it in codebases without proper commented explanations of why it's needed, and why alternatives aren't better (late, a null check, etc). I might even argue that the map lookup exception shouldn't be allowed because it can be unsafe, since map lookups can't be guaranteed to return a non-null value unless it's a constant.

[jakemac53]

I don't think it's safe to have it in codebases

While it's certainly true it can fail, so can any cast. And actually you can consider ! essentially identical to just inserting a cast to the non-nullable type of the receiver. But we don't have a lint to never use casts (note that you could avoid casts using the same mechanisms to avoid using !).

I do think this is a lint that some teams would choose to enable, I just don't think it should be a part of pub scoring (or even the recommended lints). For instance this code is perfectly safe:

if (map.contains('foo')) map['foo']!;

And also sometimes you really do just want something to fail if something isn't null that you expected to be null. Maybe you are handling that exception in a different way, or are in an application where simply crashing is fine for that use case.

[goderbauer]

I like the cleaner code this produces in the examples.

I was slightly confused by the proposed name of the lint, assertions made me initially think this lint will have have something to do with assert statements.

[natebosch]

We shouldn't put it in the recommended or scoring lints, but I think it would be fine to have as an option for projects that want to try to avoid !.

[bwilkerson]

Elsewhere we refer to the ! as the null-check operator, so I'd propose something like avoid_null_check_operators.

[pq]

I'd propose something like avoid_null_check_operators

Works for me. Updated.

[pq]

I like the cleaner code this produces in the examples.

I do too. I mean they're a little contrived but I think compelling enough. For an example of code in the wild that would benefit, check out this bit from draggable_scroll_sheet:

  List<double> _impliedSnapSizes() {
    for (int index = 0; index < (widget.snapSizes?.length ?? 0); index += 1) {
      final double snapSize = widget.snapSizes![index];
      assert(snapSize >= widget.minChildSize && snapSize <= widget.maxChildSize,
        '${_snapSizeErrorMessage(index)}\nSnap sizes must be between `minChildSize` and `maxChildSize`. ');
      assert(index == 0 || snapSize > widget.snapSizes![index - 1],
        '${_snapSizeErrorMessage(index)}\nSnap sizes must be in ascending order. ');
    }
    widget.snapSizes?.asMap().forEach((int index, double snapSize) {
    });
    // Ensure the snap sizes start and end with the min and max child sizes.
    if (widget.snapSizes == null || widget.snapSizes!.isEmpty) {
      return <double>[
        widget.minChildSize,
        widget.maxChildSize,
      ];
    }
    return <double>[
      if (widget.snapSizes!.first != widget.minChildSize) widget.minChildSize,
      ...widget.snapSizes!,
      if (widget.snapSizes!.last != widget.maxChildSize) widget.maxChildSize,
    ];
  }

IMO extracting a local for widget.snapSizes would help clean this up quite a lot.

[pq]

I might even argue that the map lookup exception shouldn't be allowed because it can be unsafe, since map lookups can't be guaranteed to return a non-null value unless it's a constant.

I tend to agree personally, but worry this might be too strict for the rule to see wide adoption -- it also contradicts the explicit advice in the FAQ.

I'd be curious to get @munificent's 2 cents though.

[lrhn]

Code like

if (map.contains('foo')) map['foo']!;

is perfectly safe because of the invariant promised by the Map class API, which is something the compiler cannot be expected to understand in general (and not something all subclasses necessarily satisfy).
That is exactly the situations where ! is the recommended approach: When the programmer knows, beyond any doubt, that the value is going to not be null, because of API promises and invariants, not because of language semantics.

There is nothing wrong with using ! when you use it correctly. The compiler just can't tell you whether you use it correctly or not.

If we introduce a lint, I'd actually prefer it to apply to all casts, not just non-null-casts. Casts are dynamic features with no static guarantees that they'll succeed (same as using dynamic or late).
Enforcing such a lint is dangerous. If all that happens is that people rewrite a! to a ?? (throw NullError()) or if (a == null) throw NullError(); ... use promoted a ..., you haven't gained anything, and you've lost conciseness.

(I do prefer using tests instead of casts. For map lookups, I'd actually prefer using null checks: var foo = map['foo']; if (foo != null) { ... } or map['foo'] ?? .... If that doesn't work the same, because the map value type is nullable, then the ! was probably also wrong. But, there is expressiveness in doing casts, assertions that this is going to be true, there is no need for an else branch. It has its place.)

P.S.: Oh dear, widget.snapSizes!.isEmpty reads so much like "is not empty" to me, I had to re-read it a couple of times to understand it. 😞

[pq]

P.S.: Oh dear, widget.snapSizes!.isEmpty reads so much like "is not empty" to me, I had to re-read it a couple of times to understand it.

Exactly! The presence of a ! makes me uneasy in general because I feel like it's a shortcut that benefited the author once but maintainers pay for again every time they read the code to convince themselves that it's reasonable.

If we introduce a lint, I'd actually prefer it to apply to all casts, not just non-null-casts.

I'd like to explore this more. (I think @srawlins has done some thinking here and I wonder how this corresponds to the various strict mode conversations that have started and stopped over the years.)

Thanks for the thoughtful responses and feedback!

[munificent]

I don't know if I'm convinced that ! is something that should be avoided so broadly that a lint is a good idea. We don't have a lint for as, and it has all of the same failure modes as !.

[pq]

We don't have a lint for as, and it has all of the same failure modes as !.

Well, we did have avoid_as but it's deprecated. 😄

[lrhn]

Well, then: Why did avoid_as become deprecated, and would the same arguments apply to !?

According to #1401 it seems there were just too many cases where you actually needed the cast after we removed implicit downcasts. (Possibly even before, if people had no_implicit_casts enabled).
So basically: Too many false positives.
I don't see why we'd expect ! to be any different, except that we do have more null-aware operators that can allow us to bypass the null instead of throwing. If you actually do know that a value is going to be non-null, using var value = this.value!; seems like the correct thing to do.

[pq]

I don't see why we'd expect ! to be any different, except that we do have more null-aware operators that can allow us to bypass the null instead of throwing.

This is the rub for me. With null-aware operators and an idiom of local assignment (which is often more readable anyway IMO), I think the judicious uses of ! are few and the costs of abuse significant. (draggable_scroll_sheet is maybe an extreme but does make the point.)

For a point of reference, it's interesting to see comparable advice in the Google Swift Style Guide:

https://google.github.io/swift/#force-unwrapping-and-force-casts

(which I generally agree with).

[natebosch]

I think the judicious uses of ! are few

I don't think I agree, or at least not "few" in relation to the amount of false positives I'm comfortable with.

I don't think I have strong feelings against the lint existing if someone really wants it. I'm in favor of helping educate authors who are not as wary of ! as they should be.

Mostly, though, I'm going to be sad if this lint results in someone rewriting foo!.bar(); into

var foo = this.foo;
if (foo == null) {
  throw StateError('foo should not be null');
}
foo.bar();

For full transparency, there was a case in the test package (twice, it recently came up again) where we were using a ! that confused users, and we refactored to a throw with a more useful message. dart-lang/test#1599

[pq]

Oof, yeah. I'd hate to see a bunch of gratuitous StateErrors too. That's bad enough to be it's own lint! 🤣

That example from test is great. Thanks! It'd be interesting to look at some more. In particular I'd be curious to know if in practice we wouldn't see other remedies (maybe foo should be declared late)?

I'm in favor of helping educate authors who are not as wary of ! as they should be.

💯

I guess the wrinkle is how to do that. Banning !s is a big hammer for sure. Other thoughts?

[duttaoindril]

I just think that teams should be able to have the option to easily highlight all the ! in their codebase, as either a ban hammer or a warning or info. There's no easy regex for it.

[davidmorgan]

Big -1 from me. People actually have to learn this, the lint doesn't help.

I've seen overly-strong guidance against using ! cause people to add codepaths supporting null even when they know there cannot be null, to avoid writing !.

[lrhn]

Throwing a better error message is good when the value can actually be null due to user error.

When the code has invariants which ensure that the value is guaranteed to be non-null, using ! is better.
If a ! throws, it's a programming error at the ! (the value could be null, you said it couldn't).
It's just like as cast in that aspect: Only use it when you know it will work. If it fails, you made a mistake.
If you don't know for certain that the cast will work, use a test and throw a better error targeting the one who made the actual mistake.

Different use-cases have different code. In some cases using ! is the correct and idiomatic choice. A general "don't use !" lint would be as wrong as the "don't use as" lint.

[jamesderlin]

Elsewhere we refer to the ! as the null-check operator, so I'd propose something like avoid_null_check_operators.

As I pointed out in dart-lang/sdk#47185, I think that that terminology is confusing and is particularly bad for less experienced Dart users. Calling it the "null-check operator" is ambiguous: there are many types of null-checks, some of which involve special operators (e.g. ??, ?.). Although we refer to ! as the "null-check operator" elsewhere, we refer to it as the "null assertion operator" elsewhere too (particularly https://dart.dev/null-safety/understanding-null-safety#null-assertion-operator).

[lrhn]

Just stumbled back into this thread to say that "null assertion operator" is bad because it's not an assertion (enabled by --enable-asserts).
The full "what it does" name should be "ensure not null-operator".

Maybe "non-null-check-operator" or "non-null-enforcing-operator"?
Or even just "throw-if-null-operator".

[jamesderlin]

@lrhn Yeah, I agree that "null assertion operator" also has problems. Also see dart-lang/sdk#47185 (comment).

[eernstg]

I'd suggest avoid_non_null_checks or avoid_non_null_check_selectors, cf. dart-lang/sdk#47185 (comment).

[pq]

Great suggestions, all. Thanks!

I've updated the proposal to use @eernstg's avoid_non_null_checks suggestion and preferred "non-null-check operators" over "null-check operators" as offered by @lrhn.

[scheglov]

My view on code like this:

var foo = this.foo;
if (foo == null) {
  throw StateError('foo should not be null');
}
foo.bar();

...or its original in form

foo!.bar();

...is to question "how do we know that foo is actually not null?". If we know that because we have already checked foo in the method that invokes code that does foo!.bar(), then why don't we pass the non-nullable value foo to this method? If we know this because foo is not null when foo2 is not null, and we checked foo2, then maybe these two values should be wrapped into another value and null-conditioned together.

Something like this - don't force types, but satisfy them.

[dkbast]

Just want to add my two cents: It would be great if we wouldn't have to make local copies, for the checks to work properly - just refactored a bunch of widgets out of my build routine and now "everything" is red because I don't have the local copies anymore ... might be hard to solve, but as it is right now its making me think that the benefits of avoiding (!) is not greater than adding lots of local assignments.

[pq]

Bumping for @scheglov @srawlins @keertip @bwilkerson @jcollins-g as it relates to our conversation this morning.

[parlough]

I'm coming back to this issue and have a few questions after reading through some the discussion.

Operator name (a bit off topic for this repo)

Long...discussion on operator name (Collapsed since moving elsewhere)

I collapsed this discussion, since I'm moving it elsewhere. Feel free to respond still though :)

I had a bit of trouble finding this issue, because on the Dart site, we always referred to the operator with assert/assertion in its name. In Bob's original understanding null safety doc, it's called "The postfix null assertion "bang" operator (!)".

I did a quick scan through various sources including linter rules, diagnostics, dart.dev, the language repo, and the SDK and found quite a mix of terms: null assert/ion operator, non-null assertion operator, [runtime] null check operator, non-null check operator, sometimes just the `!` operator, and there are likely other terms in use.

What is the consistent name we should be presenting to developers? So they can find and understand documentation, lints, issues, and more discussing it, and discuss it with each other consistently. It seems each name has problems:

• ❌ null-assertion operator - This one doesn't make sense to me. Since "assert" means the condition is true, but it's the opposite in this case. I guess unless your goal is to throw an exception.
• 🧡 non-null assertion operator - I like this. It's familiar to Kotlin developers and TypeScript developers. However, the concern presented above is that "assert" means something already in Dart, and are seen as toggleable. I've never seen this misconception in the real world, but it could be an issue.
• 🟧 non-null check operator - This seems to be at least tolerated among the among discussion, but I feel it's not accurate. The operator itself isn't doing any checks and "check" gives the impression I can do something if it is or is not null, but that something is throw an exception.
• 🟧 non-null cast operator - To take advantage of user's understanding of as. Kind of cool, but my understanding is that this may provide false expectations that they are exactly the same.
• 🟧 throw if null operator or null-failure operator - These ones could be good. They're not super familiar, but it's behavior is very clear from the name :)
• 🟫 non-null enforcing operator - I don't know what to think about this one yet. Might be good?
• 🟥 the (postfix) `!` operator - It's confusing since we have the negation operator and it's really hard to search for.
• ❓ Extra: C# calls its similar operator the null-forgiving operator. I'm not a huge fan, because it could give the impression that it's forgiving to null values at runtime, but in-fact it's the complete opposite.
• ❔ Extra 2: Swift calls its similar operator the force unwrap operator or often just the `!` operator.

While it'd be nice to have the perfect name, choosing one and standardizing it across our documentation and shared vernacular is more important. To that end, I would suggest non-null assertion operator despite the other connotations assert has. "Assertion" vs just "assert" helps differentiate it, but most of all, it greatly benefits from being familiar to developers of a few other programming languages.

I'd like to get the docs standardized sooner rather than later, as names do tend stick, so I'd love some thoughts or just a final decision from someone :)

Lint name

This depends on name we standardize on for the operator, but I will say avoid_non_null_checks seems quite misleading to me. It wouldn't be what I look for if I wanted to enable a lint with this functionality. It personally reads as if it's telling me that the following is bad:

if (value != null) {
}

Lint status

I would love to see this lint implemented. Especially since this issue was raised, the situation has got a lot better. Pattern matching and to a lesser extent, field promotion, make safer options to the operator much easier to implement.

It might not be something everyone wants to enable all the time, but even for those cases, it could be nice for occasional code-base auditing or cleanup.

[tisohjung]

just adding on to ideas since nobody mentioned
coming from swift background, avoid_force_unwrapping seems clear
https://realm.github.io/SwiftLint/force_unwrapping.html

and this is Bad in that case.

String getEntry(String key) => myMap[key]!;

[lrhn]

While this is trending for those sorting by recently updated, the "good" example can now use patterns (and length should be a getter):

class Buffer {
  String? contents;

  int get length {
    if (contents case var c?) {
      return c.length;
    }
    return -1;
  }
}

I'd probably do int get length -> contents?.length ?? -1; in practice, though. A less reducible example might be better.

[eernstg]

I hope it isn't too spammy to add another comment on the naming. Otherwise please skip this comment. ;-)

As I mentioned here, the Dart specification documents use 'test' to describe behaviors like e is T (where a boolean value is computed) and 'check' to describe behaviors like e as T (where a boolean value b is computed and the expression has the value of e if b is true, and the expression throws if b is false).

The motivation for this terminology has not been made explicit anywhere that I know of, but you could say that when we 'check' something it is assumed to be true ("let me check that the brakes on this car are actually working"), and we need to have some kind of recovery if it isn't; and if we 'test' something then it may or may not be true, we just want to know which one it is (a bit like Abraham being tested to see if he would make the sacrifice or not).

So I suggested that we could use a name of the form ..._check.

However, as @parlough and others have argued, this interpretation is not obvious at all to everybody.

The words 'assert' or 'assertion' have been mentioned as less confusing ways to designate the behavior where 'false' causes a run-time failure, and the standard argument against this is that assert as a language construct can be enabled and disabled, and we don't want to imply that ! in e! can be turned off. I'm also a little bit worried about having different canonical words for the same thing in different parts of the Dart universe.

However, as I understand the semi-recent developments in the naming of lints, avoid_... isn't considered to be a good naming style anyway. The recommended naming style describes the problematic property of the target code and doesn't mention that it should be avoided.

So maybe a really simple name like

     throws_if_null

could be used? The lint flags an expression of the form e!, and the problems is that it will throw at run time if e evaluates to null. It is not said (but perhaps sufficiently obvious) that this is a bad thing, and also that there are other ways to write the code such that a run-time failure will not occur. We're basically just repeating the meaning of !. So it's really not very informative. However, I suspect that it could work well in practice because it states the problem very directly, and developers won't need much familiarity with this lint before they can fill in the blanks.

You could argue that many other constructs will match the same description (aReceiverOfTypeDynamic.whatever() will certainly also throw if said receiver evaluates to null), but that's probably a source of confusion that goes away when the lint has been encountered just a few times.

As I see it, the really big question is whether there will be too many false positives. It's basically outlawing an entire language feature.

(Oh, should I even mention this? Of course not! So here we go: We could introduce some kind of support for marking certain categories of expressions as "safe for non-null checking", e.g., all invocations of a specific getter g could allow e.g! without a lint message, because the declaration of g has @nonNullChecksExpected. Well, how lucky I didn't say that ;-).