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

ArrayBuffer is not eliminated from union as expected in narrowing of ArrayBuffer | Uint8Array #50714

Closed
Jamesernator opened this issue Sep 11, 2022 · 6 comments
Labels
Duplicate An existing issue was already created

Comments

@Jamesernator
Copy link

Bug Report

πŸ”Ž Search Terms

arraybuffer, narrow, union

πŸ•— Version & Regression Information

  • This is the behavior in every version I tried, and I reviewed the FAQ for entries about ArrayBuffer

⏯ Playground Link

Playground link with relevant code

πŸ’» Code

import fs from "node:fs/promises";

async function writeData(data: Uint8Array | ArrayBuffer): Promise<void> {
    if (data instanceof ArrayBuffer) {
        data = new Uint8Array(data);
    }
    // Argument of type 'Uint8Array | ArrayBuffer' is not assignable to parameter of type 'string | ArrayBufferView | Iterable<string | ArrayBufferView> | AsyncIterable<string | ArrayBufferView> | Stream'.
    //   Type 'ArrayBuffer' is not assignable to type 'string | ArrayBufferView | Iterable<string | ArrayBufferView> | AsyncIterable<string | ArrayBufferView> | Stream'.
    //      Type 'ArrayBuffer' is missing the following properties from type 'Float64Array': BYTES_PER_ELEMENT, buffer, byteOffset, copyWithin, and 24 more.(2345)
    await fs.writeFile("./some-file", data);
}

πŸ™ Actual behavior

Currently ArrayBuffer is not eliminated from the union, so data being passed to fs.writeFile is not of the right type.

πŸ™‚ Expected behavior

I expected the conditional to remove ArrayBuffer from the union, making data's type Uint8Array (after the conditional).

@jcalz
Copy link
Contributor

jcalz commented Sep 11, 2022

Duplicate of #42534, #31311, and ultimately #202. It's hard to fix this without breaking other things, apparently.

@Jamesernator
Copy link
Author

Jamesernator commented Sep 11, 2022

Duplicate of #42534, #31311, and ultimately #202. It's hard to fix this without breaking other things, apparently.

I saw those issues, but even with them, they don't explain why narrowing shouldn't work here. Like even if TS (incorrectly) assumes Uint8Array <: ArrayBuffer, the conditional data instanceof ArrayBuffer should be recognized as always true, in which case data = new Uint8Array(data) should always narrow data to Uint8Array anyway.

An even simpler case (that would be incorrect at runtime, but TS shouldn't notice) that currently fails is (and should be what TS effectively sees for the above conditional):

import fs from "node:fs/promises";

async function writeData(data: Uint8Array | ArrayBuffer): Promise<void> {
    data = new Uint8Array(data);
    await fs.writeFile("./some-file", data);
}

Like why is data still inferred as Uint8Array | ArrayBuffer once it hits the second line of the function? It should be narrowed to the subtype Uint8Array as it's clearly been set to the subtype.

@jcalz
Copy link
Contributor

jcalz commented Sep 11, 2022

Okay, I see. So maybe the minimal repro here should be something like

function test(x: Uint8Array) {
    const y: Uint8Array | ArrayBuffer = x; // okay
    y // still Uint8Array | ArrayBuffer, no union narrowing here?!
    const z: Uint8Array = y; // error
}

and the question is why the assignment fails to narrow. You might want to put the stripped down repro at the top of the issue and mention how this differs from #31311, although presumably interested parties would be reading these comments too.

@jcalz
Copy link
Contributor

jcalz commented Sep 11, 2022

So maybe this is a combination of #42534 (array buffers and typed arrays are erroneously compatible) and #47731 (subtype reduction doesn't consistently happen upon assignment)?

@andrewbranch andrewbranch added the Duplicate An existing issue was already created label Sep 16, 2022
@typescript-bot
Copy link
Collaborator

This issue has been marked as a 'Duplicate' and has seen no recent activity. It has been automatically closed for house-keeping purposes.

Offroaders123 added a commit to Offroaders123/NBTify that referenced this issue Dec 6, 2022
Changes:
- Updates the names of variables and parameters that deal with the tag IDs/types to all be consistently named `type`, rather than `tag`. I'm trying to make it consistent to where I refer to where the raw tag prefix be referenced as the tag type, and the live value itself be the tag.
- Moved the internal private `TextEncoder()` off of the writer class, and onto it's own local constant within the writing module. This makes it consistent to the `TextDecoder()` over in the write module.
- Did some other formatting and renaming too, along to match the same changes as above.

Fixes:
- The resulting Uint8Array from the `NBTWriter` now slices the allocated data like it used to. Using `subarray()` isn't the right fit here, as I do actually want to return an `ArrayBuffer` with the new size of the slice, rather than a smaller view on the same data.

Was going to try adding support for accepting both either the `ArrayBufferLike` or `Uint8Array` types, but looks like TypeScript doesn't have enough type information to decipher the difference between the two when declaring them in a union type, unfortunately. This repo issue covers it nicely:
microsoft/TypeScript#50714

Turns out I also noticed this a while back when attempting to assign a `Uint8Array` to a `DataView`, which passes the type checker's concerns, but is not supported on the runtime side, and is invalid. It's because `ArrayBuffer` isn't strictly-typed-differently enough to compare against the `TypedArray` types, as they are similar enough to both pass, which is a coincidence/overlap kind of thing. They have similar properties, which both have the same type declarations, but not specified enough to discern each other from the other. This is the same issue that I ran into with my Primitive Class constructors, but I figured out how to prevent that from working by defining the `[Symbol.toStringTag]` property. `ArrayBuffer` does this as a `string`, while `Uint8Array` is a constant `"Uint8Array"`. So, in the TS side of things, `Uint8Array`s will validate where you can use `ArrayBuffer`s, but not the other way around. I saw that you can try to use `interface` narrowing to locally add a custom `toStringTag` definition for `ArrayBuffer`, but that didn't work for me.

It's a bit hectic just for handling all of that still, so I'm gonna hold off trying to add that for a little bit. Looks like `fs.writeFile()` accepts `TypedArray`s and not raw `ArrayBuffer`s, interestingly, so maybe I don't need to support raw `ArrayBuffer`s either. It does seem to be more standard to use `TypedArray`s for API design IO. The only one that is different from the others is `fetch()`, which returns raw `ArrayBuffer`s. I think it would be great to just call `fetch("./bigtest/nbt").then(response => response.arrayBuffer()).then(NBT.read)`, rather than having to add a `new Uint8Array()` constructor in the middle there. So I think I'll probably get around to just accepting both, since in the end it does make things easier to work with. I'm gonna look more into it in the meantime before directly implementing it :)

Have had barely any issues with TypeScript thus far, and this one is kind of minute. Very impressed! The other thing that I want to make a feature request for, which isn't a feature yet, is to add `satisfies` support to `interface` declarations, that way you can type check a new `interface` against another `interface` which has an index signature. This would prevent the type checked interface from allowing any key, because it doesn't have the index signature *on it*. It's only using it to check it. That would be sick!!!! I'd use it like this, ideally: `interface BedrockLevelDat {} satisfies CompoundTag`

I love documenting all of this, it's nice to write all of it out.
@Offroaders123
Copy link

Offroaders123 commented Dec 13, 2022

Came up with a great solution! I'm using this in my own library at the moment.

ArrayBuffer.toString() is simply on the prototype, inherited from Object, so there's nothing directly defined on ArrayBuffer for toString() itself.

I tried a few different ways to make it work, like a facade property definition, but that was fairly ugly, as it added a property that didn't exist at all, and it wasn't real.

This on the other hand, toString() already exists, and this simply changes the return type from string to a more specific "[object ArrayBuffer]". So happy this solved it! Hopefully it works in your use case too.

// global.d.ts

declare global {
  interface ArrayBuffer {
    toString(): "[object ArrayBuffer]";
  }
}

export {};

Another note: I defined this inside of my project's global.d.ts file, so I think that means it won't leak out of your own codebase, and into other type definitions. If that's not the case, definitely let me know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Duplicate An existing issue was already created
Projects
None yet
Development

No branches or pull requests

5 participants