null_check_on_nullable_type_parameter: runtime cost difference between t! and t as T

[kevmoo]

Describe the issue
Chatting with @yjbanov about the performance impact of switching many cases of t! to t as T

Should we expect our compilers to "do the right thing" in the cases below?

class Example<T> {
  Example(this._value);
  final T? _value;

  bool transformNullable(bool Function(T value) transformer) =>
      _value == null || transformer(_value!);
}

vs

class Example<T> {
  Example(this._value);
  final T? _value;

  bool transformNullable(bool Function(T value) transformer) =>
      _value == null || transformer(_value as T);
}

[pq]

the performance impact of switching many cases of t! to t as T

What is the performance impact?

/cc @mraleph

[kevmoo]

The code is different with dart2js – but I can't parse if it's any worse or not.

[yjbanov]

as type casts show up in our performance profiles consistently (although we have asked that in -O4 mode they are removed, which so far has been rejected by the dart2js team), so without a compiler guarantee that T? as T is automatically converted into a null check, we should not use this lint by default.

I'm also quite skeptical of this lint. Its effect in practice is to lead people to use the as cast instead, which both reintroduces the problem that the lint is trying to solve and makes Dart programs slower.

[fishythefish]

@kevmoo: Could you share the code difference so we have something concrete to analyze here? In general, I would expect t as T to be slower in dart2js than t!.

Like our other null-checks, t! will generally be lowered using .toString, which is cheap. It can also be optimized away when dart2js knows the receiver is not null, e.g. because a property access has already been performed.

I don't think we currently recognize when t as T is equivalent to a null check. This means that we're performing a type test at runtime. The general case is considerably more expensive than just invoking .toString. For some types, like "primitive" types or interface types that don't require any checking on type arguments, we'll emit something more efficient, like typeof or another property lookup, but even then, there's a small amount of runtime overhead to do this dispatch.

[nshahan]

Maybe the better fix in this case would be to use a local variable and type promotion to avoid the second operation (null check or cast) all together.

class Example<T> {
  Example(this._value);
  final T? _value;

  bool transformNullable(bool Function(T value) transformer) {
      var localValue = _value;
      return localValue == null || transformer(localValue);
}

[kevmoo]

@fishythefish

Look at this repo - and comment out this ignore: https://github.com/google/quiver-dart/blob/master/analysis_options.yaml#L7

[kevmoo]

I think the problem here is that for the tree bits and optional, we should use T extends Object

[sigmundch]

Generally speaking (not just about type parameters), I prefer ! over a cast because it makes the intent much more explicit (a cast unfortunately can hide issues, for example if you picked the wrong type and did a side-cast accidentally). I noticed this often when doing migrations, a cast rarely conveys to a code reviewer the intend: the attention goes to the type in use, not to the fact that the type is non nullable.

That said, it seems to me that this lint is really about raising the fact that a type variable that appears non-nullable can still be a nullable value, as in:

class A<T extends Object?> {
  T doError(T? value) => value!;
}
...
int result = A<int>().doError(null); // intended use, expected runtime error
int? result2 = A<int?>().doError(null); // unintended runtime error

Using ! above is excluding null where null was allowed in the first place.

I wonder whether the lint suggestion message should hint that, even though as T is more appropriate for refining the type, that they should also consider whether using a non-nullable type parameter is more appropriate, like:

class A<T extends Object /*non-nullable!!*/> {
  T noUnintendedErrorAnymore(T? value) => value!;
}
...
int result = A<int>().noUnintendedErrorAnymore(2); // you can't pass `int?` as a type parameter anymore
int result2 = A<int>().noUnintendedErrorAnymore(null); // runtime error as intended

[kevmoo]

I looked more closely at quiver – I think this lint is okay. See here google/quiver-dart#711

CC @yjbanov

[mraleph]

FWIW on VM we turn T? x; x as T into x == null ? x as T : x which removes as check from the hot path. This was important to optimise things like built-in iterators.

[rakudrama]

Like the VM, dart2js compiles T? x; x as T into x == null ? x as T : x here.
We do this for the same reason, so that iterators can use this pattern:

    T? _current;
    T get current => _current as T;

As has been pointed out above, this pattern works correctly when T itself is a type allowing null, like dynamic or int?.

Give that this pattern is used heavily in the built-in iterators, I think we would notice some benchmark regressions if this optimization became disabled.

---

In the original example

class Example<T> {
  Example(this._value);
  final T? _value;

  bool transformNullable(bool Function(T value) transformer) =>
      _value == null || transformer(_value as T);
}

the as T check is completely eliminated by dart2js because dart2js figures out it can re-use the loaded this._value final field value, allowing it to recognize that the nested == null tests are testing the same value, so can be simplified, in effect reducing the x == null ? x as T : x pattern to x.

dart2js would also optimize away the null check if it was written as transformer(_value!).

I like Nick's suggestion of using promotion to infer a non-nullable type since there no check of any kind to be optimized away. The only downside is that when we eventually get better type promotion for fields, the _value as T and _value! versions might be flagged as unnecessary and cleaned up. That is unlikely to happen for the local variables.

[lrhn]

Drive-by-comment: Could x == null ? null as T : x be potentially more efficient, since it knows statically that it just needs to check whether T is nullable.
(As compare to x == null ? x as T : x, which doesn't look like it can promote x to Null, and therefore has to be prepared to look at the runtime type of x, even if it always ends up being the null valye.)

[rakudrama]

@lrhn I don't think null as T would help significantly for dart2js.

Each internal type object (Rti) has an _as property that contains the test. x as T compiles to T._as(x). There are many such _as methods, specialized to the type. For nullable types, the test generally checks for null before doing anything more complicated. If the type is not nullable we will need the type of the argument anyway to report the error.

The VM has similar type-cast stubs.

[yjbanov]

Perhaps the issue is in the semantics of ! operator when applied to generic parameters of unknown nullness? The problem is that T in class Foo<T> is abstract. For a concrete type V, T can either be V or V?. In that case, T? is actually either V? or V??.

Perhaps the semantics of ! when applied to generic parameters should be "remove one question mark". That is, if applied to V? it performs a normal null check. However, if applied to V??, it results in V?, or in other words, always succeeds. In other words, we could think of ! as: if null is not allowed make sure it's not null, otherwise don't do anything.

[nshahan]

Beyond that being a large change of semantics, what if there are three layers of nullability? What if you really want the value to be non-null. Do you use x!!!? For our sanity, we always normalize T?? to T?. There is no notion of a degree of nullability. It is just nullable or non-nullable.

I don't see how the author of the original code example could know how many levels of nullability were being passed in through the type argument.

[yjbanov]

If you really wanted the value to be non-null, wouldn't you have declared <T extends Object>? I only use the V?? notation to demonstrate the abstract nature of <T>. Basically if your type is T? t then t! could mean "assert that t is T", but because T is a template and may actually be V?, then this becomes "assert that t is V?", which seems like a fine statement to assert. Or maybe I'm missing some complication in other scenarios?

[sigmundch]

If I understand correctly your suggestion @yjbanov , you'd like an operator that, given a type X? produces logic equivalent to as X. To avoid confusion, let's say that operator was ^, then you would have:

class A {}
method<T>() {
  A? a = ...;
  T? t = ...;

  A a0 = a^; // equivalent to `a!` and `a as A`, null effectively not allowed
  T t0 = t^; // equivalent to `t as T`,  null allowed if T is nullable
}

I see the value added here: an operator would be less error prone and make more explicit that we are really trying to manage details on nullability and nothing else about the type itself (a cast hides this and makes it very implicit). Potentially backends could optimize t^ better than t as T, but that's unclear at the moment. It could very well be implemented at first using the downcast.

So this brings up two questions for the language folks (@lrhn @leafpetersen):

• Should ! take this new semantics instead, and have different meaning depending on the context?
• If not, is it worth adding a new operator like this? (clearly the ^ was just an illustration, we'd then have to do some bikeshedding to pick the right symbol for it)

[lrhn]

So, t^ takes the type T? and removes one ?, giving a static type of T. It matches this at runtime by doing as T. I assume the conversion, from T? to T is performed at compile time, with no knowledge of the runtime type bound to T.
It maybe be the NOT_NULL function, which converts int?? to int, or do you want to remove only one ??
(Be aware that NOT_NULL(FutureOr<int?>) is FutureOr<int?>, which is still nullable.)

If we have T t2 = ...; var t3 = t2^;, is this allowed? What would it do? (Behave as t2! or just as as T, giving a warning about being unnecessary code?)

I'd recommend just using as T on something of type T?, if that's what you actually want.

I know it's an easy mistake to make, forgetting that the actual value may also be null. (I've made it.)
But changing the ! operator is not really viable at this point, too much code depends on it already. And that code might very well want to throw on all null values.
Adding a new operator seems like it would complicated things for users, more than it would help. They'd have to remember which operator to use when.

Might as well just teach them to use as T.

(I have encouraged our compilers to recognize the pattern of checking X? against X, where X is a type variable. It can be reduced to IsNullable(T) || identical(value, null). If the compiler can check IsNullable(T) efficiently, that may be more efficient than checking the value's runtime type for being a subtype of T.)