Skip to content
This repository was archived by the owner on Feb 22, 2018. It is now read-only.

Infer tighter return types for math.max() and math.min() #281

Closed
munificent opened this issue Aug 5, 2015 · 7 comments
Closed

Infer tighter return types for math.max() and math.min() #281

munificent opened this issue Aug 5, 2015 · 7 comments

Comments

@munificent
Copy link
Contributor

Right now, this code:

void expectInt(int i) {}

main() {
  expectInt(math.min(1, 2));
}

generates a warning:

[warning] math.min(1, 2) (num) will need runtime check to cast to type int

It would be really nice if DDC could infer the tighter int return type if it knows the arguments are both ints. A "real" solution likely involves either generic methods:

T min<T extends num>(T a, T b) { ... }

or maybe some kind of TypeScript-style overloading. In the absence of either of those, it could be worth special-casing min and max in the same way that the spec does for the arithmetic operators.

@jmesserly
Copy link
Contributor

Maybe I tend to approach these too much from the JS perspective, but ... we could just treat int and double the same. It doesn't make any difference to the generated code. I'm not even sure we'll actually emit the cast if you wrote it (maybe just as a null check)

@jmesserly
Copy link
Contributor

FWIW, looks like generic methods DEP would fix this: https://github.com/leafpetersen/dep-generic-methods/blob/master/proposal.md#functions

we'll need to implement it soon, at least for the SDK. We're currently ignoring a bunch of unsound casts for a hard coded list of types: https://github.com/dart-lang/dev_compiler/blob/5e0e7d27c36a8d7d6858ddf3b7a66892de6539ae/lib/runtime/_operations.js#L180

see also #28

@vsmenon
Copy link
Contributor

vsmenon commented Aug 5, 2015

We're hitting this several times in Angular too - see:
https://travis-ci.org/angular/angular/jobs/74151416

This should be captured by @leafpetersen 's inference-based generic method work.

@jmesserly
Copy link
Contributor

yeah. I have a quick fix about to send out, but Leaf can definite feel free to delete it after :)

@jmesserly
Copy link
Contributor

@jmesserly
Copy link
Contributor

@munificent this is now fixed! I used your suggestion: special case min/max, until we get generic methods.

I take it you were trying out --strong mode? if you're willing to give it another go, I can kick out a dev compiler release and roll it into SDK, would show up in next SDK dev build. Or you could try pub global activate dev_compiler; dartdevc filename.dart ... should give more or less the same results as analyzer_cli.

@munificent
Copy link
Contributor Author

Awesome, thank you for the quick fix! ❤️

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

No branches or pull requests

3 participants