Implement null_safety rule

[asashour]

Implements the first requirement of #2702

I am not sure how best to process pubspec.yaml

Should the name be sound_null_safety, unsound_null_safety, ...?

[coveralls]

Coverage increased (+0.01%) to 95.83% when pulling 4568fe5 on asashour:2702_null_safety into 54b1663 on dart-lang:main.

[bwilkerson]

Thanks!

You should be able to handle the pubspec case by overriding PubspecVisitor getPubspecVisitor() in your subclass of LintRule. I haven't verified that it will give you access to everything you need, but we can extend it if necessary.

[bwilkerson]

Although it isn't strictly a requirement, consider naming this with a verb phrase, something like enable_null_safety. We generally try to name lints so that they describe either what should be done or what shouldn't be done.

[asashour]

I see, it is now enable_null_safety. Another suggestion is ensure_sound_null_safety.

[bwilkerson]

@pq Do we want to merge this now (well, after the conflict is resolved) and risk it being a breaking change to have additional checks, or would it be better to merge this after the next roll into the SDK so that it can't be used yet? A third option would be to mark it as experimental until the pubspec checking has been added. (FYI, Flutter is interested in this lint.)

[pq]

I don't know how expensive the comment parsing below is but I wonder if it's worth bailing if non-nullability is enabled?

    if (context.isEnabled(Feature.non_nullable)) {
      return;
    }

[bwilkerson]

The comments have already been parsed and are in the AST so it's fairly inexpensive to iterate over them, but it wouldn't hurt to check.

[pq]

👍

More specifically I mean the cost of the regexps.

[asashour]

Thanks for hinting.

if (context.isEnabled(Feature.non_nullable)) {
  return;
}

doesn't work, giving all cases are ok (2 should fail).

On the other hand, there was also a suggestion to do something like:

if (line.replaceAll(' ', '').startsWith('//@dart=')) {
    // parse the version
}

if there is a concern about regExp.

[bwilkerson]

It might not be too bad, given that we only need to look at the comments before the first non-comment token in the file (modulo the corner case below). The performance is then dependent on the length of the file header and library documentation comments.

[srawlins]

I think it would be super good to land this. Would you mind fixing the merge conflict, @asashour ?

[srawlins]

@bwilkerson @pq this is the rule that bans // @dart=2.9, etc. Likely not needed for Dart >= 3.0, but maybe extra useful before then.

[bwilkerson]

It might not be too bad, given that we only need to look at the comments before the first non-comment token in the file (modulo the corner case below). The performance is then dependent on the length of the file header and library documentation comments.

[pq]

👍

[pq]

I'd like to hold off on landing this until after the currently in progress roll is finished. (See: https://dart-review.googlesource.com/c/sdk/+/264961)

Thanks for your patience!