dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.25k stars 1.58k forks source link

[breaking change] Prevent multiplying a Point<int> with a double factor #52937

Open rorystephenson opened 1 year ago

rorystephenson commented 1 year ago

Change Intent

Remove Point's * operator method and add extensions to Point which define * such that only runtime-safe multiplications are possible. This would look something like:

void main() {
  final intPoint = PointExperiment<int>(2, 3);
  final doublePoint = PointExperiment<double>(2.0, 3.0);
  final numPoint = PointExperiment<num>(1, 2);
  // This triggers a compile-time error and analyzer warning:
  // 'A value of type 'double' can't be assigned to a variable of type 'int'.'
  // intPoint * 2.5;

  // Output: PointExperiment<int>(2, 3) * 2 = PointExperiment<int>(4, 6)
  print('$intPoint * 2 = ${intPoint * 2}');
  // Output: PointExperiment<double>(2, 3) * 2.5 = PointExperiment<double>(5, 7.5)
  print('$doublePoint * 2.5 = ${doublePoint * 2.5}');
  // Output: PointExperiment<num>(1, 2) * 2.5 = PointExperiment<num>(2.5, 5)
  print('$numPoint * 2.5 = ${numPoint * 2.5}');
}

class PointExperiment<T extends num> {
  final T x;
  final T y;

  const PointExperiment(this.x, this.y);

  // This is not a proposed change, it is just for printing.
  @override
  String toString() => 'PointExperiment<$T>($x, $y)';
}

extension NumPointExtension on PointExperiment<num> {
  PointExperiment<num> operator *(num factor) {
    return PointExperiment(x * factor, y * factor);
  }
}

extension DoublePointExtension on PointExperiment<double> {
  PointExperiment<double> operator *(num factor) {
    return PointExperiment(x * factor, y * factor);
  }
}

extension IntegerPointExtension on PointExperiment<int> {
  PointExperiment<int> operator *(int factor) {
    return PointExperiment(x * factor, y * factor);
  }
}

Justification

The * operator on Point<T extends num> allows multiplying by a num factor. This was added as a convenience method to allow multiplying a Point<double> with an int factor (this is documented on the method). The issue is that multiplying a Point<int> by adouble factor will cause a runtime error.

This proposed change would still allow multiplying Point<double> and Point<num> with num whilst preventing Point<int> multiplication with num or double and still allowing Point<int> multiplication with int.

Impact

Code that multiplies a Point by a double factor will now trigger the following compile-time error (flagged by the analyzer):

A value of type 'double' can't be assigned to a variable of type 'int'.

Mitigation

Adding the extension methods allows correct use of * whilst preventing Point<int> * double. I'm not sure what else can be done to mitigate the impact.

rorystephenson commented 1 year ago

If you agree with the proposed change I would be happy to draft a PR.

lrhn commented 1 year ago

The Point class is generally bad, I'd love an update on it.

It probably shouldn't have any multiplication at all, it's a point, not a vector.

Its attempt to be generic over T extends num is a failure. It should just have subclasses for integer points, double points, and maybe num points.

Fixing just *, by making it an extension method, could apply to every method, but really requires the class to be final, otherwise we could be breaking subclasses. (And until all code is 3.0 and respects final, that's tricky.) And still only works if all users import the library.

Making it an extension type sounds more promising.

rorystephenson commented 1 year ago

Its attempt to be generic over T extends num is failure. It should just have subclasses for integer points, double points, and maybe num points.

Fixing just *, by making it an extension method, could apply to every method, but really requires the class to be final, otherwise we could be breaking subclasses. (And until all code is 3.0 and respects final, that's tricky.) And still only works if all users import the library.

Making it an extension type sounds more promising.

@lrhn Thanks for the quick response. It's not clear to me what you believe is the best approach? When you say an extension type does that mean defining extensions as I have done or something else?

lrhn commented 1 year ago

An extension type is a new feature in development, which allows creating a new type with a different view on an existing type.