proposal: Consider named constructors as nonobvious for "obvious type" lints

[loic-sharma]

Problem

It is not obvious from a call site whether Foo.bar(...) is a static method or a named constructor. As a result, the return type of Foo.bar(...) is non-obvious.

Proposal

Update the following lints to consider named constructors to have non-obvious types:

1. omit_obvious_local_variable_types
2. omit_obvious_property_types
3. specify_nonobvious_local_variable_types
4. specify_nonobvious_property_types

Example

class MediaQuery {
  MediaQuery.removeViewPadding(...) {}
  static Widget fromView(...) => ...;
}

// Explicit local variables - types are obvious
final Widget one = MediaQuery.fromView(...);
final MediaQuery two = MediaQuery.removeViewPadding(...);

// After - what's the type of `one`? It's not obvious
final one = MediaQuery.fromView(...);
final MediaQuery two = MediaQuery.removeViewPadding(...);

See also: MediaQuery.fromView, MediaQuery.removeViewPadding.

Counterarguments

Changing the behavior of an existing lint

Changing existing lints can be painful. For example, this can cause your project to fail to build when you update your Dart SDK.

However, I suspect that changing this behavior is acceptable as:

1. All affected lints are marked experimental.
2. All affected lints are used by less than 1% of all users. Google internal dashboard.

Current behavior is beneficial for more experienced developer

For someone aware that the lint is active, the very absence of a type declaration signals that the right-hand side must be a named constructor call whose type is considered unambiguous by the tool. This provides a subtle informational cue, but it was acknowledged that this is an "advanced" reading of the code that relies on prior knowledge of the linting rules.

While this might be true, I'd argue this is orthogonal to the lints' goal of specifying non-obvious types. This named constructor behavior is non-obvious! :)

cc @eernstg

[lrhn]

If you name your named constructors suitably, it should be possible to distinguish them from methods.

And it's ok to name a static method like a constructor of it returns an instance of the type.

  MediaQuery.withoutViewPadding(...) {}
  static MediaWidget fromView(...) => ...;
  static Widget widgetFromView(...) => ...;

If you name things confusingly, everything can be unobvious.

[loic-sharma]

@lrhn I agree we shouldn't name things confusingly, but reality is difficult. The Flutter framework has an enormous API surface. We receive a firehose of contributions from the community, which we try to clean up, but things slip. And we can't make breaking changes willy nilly, which makes it hard to clean up the cruft.

The goal of specify_nonobvious_local_variable_types is to make sure local variable types are obvious. Unfortunately, there are real-world cases where this lint does not do this. The lint can be a little more pragmatic :)

[eernstg]

@loic-sharma, let's forget about the potential breakage for now, based on the low numbers. This is likely to change radically if Flutter uses some or all of those lints, so we have a window at this time where changes aren't too hard to handle.

I think it's a very interesting idea to special-case instance creation expressions (that is, constructor invocations) when they call a constructor whose name is of the form C.name where C is the class name.

We could do that, and it might be more consistent than the current approach.

---

Still, let me mention a couple of arguments in favor of the current approach:

If we start "treating C() better than C.name()", developers might start worrying about which constructor should have the "good" name (the one that allows clients to have more concise code). This could give rise to naming designs where a constructor gets the good name C because it is expected to be used most frequently, and other constructors will have to live with C.someName, even in the case where the privileged constructor whose name is C would really be more comprehensible at call sites if it had had a name of the form C.veryInformativeName.

As you mentioned in the section 'Current behavior is beneficial for more experienced developer', I've argued that a reader of the code can actually use the knowledge about the enabled lints to reach a conclusion:

Currently, instance creation expressions are classified as obvious when they create an instance of a non-generic class, or they create an instance of a generic class, and the actual type arguments have been provided explicitly.

Static method invocations are classified as non-obvious (and I don't think anybody is suggesting that this should change).

This means that if specify_nonobvious_local_variable_types is enabled then we can actually recognize the constructor invocations because they have no type annotation (so we know that the type can be read off of the initializing expression), or it doesn't matter because there is a type annotation (which may or may not be needed according to the lint).

If both specify_nonobvious_local_variable_types and omit_obvious_local_variable_types are enabled, we also know that the instance creation expressions that don't require type inference definitely do not have a type annotation, and static method invocations definitely have it.

Another thing to consider is that we may need to push harder on lints/warnings about type arguments which have been omitted in type annotations:

class C<X>.name(final X x); // Using a primary constructor.

void main() {
  var c1 = C.name(1); // `specify_nonobvious_..` lints this.
  var c2 = C<int>.name(1); // No lint messages, type is now obvious.
  C c3 = C.name(1); // No lint messages, but it is now a `C<dynamic>`.
  C c4 = .name(1); // No lint messages, but it is again a `C<dynamic>`.
}

If a developer writes a declaration like c1 then specify_nonobvious_local_variable_types will lint it because the type isn't obvious (this is true when the name of the constructor is C.name and also when it is C). One way to fix this is shown in the declaration of c2. Another action which might be taken is like c3, adding a type annotation.

However, the instance creation in c1 is subject to type inference, and the resulting type argument is int (based on the declaration of the constructor and the type of the actual argument). In contrast, c3 has the declared type C which means C<dynamic> (we might change that to C<Object?>, but that hasn't happened yet), and the initializing expression C.name(1) will then be inferred as C<dynamic>.name(1).

With dot shorthands, we might even see a lot of declarations like c4 where the duplication of the class name has been eliminated ... but the resulting object (and the declared type of the variable) is still C<dynamic>.

In terms of type safety, c1 and c2 are arguably preferable to c3 and c4. This could be an argument in favor of writing this kind of initializing expression as in c1 or c2, rather than pushing the community in the direction of c4.

[loic-sharma]

@eernstg Thank you for taking the time to write such a rich response, it is very much appreciated :)

... developers might start worrying about which constructor should have the "good" name (the one that allows clients to have more concise code) ...

This is an interesting argument, but I'm personally not too concerned.

The Flutter app ecosystem overwhelmingly uses Effective Dart; fewer than 1% of Dart developers use specify_nonobvious_local_variable_types. I don't expect this to change, even after we promote specify_nonobvious_local_variable_types out of experimental status - Flutter app developers tend to prefer concise code.

Given this, the Flutter framework should optimize its API design for Effective Dart, which always uses type inference for local variables regardless of whether the type is obvious. In other words, Flutter should not design APIs such that we optimize C() to be a "good" name for concise code. If this starts becoming a concern for Flutter contributors, I would be happy to amend our style guide to advise against this "good" name pattern.

Does that address this particular argument?

In terms of type safety, c1 and c2 are arguably preferable to c3 and c4. This could be an argument in favor of writing this kind of initializing expression as in c1 or c2, rather than pushing the community in the direction of c4.

Agreed 👍

At a high-level, the guidance we're considering for Flutter's style guide is: when declaring a variable, decide whether you want to infer the variable's type or not first, and then based on that, decide if you're still able to use a dot shorthand for the initializer. As a result, c1 and c2 should be preferred over c3 and c4 for local variables.

  C c3 = C.name(1); // No lint messages, but it is now a `C<dynamic>`.
  C c4 = .name(1); // No lint messages, but it is again a `C<dynamic>`.

Thanks for bubbling this up! This is concerning - we plan to allow c4 for things like public fields. Would it make sense to prevent c4 by adding a new always_specify_generic_type_arguments that would be compatible with always_specify_property_types (see #61899) and specify_non_obvious_local_variable_types?

[FMorschel]

Would it make sense to prevent c4 by adding a new always_specify_generic_type_arguments

We already have strict-inference and strict-raw-types (docs) that do something similar, but as errors (on analysis_options.yaml):

analyzer:
  language:
    strict-casts: true # Never casts dynamic to anything by itself
    strict-inference: true # Never infers type arguments on assignments
    strict-raw-types: true # Never infers type arguments on declarations 

An error for each of the above in order

void f(String s) {
  f(jsonDecode(s)); // argument_type_not_assignable
  var map = {}; // inference_failure_on_collection_literal
  List list = [1, 2, 3]; // strict_raw_type
}

[eernstg]

Here's an implementation to play around with: https://dart-review.googlesource.com/c/sdk/+/459743.

Note that this changes the treatment of C.name() such that it is considered to have a non-obvious type, but it still considers C<int>.name() to have an obvious type: The placement of the actual type arguments implies that it is an instance creation.

[loic-sharma]

The placement of the actual type arguments implies that it is an instance creation

That sounds reasonable to me 👍

[lrhn]

I think making named constructors be unobvious would be a set-back in more cases than it would be a benefit.

Take:

var bigInteger = BigInt.from(-1); 
final now = DateTime.now();  
final string = String.fromCharCodes([68]);  
final fileDirectoryWindows = Uri.directory(r'C:\data\images', windows: true);

all of which occurs in the Dart platform library documentation, or similar code like:

var random = Random.secure();
var timer = Timer.periodic(const Duration(seconds: 1), callback);
var newSpec = ZoneSpecification.from(oldSpec, handleUncaughtError: errorHandler);
var jsonCodec = JsonCodec.withReviver(_reviver);
var bytes = Uint8List.fromList(numbers);

All of these are properly named constructors, with names that do not look like a method.
If all of those would need to repeat the class name, it would not make the code more readable, just more redundant.

It would be a reason for me to not want to use (or recommend) the lint. (Not saying I would otherwise, but it definitely wouldn't help.)

I expect most named constructors are distinguishable from methods by their name, which means that
making all (non-generic) named constructors count as non-obvious would be a net negative.

[loic-sharma]

all of which occurs in the Dart platform library documentation

I'd argue that the type of Random.secure(), Timer.periodic(), and JsonCodec.withReviver() are not immediately obvious. I don't use these APIs frequently. These could easily be static methods that return types like SecureRandom, PeriodicTimer, and ReviverJsonCodec respectively.

A middle ground might be to consider Dart SDK named constructors as obvious... This would not be my preference though, but I'm happy to compromise.

I think making named constructors be unobvious would be a set-back in more cases than it would be a benefit.

Please keep in mind that "obvious type" lints are designed for folks that believe explicit types improve readability regardless of the added verbosity. They would prefer to have some redundant information in obvious locations than to omit information in some nonobvious locations.