Skip to content

isArrayType declared internally returns type is TypeReference however the converse is not true #55510

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

Closed
NilsIrl opened this issue Aug 25, 2023 · 7 comments
Labels
Duplicate An existing issue was already created

Comments

@NilsIrl
Copy link
Contributor

NilsIrl commented Aug 25, 2023

⚙ Compilation target

N/A

⚙ Library

N/A

Missing / Incorrect Definition

From src/compiler/checker.ts:

isArrayType(type: Type): type is TypeReference;

This implies that if isArrayType returns false then type is not a TypeReference which may not be the case as pointed out by #55010 (comment).

If instead an interface ArrayType was added like so:

interface ArrayType extends TypeReference {}

and isArrayType updated like so:

isArrayType(type: Type): type is ArrayType;

Then the signature would be safe and correct. This will then allow the externally available compiler API to match the internal one. Currently because the internal signature of isArrayType is not safe the external signature instead is:

isArrayType(type: Type): boolean;

Sample Code

if (typeChecker.isArrayType(type)) {
    // complains that type is not TypeReference
    typeChecker.getTypeArguments(type);
}

Documentation Link

Continuation of #55010

@jakebailey
Copy link
Member

jakebailey commented Aug 25, 2023

I don't understand; is this not just #55010, which you filed and we closed as working as intended?

Is there any new info here? I don't see how adding an empty interface would help, either.

@NilsIrl
Copy link
Contributor Author

NilsIrl commented Aug 25, 2023

In #55010, because the proposed solution was not safe/strictly correct, the issue was marked as working as intended. The solution proposed here is safe.

Adding an empty interface helps because if isArrayType returns false. Then the assertion is that type is not an ArrayType but that doesn't imply that type isn't a TypeReference.

@MartinJohns
Copy link
Contributor

MartinJohns commented Aug 25, 2023

Adding an empty interface helps because if isArrayType returns false. Then the assertion is that type is not an ArrayType but that doesn't imply that type isn't a TypeReference.

I don't see how this is true. According to your definition every TypeReference is also a TypeArray because the types are structurally the same.

@andrewbranch andrewbranch added the Duplicate An existing issue was already created label Aug 25, 2023
@typescript-bot
Copy link
Collaborator

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

@typescript-bot typescript-bot closed this as not planned Won't fix, can't repro, duplicate, stale Aug 28, 2023
@NilsIrl
Copy link
Contributor Author

NilsIrl commented Aug 28, 2023

Oops. I didn't realize typescript treats two types as the same if they have the same structure.

Adding a field to make ArrayType and TypeReference distinct would work. Using __brand is conventional according to https://www.typescriptlang.org/play/?q=30#example/nominal-typing

@jakebailey
Copy link
Member

We have many branded types internally; but I think it stands to be seen why this change really needs to be made and whether or not there's some bug this fixes or something.

@NilsIrl
Copy link
Contributor Author

NilsIrl commented Aug 28, 2023

The motivation for fixing this bug is shown in the following example:

if (typeChecker.isArrayType(type)) {
    // complains that type is not TypeReference
    typeChecker.getTypeArguments(type);
}

Currently, either a cast needs to be performed or an extra check needs to be performed.

Otherwise the internal declaration is not strictly correct but it hasn't caused any bugs that I know of.

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