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

ReadonlySet and ReadonlyMap are not narrowed by instanceof checks #53800

Closed
cevr opened this issue Apr 16, 2023 · 8 comments
Closed

ReadonlySet and ReadonlyMap are not narrowed by instanceof checks #53800

cevr opened this issue Apr 16, 2023 · 8 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@cevr
Copy link

cevr commented Apr 16, 2023

Bug Report

🔎 Search Terms

🕗 Version & Regression Information

I have noticed this bug on the 5.0.4 version of typescript, but I suspect this has been around since ReadonlySet and ReadonlyMap were introduced.

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

⏯ Playground Link

Playground link with relevant code

💻 Code

export function toCollection<T>(value: T): T extends ListLike<infer V> ? V[] : T extends RecordLike<infer V> ? Record<string, V> : never {
  if(isListLike(value)) {
    if(value instanceof Set) {
      return [...value]
    }
    // the resulting type union still includes ReadonlySet
    return value
  }
  if(isRecordLike(value)) {
    if(value instanceof Map) {
      return Object.fromEntries(value.entries())
    }
    // the resulting type union still includes ReadonlyMap
    return value
  }

  throw new Error("not a collection-like")
}


export type ListLike<T> =
  | T[]
  | Set<T>
  | ReadonlyArray<T>
  | ReadonlySet<T>;

export type RecordLike<T> =
  | Record<string, T>
  | Map<string, T>
  | ReadonlyMap<string, T>;

function isListLike<A>(value: unknown): value is ListLike<A> {
  return value instanceof Set || Array.isArray(value)
}

function isRecordLike<A>(value: unknown): value is RecordLike<A> {
  return (
    value instanceof Map ||
    (typeof value === "object" &&
      value !== null &&
      !Array.isArray(value) &&
      !(value instanceof Set))
  );
}

🙁 Actual behavior

ReadonlySet and ReadonlyMap are not narrowed by instanceof Set or instanceof Map conditions

🙂 Expected behavior

I would expect both ReadonlySet and ReadonlyMap to be narrowed by instanceof Set or instanceof Map since they are just qualifiers over Set and Map.

@MartinJohns
Copy link
Contributor

since they are just qualifiers over Set and Map.

But they're not. They're independent interfaces. There's nothing in the type system that links these interfaces to Set and Map,

@cevr
Copy link
Author

cevr commented Apr 16, 2023

@MartinJohns Are you suggesting that a ReadonlyMap is not a Map when compiled?

The expectation is that when checked with instanceof the resulting union includes the readonly variants, not exclude them.

@MartinJohns
Copy link
Contributor

Technically, neither values typed as ReadonlyMap nor Map (the interfaces of TypeScript) have to be instances of the Map class at runtime, the values just have to match the interfaces.

The type system can't really represent what you want. instanceof Map narrows to Map because the value Map is typed as MapConstructor (es2015.collection.d.ts line 36), and the prototype of MapConstructor is typed as Map<any, any> (es2015.collection.d.ts line 34). There's no such thing for ReadonlyMap. It can't narrow to both Map<> and ReadonlyMap<>.

@cevr
Copy link
Author

cevr commented Apr 16, 2023

I see. It's something that can easily worked around in code, I figured it might be possible to make it easier to work with these interfaces in library code.

@robbiespeed
Copy link

Related issue for readonly arrays: #17002

You can get it to narrow properly for your toCollection function by excluding readonly types from the isX functions here, playground link. Which makes more sense anyway in this particular case since readonly types are not readonly at runtime.

@MartinJohns
Copy link
Contributor

since readonly types are not readonly at runtime.

Always important to point out that "readonly" only means "readonly via this interface", it does not mean immutable.

@RyanCavanaugh RyanCavanaugh added the Working as Intended The behavior described is the intended behavior; this is not a bug label Apr 17, 2023
@RyanCavanaugh
Copy link
Member

Not being a Map doesn't mean something isn't a ReadonlyMap, the same way that something not being a dog doesn't mean it's not an animal.

@typescript-bot
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

5 participants