Skip to content

instanceof does not respect inherited Symbol.hasInstance #56536

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
rotu opened this issue Nov 25, 2023 · 2 comments Β· Fixed by #57602
Closed

instanceof does not respect inherited Symbol.hasInstance #56536

rotu opened this issue Nov 25, 2023 · 2 comments Β· Fixed by #57602
Assignees
Labels
Fix Available A PR has been opened for this issue

Comments

@rotu
Copy link

rotu commented Nov 25, 2023

πŸ”Ž Search Terms

hasinstance, instanceof, 2359, TS2359

πŸ•— Version & Regression Information

⏯ Playground Link

https://www.typescriptlang.org/play?declaration=false&target=9&module=7&ts=5.4.0-dev.20231124#code/C4TwDgpgBAEghgZwJIDsHDigxhA8gMygF4oBvAKCigG0BlEAWwCMB7AGwDoALRVdTHAF0AFAA8AXAFcUAaxQsA7igCU4qK3YRM5AL7lyAegNQWM8vmlZgASxYooDEHwzY8+MVNnylAGihZxeGQ0FxwCZQoqACcIYEko+1EoaxCBCBZCLF19I3Uo0wgUc0sbOwcnVNcCVGAIKIQIK1sUD2k5RRQ-AKDnNIIoADITJgArRuAIyigYuISoJJT+Vwz-bMNjJnyZQuLsUvtHXqr8AHFCuussAB4AYSgIUVqUABMEWF5KsPwAPlavDq64huk2isXiiWSn3SmV0QA

πŸ’» Code

type HasInstanceOf = {
  [Symbol.hasInstance](x:unknown): boolean
}

// ok
function myInstanceOf(x:unknown, c:HasInstanceOf){
  return x instanceof c
}

// broken
function myInstanceOfIntersection(x:unknown, c:HasInstanceOf & object){
  return x instanceof c
}

// broken
function myInstanceOfGeneric<C extends HasInstanceOf>(x:unknown, c:C){
  return x instanceof c
}

πŸ™ Actual behavior

The second and third examples give error TS2359

The right-hand side of an 'instanceof' expression must be either of type 'any', a class, function, or other type assignable to the 'Function' interface type, or an object type with a 'Symbol.hasInstance' method.

In all cases, the right-hand side is an object type with a Symbol.hasInstance method.

πŸ™‚ Expected behavior

I expect all three examples to typecheck.

Additional information about the issue

No response

@fatcerberus
Copy link

does not respect inherited Symbol.hasInstance

For the record, it actually works correctly with an inherited hasInstance:
Playground link

So the issue is specifically with intersections and generics.

@rotu
Copy link
Author

rotu commented Nov 25, 2023

@fatcerberus well observed!

Also broken with unions:

const Top = {[Symbol.hasInstance]:(x:unknown)=>(true as const)}
const Bot = {[Symbol.hasInstance]:(x:unknown)=>(false as const)}

// broken
function myInstanceOf(x:unknown, c : typeof Top | typeof Bot)
{
  return x instanceof c
}

Workbench Repro

@andrewbranch andrewbranch added the Needs Investigation This issue needs a team member to investigate its status. label Nov 27, 2023
@andrewbranch andrewbranch added this to the TypeScript 5.4.0 milestone Nov 27, 2023
@typescript-bot typescript-bot added Fix Available A PR has been opened for this issue labels Mar 1, 2024
Abe27342 added a commit to microsoft/FluidFramework that referenced this issue Jul 16, 2024
## Description

With the removal of concrete DDS classes, one pattern that's come up
repeatedly is customer code which previously checked `instanceof
MySharedObject` (usually when the code supported multiple shared object
types for whatever reason), which no longer works. This change adds a
drop-in replacement to the public API surface.

## Breaking Changes

As `SharedObjectKind` was marked sealed, this is non-breaking.


## Alternatives Considered

We could expose free functions in each package easily e.g. by using a
helper like this:

```typescript
export function createSharedObjectTypeguard<TSharedObject>(
	kind: ISharedObjectKind<TSharedObject>,
): (loadable: IFluidLoadable) => loadable is IFluidLoadable & TSharedObject {
	const factoryType = kind.getFactory().type;
	return (loadable: IFluidLoadable): loadable is IFluidLoadable & TSharedObject => {
		return isChannel(loadable) && loadable.attributes.type === factoryType;
	};
}
```

Ultimately this will be more code though and arguably less discoverable.

We could also add back support for `instanceof` using
`Symbol.hasInstance` (and the same implementation as `.is`), but due to
microsoft/TypeScript#56536, this won't work
for customers using TS below 5.5, so we'll need something else anyway at
least for now.

---------

Co-authored-by: Abram Sanderson <[email protected]>
Co-authored-by: Craig Macomber (Microsoft) <[email protected]>
RishhiB pushed a commit to RishhiB/FluidFramework-1 that referenced this issue Jul 18, 2024
## Description

With the removal of concrete DDS classes, one pattern that's come up
repeatedly is customer code which previously checked `instanceof
MySharedObject` (usually when the code supported multiple shared object
types for whatever reason), which no longer works. This change adds a
drop-in replacement to the public API surface.

## Breaking Changes

As `SharedObjectKind` was marked sealed, this is non-breaking.


## Alternatives Considered

We could expose free functions in each package easily e.g. by using a
helper like this:

```typescript
export function createSharedObjectTypeguard<TSharedObject>(
	kind: ISharedObjectKind<TSharedObject>,
): (loadable: IFluidLoadable) => loadable is IFluidLoadable & TSharedObject {
	const factoryType = kind.getFactory().type;
	return (loadable: IFluidLoadable): loadable is IFluidLoadable & TSharedObject => {
		return isChannel(loadable) && loadable.attributes.type === factoryType;
	};
}
```

Ultimately this will be more code though and arguably less discoverable.

We could also add back support for `instanceof` using
`Symbol.hasInstance` (and the same implementation as `.is`), but due to
microsoft/TypeScript#56536, this won't work
for customers using TS below 5.5, so we'll need something else anyway at
least for now.

---------

Co-authored-by: Abram Sanderson <[email protected]>
Co-authored-by: Craig Macomber (Microsoft) <[email protected]>
@rbuckton rbuckton removed the Needs Investigation This issue needs a team member to investigate its status. label Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fix Available A PR has been opened for this issue
Projects
None yet
5 participants