GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — BREAKING CHANGE: Unify Math & Mathf. Make every method in Math polymorphic by MaxGraey · Pull Request #2335 · AssemblyScript/assemblyscript

Conversation

@MaxGraey

@MaxGraey MaxGraey commented Jun 24, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
MaxGraey added 3 commits June 24, 2022 23:47
@MaxGraey
refactoring
abf8b69
@MaxGraey
lint
7fafa43
@MaxGraey
cleanups
e746365
@MaxGraey MaxGraey added enhancement performance breaking change standard library compatibility labels Jun 24, 2022
MaxGraey added 5 commits June 25, 2022 08:02
@MaxGraey
defs
3bd9eda
@MaxGraey
remove NativeMath#mod but still test it
c059065
@MaxGraey
skip ts specialise input types as literals and break flow analysis
562d008
@MaxGraey
refactor some definitions
1b66da9
@MaxGraey
Narrow -> Widen
081dd08
@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
MaxGraey added 3 commits June 25, 2022 13:43
@MaxGraey
refactor test utils
fa53220
@MaxGraey
refactor again
94b3e8b
@MaxGraey
remove integer path for Math.log2
0394a49
@dcodeIO

dcodeIO commented Jun 25, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

So far, Math is quite intentionally identical to JS Math, also because the implementations can be switched with --use. Making one generic and the other not seems problematic in this regard. Similarly, this might break portable code relying on both being identical. The mentioned caveat that integer literals will be inferred as i32, which sometimes might either not work or produce unexpected results in the absence of a type parameter, seems quite unfortunate as well. Are you sure this is actually viable?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@MaxGraey
mark as deprecated non-recommended methods
baee9e8
@MaxGraey

MaxGraey commented Jun 25, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

MaxGraey added 2 commits June 25, 2022 21:36
@MaxGraey
add compilation warnings for unnecessary integer operations
69a4fca
@MaxGraey
cleanup
771e21a
@PaperPrototype

PaperPrototype commented Feb 4, 2026

Copy link
Copy Markdown

Has this been solved? It has been almost 3 years since the last commit. Also maybe casting the type to f64 could make sense in this case?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

Reviewers

@dcodeIO dcodeIO Awaiting requested review from dcodeIO

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

breaking change compatibility enhancement performance standard library

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@MaxGraey @dcodeIO @PaperPrototype
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.