site-www icon indicating copy to clipboard operation
site-www copied to clipboard

Discourage use of .runtimeType on the 'Effective Dart: Usage' page

Open sigmundch opened this issue 4 years ago • 8 comments

/cc @munificent

Every now and then a user runs into a problem because they believe they can freely use .runtimeType, but in practice it is an API that needs to be used with a lot of caution. See https://github.com/flutter/flutter/issues/78041

What are your thoughts on using the guide to educate about this issue? While it would be great to fix the language and libraries to avoid such pitfalls (see https://github.com/dart-lang/sdk/issues/46373), maybe our effective guide can encourage positive patterns?

Two requests:

  • [ ] Add a new guidance to encourage is tests instead of .runtimeType == string, and emphasize that the string representation is unstable in production environments, like dart2js -O2 or higher.
  • [ ] Remove the use of runtimeType from the List.from example. And change it to use the recommended pattern. For example:
// Prints: List<int>
print(iterable.toList().runtimeType);

is updated to:

// Prints true
print(iterable.toList is List<int>);

and similarly the bad case as

// Prints false because the type is a List<dynamic> instead
print(List.from(iterable) is List<int>);

Thoughts?

sigmundch avatar Jun 23 '21 17:06 sigmundch

Both of those SGTM.

munificent avatar Jun 23 '21 21:06 munificent

Re-opening because I think @sigmundch would still like some changes to "Effective Dart". I'm not crazy about using is because I don't want users to think subtyping gets involved, but maybe that's the best we can do. Maybe if we are just very explicit, like:

// good:

// Creates a List<int>:
var iterable = [1, 2, 3];

// Creates another List<int>:
var copy = iterable.toList();
print(copy is List<int>); // Prints "true".
print(copy is List<dynamic>); // Prints "false".
// bad:

// Creates a List<int>:
var iterable = [1, 2, 3];

// Creates a List<dynamic>:
var copy = List.from(iterable);
print(copy is List<int>); // Prints "false".
print(copy is List<dynamic>); // Prints "true".

If you want to change the type, then calling List.from() is useful:

var numbers = [1, 2.3, 4]; // List<num>.
numbers.removeAt(1); // Now it only contains integers.
var ints = List<int>.from(numbers);

munificent avatar Aug 10 '21 22:08 munificent

Is there any updates here? Coming from https://github.com/flutter/flutter/issues/78041 My web app depends on runTimeType a lot, because it was developed long time ago when this wasn't a problem and now when I updated this thing its causing trouble on prod. for me.

Live link: https://hopmaldives.com

Exception thrown:

Another exception was thrown: Instance of 'minified:mM<void>'

Attaching video of prod: https://github.com/dart-lang/site-www/assets/43790152/9b6287f1-9313-4361-b216-08b7dc387efc

mhmzdev avatar Jul 06 '23 07:07 mhmzdev

As far as I can see, 'Effective Dart' has just one section talking about run-time types. This section makes the point that it is a bad idea to emulate static overloading based on dynamic type tests. For example:

class A {
  // Hypothetically, assume that we have static overloading and we want this:
  void m(int i) {...}
  void m(String s) {...}

  // Here's one thing we could do, but that's a bad idea:
  void m(Object o) {
    switch (o) {
      case int i: ...
      case String s: ...
    }
  }
}

A similar (but not equivalent) semantics could have been achieved using runtimeType, but that's not really the point here. So unless I failed to find it, Effective Dart doesn't actually give any advice on the use/non-use of runtimeType at all.

Should it?

The main points to note would probably be:

  • runtimeType and == is a near-exact type test, which is fundamentally at odds with the fact that OO code otherwise always allows an expression of static type T to have a run-time type which is some (typically unknown) subtype of T.
  • An invocation of runtimeType on a receiver of type T may cause program execution to be more costly in time and space because it is necessary to preserve type argument information on all objects of type T or any subtype thereof (so just one occurrence of o.runtimeType will preserve everything if the type of o is Object? or dynamic).

Type.toString is a separate topic, and it is already the target of no_runtimeType_toString and avoid_type_to_string. It is probably fine to just target this particular construct with lints.

It isn't easy to come up with a couple of lines of code to illustrate that runtimeType is a serious source of performance issues, but the "near-exact type test" aspect could be addressed directly:

void main() {
  Object o = <num>[1, 2, 3];

  // `o` is an instance of a _subtype_ of `List`.
  print(o.runtimeType == List<num>); // 'false'.
  print(o is List<num>); // 'true'.
}

eernstg avatar Jul 06 '23 08:07 eernstg

So, what would you suggest here? @eernstg Bcz for me the app was developed a long time ago and we were using runTimeType to differentiate between various model classes and the navigate accordingly

mhmzdev avatar Jul 06 '23 08:07 mhmzdev

(Hmmm, I wonder if this is off topic for site-www, should we take it somewhere else? But this kind of discussion is actually relevant if site-www and Effective Dart will be giving some advice about how to eliminate invocations of runtimeType. So let's talk a bit about it here.)

I don't think there's a general strategy which would allow a program that uses runtimeType to stop doing that, and still have the same semantics. You could be using those Type objects in so many ways.

differentiate between various model classes and the navigate accordingly

If you're using something like if (myModel.runtimeType == OneSpecificModel) ... else if ... or a corresponding switch statement then you should be able to replace that by if (myModel is OneSpecificModel) ..., and then make sure the tests are reordered such that you're never testing for a supertype before a subtype (if (o is num) {...} else if (o is int) {...} will never execute the int case).

You could have a more dynamic approach like this:

var someAction = <Type, void Function(SomeType)>{
  OneSpecificModel: (arg) => ...,
  AnotherSpecificModel: (arg) => ...,
  ...
  // And/Or it could be updated dynamically.
}

void main() {
  Model m = ...;
  someAction[m.runtimeType](arg);
}

This is basically an approximation of dynamically adding members to a set of classes. It is a dangerous path to take because it is incomplete by nature (it is a near-exact type test, so subtypes won't inherit anything, they all have to be mentioned explicitly as keys; it does not work when the classes are generic; and so on).

However, you could use the visitor pattern in order to allow "adding members to existing classes without editing them".

Or you could use (receiver) => receiver is OneSpecificModel as a type test function rather than the Type object yielded by using OneSpecificModel as an expression, and then you could have a list containing pairs (aTypeTestFunction, anAction), and search through the list until a type test function says 'true' to your model object. That would be very dynamic, and a bit safer (because it's a subtype test rather than a near-exact type test), but you would still need to ensure that the pairs are ordered correctly.

In short, I think it's really difficult to come up with anything like a general rule for how to eliminate runtimeType, but every concrete case where runtimeType is used may (or may not) have a corresponding strategy for how to do it. The most important point is probably simply to say that it may be costly to use runtimeType, and then each developer/organization would have to think about it. ;-)

eernstg avatar Jul 06 '23 09:07 eernstg

Man you are a life savior @eernstg Thanks a lot

I literally had to remove all the runTimeType comparison, extracted out a function:

 static String propertyType(dynamic property) {
    if (property is Hotel) {
      return "Hotel";
    } else if (property is Resort) {
      return "Resort";
    } else if (property is Diving) {
      return "Diving";
    } else if (property is Excursion) {
      return "Excursion";
    } else if (property is Tour) {
      return "Tour";
    } else if (property is Island) {
      return "Island";
    }

    return "Json";
  }

And used this to navigate in the service, works perfectly fine. Thanks once again from my whole team :))

mhmzdev avatar Jul 06 '23 13:07 mhmzdev

Using the new pattern matching features in Dart 3.0, you could write:

static String propertyType(dynamic property) => switch (property) {
  Hotel() => "Hotel",
  Resort() => "Resort",
  Diving() => "Diving",
  Excursion() => "Excursion",
  Tour() => "Tour",
  Island() => "Island",
  _ => "Json",
};

munificent avatar Sep 05 '23 20:09 munificent