Skip to content

StaticArray#concat and #slice are brittle #2580

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

Open
dcodeIO opened this issue Nov 24, 2022 · 2 comments
Open

StaticArray#concat and #slice are brittle #2580

dcodeIO opened this issue Nov 24, 2022 · 2 comments

Comments

@dcodeIO
Copy link
Member

dcodeIO commented Nov 24, 2022

As part of the unification of StaticArray's static and instance methods in #2404, now accounting for both Array<T> and StaticArray<T> results via a type parameter in instance methods, code like the following was introduced:

let out!: U;
if (out instanceof Array<T>) {
  out = changetype<U>(__newArray(length, alignof<T>(), idof<Array<T>>()));
} else if (out instanceof StaticArray<T>) {
  out = changetype<U>(__new(size, idof<StaticArray<T>>()));
} else {
  ERROR(...)
}

where U is also the return type of the respective function. This code is brittle for several reasons:

1) If an U triggers a dynamic instanceof check, null is passed to the instanceof helper, exploding with an OOB memory access. This can potentially be addressed with a hack like

let out = changetype<U>(this);

so there is at least some value, which would also work with #2578 (where I noticed this) once merged, BUT will stop working with Wasm GC because the assignment is invalid codegen.

2) Both concat<U> and slice<U> return U, but the actual value returned is either Array<T> or StaticArray<T> that is merely changetype'd to U, which is unsound if U is a subclass. Not an issue for StaticArray since it is final, but for Array. Even if an U would be __newed, the subclass would not always be properly initialized because the constructor isn't called.

3) As mentioned, tricks like these likely won't work forever anyhow.

These seem like reasons to me to revert the PR in question, unless there is a solution I haven't thought of. cc @MaxGraey as the author of the code.

@dcodeIO dcodeIO added the bug label Nov 24, 2022
@CountBleck
Copy link
Member

CountBleck commented Dec 18, 2022

unless there is a solution I haven't thought of

Since these types are known at compile-time, an instanceof<A, B>() builtin could possibly help?

@dcodeIO
Copy link
Member Author

dcodeIO commented Dec 19, 2022

For problem 1), I think so. The others are a little more tricky and seem to indicate that there should perhaps be separate methods for Array and StaticArray, as it was before.

@dcodeIO dcodeIO added enhancement and removed bug labels Jan 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants