swiftwasm / WebAPIKit

Access the DOM and other Web APIs from Swift! (Very much a WIP)
MIT License
61 stars 8 forks source link

Map `GLintptr` IDL type to Swift `Int32` #42

Closed MaxDesiatov closed 2 years ago

MaxDesiatov commented 2 years ago

This fixes an issue with

func vertexAttribPointer(
  index: GLuint, 
  size: GLint, 
  type: GLenum, 
  normalized: GLboolean, 
  stride: GLsizei,
  offset: GLintptr
)

passing offset value of BigInt type to the JS function due to public typealias GLintptr = Int64, which led to can't cast BigInt to number runtime errors.

Either WebGL spec is wrong, or browsers implement it incorrectly. Rust folks had a similar issue and they went with i32, see https://github.com/rustwasm/wasm-bindgen/issues/800.

j-f1 commented 2 years ago

If this is the only type that doesn’t accept a BigInt, could you instead manually define public typealias GLintptr = Int32 (with a comment indicating why this is necessary)?

MaxDesiatov commented 2 years ago

As you can see, there are a few more places from other modules that utilize long long for things like timestamps, I'd be surprised if BigInt works for them though. Maybe we should fully follow Rust's approach and map this to a Int32 | Float64 union type? Do we have such type already BTW?

j-f1 commented 2 years ago

Do we have such type already BTW?

Not as far as I can tell. But such a thing could definitely be added! You’d just need to pre-define an appropriate UnionType in Context.swift and then change long long to reference the appropriate type.

MaxDesiatov commented 2 years ago

Checked a few other places where long long is used, like AudioData, Blob, and File and all of them return a JS number in corresponding properties, not BigInt. I'm 95% sure none of these support BigInt, remaining 5% certainty could be added by writing actual tests. I personally vote for either keeping these Int32 as proposed, or creating a new union type. Just riffing:

enum JSNumber: ExpressibleByIntegerLiteral, ExpressibleByFloatLiteral {
case integer(Int32)
case float(Double)
// ...
}

Should we add such type to JSKit then?

cc @kateinoigakukun

j-f1 commented 2 years ago

I think it should be a protocol that both Int32 and Double conform to, and which itself conforms to enough relevant protocols to be useful as an existential. Then JSValue could simply call Double(foo) before crossing the bridge.

MaxDesiatov commented 2 years ago

Existentials have a substantial overhead, especially too large to be acceptable for primitive types like numbers.

Either way, GLintptr as a pointer type for sure should never be represented as a floating point number. I've added an exception just for this type for now, and FIXME for long long to re-evaluate the situation with the rest of the impacted APIs later separately from this PR.