Skip to content
This repository has been archived by the owner on Apr 8, 2020. It is now read-only.

toString implementation on i32 produces surprising results #108

Open
amgando opened this issue Jan 11, 2020 · 10 comments
Open

toString implementation on i32 produces surprising results #108

amgando opened this issue Jan 11, 2020 · 10 comments

Comments

@amgando
Copy link

amgando commented Jan 11, 2020

steps to recreate

let price = "8157";

// (1) typecasting we use in our tests
// https://github.com/nearprotocol/near-runtime-ts/blob/master/tests/assembly/hello/main.ts#L55

let btcPrice: i32 = I32.parseInt(price);
logging.log(btcPrice.toString()) 

// produces 8157 <-- correct

// (2) documented typecasting
// https://docs.assemblyscript.org/basics/types#casting

let btcPrice: i32 = i32(price);
logging.log(btcPrice.toString()) 

// produces 4824 <-- incorrect

i'm not sure where I32 comes from -- the capital letter makes me think it's a boxed type vs the lower case utility functions documented on the AS website but this is out of my comfort zone.

@DanielRX
Copy link
Contributor

Just from what I would imagine (putting the wasm code in would help), this is casting the string to a number, but that means it's casting the location of the string? I could be completely wrong

@bowenwang1996
Copy link
Contributor

I am pretty sure that <T>expression is a pointer cast. If you look at what the documentation says, it pretty much tells you that you can cast between those number types, but nothing else.

@willemneal
Copy link
Contributor

You can also use changetype<T>(...) which works for everything, but as @DanielRX mentioned this would result in casting the local value, e.g. the pointer location, to become an integer.

As for I32 it is a the class definition for i32, but not a box. It is a name space for the static function and also allows you to have methods on the primitive, e.g. (<i32>1234).toString().

@willemneal
Copy link
Contributor

willemneal commented Jan 12, 2020

It also seems that <T>expr can be used for everything, but it isn't portable for <i32>, since Typescript only has a single number type, so i32() is used to force it to be an integer.

// portable
let someFloat: f32 = 1.5
let someInt: i32 = i32(someFloat)

becomes

var someFloat = 1.5
var someInt = someFloat | 0

See here.

@amgando
Copy link
Author

amgando commented Jan 14, 2020

added by @vgrichina via Slack thread
image

@amgando
Copy link
Author

amgando commented Jan 14, 2020

everyone replying here seems clear on this, it's just my confusion.

this kind of thinking is out of my comfort zone at the moment so i'll dive into the AssemblyScript docs more deeply this week and next.

I've spent most of my time over the past 10 years in Ruby and JavaScript and have never written anything meaningful in C beyond the most basic intros. clearly this is a gap in my knowledge that I can address.

I also expect this gap will be common among 80% of JavaScript developers

@willemneal
Copy link
Contributor

Yeah ideally you shouldn't have to deal with pointers or manual memory management. But perhaps it would be a good idea drafting an article or documentation aimed at JavaScript developers.

@dcodeIO
Copy link

dcodeIO commented Jan 15, 2020

Appears that i32(someStr) not emitting an error is a bug. The explicit conversion functions currently deal with basic types only, and here it is erroneously converting the pointer value, which is bad in various ways.

Regarding I32.parseInt, that is essentially one of the AS equivalents to JS's Number.parseInt, but yielding a value of the more concrete i32 type. There is one such upper-case "wrapper" class per basic numeric type, similar to how there are multiple basic types instead of just number.

Potential enhancements there are to explicitly allow/implement i32(someStr) conversions and perhaps making the global parseInt (currently returning f64) a context-sensitive builtin yielding a value of the (integer) type indicated by surrounding code.

@bowenwang1996
Copy link
Contributor

@dcodeIO Thanks for the fix! I would say i32(expr) feels like pointer cast to me so maybe it's not a good idea to implement semantic conversion there.

@MaxGraey
Copy link

MaxGraey commented Jan 16, 2020

@bowenwang1996 I guess I32(num) (not i32(num)) should be exists because it works little bit different (more stricter) than I32.parseInt / Number.parseInt. For example:

console.log(parseFloat('3a')); // => 3
console.log(Number.parseFloat('3a')); // => 3
console.log(Number('3a')); // NaN

for I32('3a') it could be zero because (NaN | 0) == 0

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

No branches or pull requests

7 participants