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

Fix object decode #241

Merged
merged 3 commits into from
Apr 7, 2024
Merged

Conversation

omochi
Copy link
Contributor

@omochi omochi commented Apr 7, 2024

The current JSObject.construct(from:) failed with JSValues where its case is function, symbol, or bigInt.

For example, JSFunction.construct succeeds with case function and JSObject.construct doesn't, but it should succeed because JSFunction is a JSObject.

symbol and bigint are primitive values in JavaScript side, and because of:

Symbol("s") instanceof Object // false

It's unclear if JSObject.construct(symbol) should succeed or nor, but I think it should because JSSymbol is a JSObject.

Note for function case:

(() => {}) instanceof Object // true

従来の実装では、JSValue の case が function, symbol, bigInt であるような JSValue を、
JSObject.construct(from:) に渡した場合にデコードに失敗していました。

例えば JSFunction.construct に case function を渡す場合は成功しますが、
値がfunction であるとわからない状況においても、
JSFunction is JSObject である以上、 JSObject としてのデコードが成功する必要があります。

symbol と bigInt については JavaScript側においてはプリミティブであり、

Symbol("s") instanceof Object // false

なので、 JSObject としてデコードできるのが正しいのかよくわからないですが、
少なくとも JSSymbol is JSObject となっている以上は、
そうなるべきだと思います。

ちなみに function については

(() => {}) instanceof Object // true

です。

case .object(let object):
return object as? Self
case .function(let function):
return function as? Self
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as? Self のキャストがある事で、
このメソッドが JSObject.construct として呼ばれた時と、
JSFunction.construct として呼ばれた時だけ通ります。
JSSymbol.construct として呼ばれている場合は nil になります。
そのため、各種の型でのオーバライド実装が不要になって、
class func から static func に変更できます。

@omochi omochi force-pushed the fix-object-decode branch from f00dc46 to 38afefd Compare April 7, 2024 07:26
Copy link
Member

@kateinoigakukun kateinoigakukun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems legit to me, thanks!

@kateinoigakukun kateinoigakukun merged commit 384d668 into swiftwasm:main Apr 7, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants