Skip to content

double.clamp should return a double #39652

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Hixie opened this issue Dec 4, 2019 · 6 comments
Closed

double.clamp should return a double #39652

Hixie opened this issue Dec 4, 2019 · 6 comments
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-core type-enhancement A request for a change that isn't a bug

Comments

@Hixie
Copy link
Contributor

Hixie commented Dec 4, 2019

I ran into two issues independently recently that both led me to the same conclusion: double.clamp should just always return a double.

Issue 1: removal of implicit casts (cc @a14n @goderbauer)

Without implicit casts, double.clamp needs continuous babysitting with as to make it work:

  // assume implicit casts are disabled for this code
  void foo(double x) { }
  void main() {
    double y = 1.1;
    foo(y.clamp(0.0, 1.0) as double); // without the as double, this is a compile-time error
  }

It's absurd that you keep having to cast things to double when you are only dealing with doubles.

This is newly important since implicit casts are going away with NNBD.

Issue 2: Accidental ints

Because int literals can masquerade as doubles, it's common to use them. Because clamp takes nums, they don't get considered as doubles there. Because clamp returns its value directly, what appears to just be code dealing in doubles is actually suddenly dealing with ints, which leads to runtime errors:

  void foo(double x) { }
  void main() {
    double y = 1.1;
    foo(y.clamp(0, 1).toDouble()); // without the toDouble, this is a runtime error
  }
@a-siva a-siva added area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-core type-enhancement A request for a change that isn't a bug labels Dec 5, 2019
@a14n
Copy link
Contributor

a14n commented Dec 5, 2019

Related to #30403 where a comment from @floitschG is:

We are dropping clamp from num and specialize the functions on int and double.

@lrhn will there be changes happening with NNBD?

@lrhn
Copy link
Member

lrhn commented Dec 5, 2019

Those changes sadly did not make it into Dart 2.0.

We are aware that some number operations have become very cumbersome (num.remainder, math.pow and num.clamp included). We may want to either special-case them in the static type system or restrict them in some way while adding alternatives.

The current clamp has type num clamp(num, num) and is declared on the num class. It allows you to do num.clamp(1, 2.5) and get either 1 or 2.5 back as result. It's hard to modify that signature, or specified behavior, without potentially breaking existing code, but if we add new methods that you can migrate to (or we can migrate you to automatically), then it might become more palatable.

@Hixie
Copy link
Contributor Author

Hixie commented Dec 5, 2019

You could solve issue 2 above by implying toDouble on the return value, that would be backwards-compatible (may require compiler work to keep it efficient).

Issue 1 is going to become seriously problematic with NNBD. We're already experiencing this in Flutter to the point that we're discussing creating an extension method on double that has the right signature and just using that. It would be very unfortunate if Flutter had to do that given that we're all one team. :-(

@lrhn
Copy link
Member

lrhn commented Dec 6, 2019

Making double.clamp always return a double is a breaking change in the sense that there might exist code which will change behavior.

In practice, I wouldn't bet on any code actually breaking.

The int.clamp issue is worse. I'd probably go for converting the result to double if any argument is a double, and then add a special typing case for that too, like we have for + etc.

Even with special typing rule exceptions, we also need the type inference to understand those exceptions. Otherwise we'll still infer num in some places where we could do better.

Adding specialized methods to int and double is definitely on my radar. Both clamp and pow are candidates.

@dungnv2602
Copy link

I'm falling into toDouble() hell when it comes to clamping doubles :-(

@lrhn
Copy link
Member

lrhn commented Sep 11, 2020

The null safety rules now say that the static type of clamp is double if any argument is double (or if the receiver is double, but double.clamp already said that), and it is int if all arguments and the receiver are ints.

Edit: Actually, the final design has the return type be double if all operands are double, and num otherwise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-core type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

5 participants