Non-nullable analysis flow fails at recognizing nullcheck

[Rudiksz]

On Dart 2.12.0-76.0.dev (dev) and windows I have the following code that gives a compile error.

An expression whose value can be 'null' must be null-checked before it can be dereferenced.

class Database {
//...
  static final Map<String, void Function()> _notificationsReadyCallbacks = {};
//...

  static void notificationsReadyCallback(
    ffi.Pointer<ffi.Void> context,
    ffi.Pointer<cbl.CBLDatabase> db,
  ) {
    final id = cbl.utf8ToStr(context.cast());
    if (_notificationsReadyCallbacks[id] != null) {
      _notificationsReadyCallbacks[id]();  // This line here 
    }
  }
//...
}

The workaround below works, but it's a needless complication and something I bump into more and more.

    final id = cbl.utf8ToStr(context.cast());
    final callback = _notificationsReadyCallbacks[id];
    if (callback != null) {
        callback(); 
    }

[MarvinHannott]

The compiler would have to prove that id and _notificationsReadyCallbacks didn't change. I don't think that is going to happen. Besides, you now avoid one additional lookup, so your "workaround" is actually more optimal.

But you could even get rid of the whole null-check with _notificationsReadyCallbacks[id]?.call().

[Rudiksz]

But you could even get rid of the whole null-check with _notificationsReadyCallbacks[id]?.call().

Well, that's a null-check too. I was not aware of this syntax, it works like a charm.

I have another case where no other type of null-check works, I'm assuming because the field is static.

typedef CBLLogCallback = void Function(
    CBLLogDomain domain, CBLLogLevel level, String msg);

class Log {
  static CBLLogCallback? _callback;

  static void _logCallbackListener(dynamic message) {

    if (_callback != null) {
      // Error: An expression whose value can be 'null' must be null-checked before it can be dereferenced.
      _callback(
        CBLLogDomain.all,
        CBLLogLevel.info,
        'a',
      );
    }

    if (_callback == null) {
      return;
    }

    // Error: An expression whose value can be 'null' must be null-checked before it can be dereferenced.
    _callback(
      CBLLogDomain.all,
      CBLLogLevel.info,
      'a',
    );
  
    // OK
    _callback?.call(
      CBLLogDomain.all,
      CBLLogLevel.info,
      'a',
    );

  }
}

I understand nnbd is still in beta and if this is the expected behavior, hopefully it gets documented in the future. Other than these few edge cases migrating my code was fairly straightforward.

[MarvinHannott]

I have another case where no other type of null-check works, I'm assuming because the field is static.

Again, the compiler would have to prove that _callback didn't change, which is hard to do considering that this could even happen in an asynchronous control-flow . Maybe it works when it were final?

[Rudiksz]

Reading the announcement article, flow analysis sounded like a cool feature, but its usefulness seems to be limited to local variables, which I barely have any.

I gave up on trying to use it, and I migrated my code to ?. syntax.

[eernstg]

@Rudiksz wrote:

flow analysis sounded like a cool feature, but its usefulness seems
to be limited to local variables

Flow analysis is never applied to any other entity than a local variable. Note that 'local variable' here covers formal parameters of enclosing functions as well as "regular" local variables (that is, the ones whose declaration is derived from <localVariableDeclaration>).

In general, any action that could run unknown code (that is, any instance method invocation and any invocation of a first-class function object) could potentially call code that changes the value of a static or top-level variable. If the variable under scrutiny is an instance variable then the same thing could happen, but also: its getter could be overridden in a subclass of the statically known type of this, and that getter could run arbitrary code and return a different object each time.

In both cases it is unsound to rely on a test in the past to conclude that the value still isn't null (or that it still has any particular property that we may have tested).

@MarvinHannott wrote:

Maybe it works when it were final?

There have been many proposals for enhancements to type promotions, e.g., to have a dynamically checked variant (cf. dart-lang/language#1188). Dynamically checked promotions would allow essentially all of these use cases, but they would of course be unsafe in the sense that they could throw at run time.

Specifically allowing promotions to take place with a static or top-level final variable is likely to be sound (because it is a bug if user code can evaluate such a variable twice and get distinct results). So that would be a quite promising enhancement, and it would not rely on dynamic checks.

However, you could argue that this solution is still half-baked, because each local context (each function body, initializing expression, etc.) would have to re-discover that said static/top-level variable value has a specific property (e.g., it is non-null; or it has a specific type which is not guaranteed because it isn't a supertype of the declared type). Hence, it would provide exactly the same level of convenience as the technique where the static/top-level variable is stored in a local variable.

PS: @MarvinHannott, thanks for giving such precise explanations!