Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Map GLintptr IDL type to Swift Int32 #42

Merged
merged 4 commits into from
May 12, 2022
Merged

Conversation

MaxDesiatov
Copy link
Contributor

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 rustwasm/wasm-bindgen#800.

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`.
@MaxDesiatov MaxDesiatov added the bug Something isn't working label May 12, 2022
@MaxDesiatov MaxDesiatov requested a review from a team May 12, 2022 17:12
@j-f1
Copy link
Member

j-f1 commented May 12, 2022

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
Copy link
Contributor Author

MaxDesiatov commented May 12, 2022

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
Copy link
Member

j-f1 commented May 12, 2022

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
Copy link
Contributor Author

MaxDesiatov commented May 12, 2022

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

@MaxDesiatov MaxDesiatov changed the title Map long long IDL type to Swift Int32 Map GLintptr IDL type to Swift Int32 May 12, 2022
@j-f1
Copy link
Member

j-f1 commented May 12, 2022

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
Copy link
Contributor Author

MaxDesiatov commented May 12, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants