deprecate avoid_returning_null

[pq]

See: dart-archive/linter#2632 (comment)

[bwilkerson]

FWIW, it isn't clear to me that the rule has any value in a null safe world. If the return type is non-nullable, then returning null is a compiler error and a lint is useless. If the return type is nullable, then I'm proposing that we not produce a lint. Hence, I think the right course of action is to deprecate the lint and disable it in null safe code.

[pq]

Agreed.

[subzero911]

False positive

It states: "Avoid returning null from members whose return type is bool, double, int, or num"
but return type is not int but int?
consider improving this linter rule to distinct nullable types

[pq]

@subzero911: we've been mulling over whether there's value in this lint really at all in the presence of nullability. Is there any reason you still have it enabled?

[subzero911]

Most likely, no.
If your type is int / bool / double / num, you can't return null (because these types are not nullable themselves).
If your type is int? / bool? / double? / num?, it is supposed that you can assign null to it, so the lint makes no sense.

[pq]

Thanks for the feedback. Much appreciated!

[pq]

dart-archive/linter#2750 addresses the false positive.

[pq]

Pushing ahead on this, I wonder if anyone has any misgivings wrt to deprecating avoid_returning_null.

/cc @bwilkerson @eernstg @lrhn ?

[bwilkerson]

The only question in my mind is how long we want to support users that have not fully migrated to null-safety. It might still be useful for code that's opted out.

[eernstg]

I can't see any situations in code with null safety where avoid_returning_null is helpful. Could it be deprecated only for code with language version >=2.12?

[lrhn]

I'm agnostic towards keeping the lint. In non-null-safe code, you should just disable it.

[pq]

Thanks for the feedback!

Could it be deprecated only for code with language version >=2.12?

I was thinking about this too. We don't currently have a mechanism but our notion of Maturity is pretty simplistic and could easily be evolved. (We could add a new one or make Deprecated configurable, or ...)

I guess the question is just whether the complexity is worth it (and if it'd come in handy elsewhere) or if as @lrhn suggested, folks should just bite the bullet and disable the lint in non-null-safe code.

@bwilkerson: I'd be curious to get your thoughts?

(@srawlins fyi, this is somewhat relevant to our discussion of tags vs. categories in #57719.)

[bwilkerson]

We have several lints that are only enabled for certain ranges of language versions, and we could certainly do the same here, but I think it's useful to make a distinction between the language versions for which a rule can be applied and whether we still support the rule at all (which is what I think 'deprecated' means: we're removing support for the deprecated item). If it's deprecated then we produce a warning in the analysis options file, when it's remove we generate an error, but there shouldn't be any diagnostic if the rule is still supported but might not apply to all code.

[eernstg]

I understood @lrhn's idea to be that this lint is only enabled when it is applied to code in a library with version 2.12 or higher. Sounds like that would work just fine.

[pq]

@bwilkerson: that's a great point. We do have a number of lints that, like this one, contain a check like this:

    // This lint does not make sense in the context of nullability.
    if (!context.isEnabled(Feature.non_nullable)) {
      var visitor = _Visitor(this);
      registry.addFunctionExpression(this, visitor);
      registry.addMethodDeclaration(this, visitor);
    }

I wonder if it would be worth surfacing this in the lint description? (It feels like it would.) Aside: if we configured lints this way we could even push the conditional call to registerNodeProcessors into the LintRule base class which seems like a win.

[bwilkerson]

I understood @lrhn's idea to be that this lint is only enabled when it is applied to code in a library with version 2.12 or higher.

Isn't that backward? The original supposition is that the lint has no value in null-safe code, so we'd only want to enable it in code with a version prior to 2.12.

I wonder if it would be worth surfacing this in the lint description?

Yes, absolutely.

[lrhn]

Yep, that! Lint should be enabled only in pre 2.12 code, because in 2.12+ code it can't trigger without also being a compile-time error anyway.

[pq]

This is done.