BREAKING CHANGE: Unify Math & Mathf. Make every method in Math polymorphic by MaxGraey · Pull Request #2335 · AssemblyScript/assemblyscript · GitHub
Skip to content

BREAKING CHANGE: Unify Math & Mathf. Make every method in Math polymorphic - #2335

Open
MaxGraey wants to merge 14 commits into
AssemblyScript:mainfrom
MaxGraey:poly-math
Open

MaxGraey wants to merge 14 commits into
AssemblyScript:mainfrom
MaxGraey:poly-math

Conversation

@MaxGraey

@MaxGraey MaxGraey commented Jun 24, 2022 •

Copy link
Copy Markdown
Member

In this PR we're doing several breaking changes:

  • move all detailed implementations to std/util/math
  • make all Math's methods polymorphic
  • remove non-standard NativeMath#scalebn
  • remove non-standard NativeMath#rem
  • remove non-standard NativeMath#mod (but not tests)
  • remove NativeMathf completely
  • for now, we have only single Math.random (for f64 type)

Caveats:

  1. I have observed quite often the script that users from JS for example:
function foo(x: i32, y: i32): i32 {
   let res = Math.floor(x / y) as i32;
   return res;
}

Before this PR this will create unnecessary cast to f64, Math.floor and downcast to i32. With this PR this code now equivalent to x / y without all this unnecessary work due to Math.floor, Math.trunc and etc accept integers and pass through it "as is".

  1. Now following code will generate a compilation error:
const cos3 = Math.cos(3);
// -> ERROR: Math.cos accept only f32 or f64 types
// Solution: it may be fixed as Math.cos(3.0)

const x: i32 = 123;
const sqr = Math.sqrt(x);
// -> ERROR: Math.sqrt accept only f32 or f64 types
// Solution: Math.sqrt(x as f64)

This is the most painful caveat, which can lead to many breaking changes in some code.
Perhaps we could add new generic builtin helper which forces parameter type to float. Something like:

// builtin will be equivalent to this:
type OnlyFloat<T> = T extends f32 | f64 ? T : f64

function sqrt<T extends i32 | i64 | f32 | f64>(x: T): OnlyFloat<T> {
   if (isInteger<T>()) {
     return builtin_sqrt<f64>(x as f64) as OnlyFloat<T>;
   }
   if (isFloat<T>()) {
     return builtin_sqrt<T>(x) as OnlyFloat<T>;
   }
}

// now we can use integer inputs which will forced to f64 for outputs but safely preserve f32
const sqrt3 = sqrt(3); // infer as sqrt<i32>(x) -> f64

Thoughts?

  • I've read the contributing guidelines
  • I've added my name and email to the NOTICE file

@MaxGraey
MaxGraey marked this pull request as ready for review June 25, 2022 09:01
@MaxGraey
MaxGraey requested a review from dcodeIO June 25, 2022 09:01
@dcodeIO

dcodeIO commented Jun 25, 2022 •

Copy link
Copy Markdown
Member

@MaxGraey

MaxGraey commented Jun 25, 2022 •

Copy link
Copy Markdown
Member Author

abs, floor, ceil, round etc will be work properly with integers even in JSMath due to i32 -> f64 -> i32 is invariant. But u32 -> f64 -> u32 or i64 -> f64 -> i64 is not invariant, so this may be a problem. However, I don't think ppl will use that functions for such types. Also, I'm going to add some compiler warnings for integer paths for according methos.

@PaperPrototype

Copy link
Copy Markdown

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants