The Wayback Machine - https://web.archive.org/web/20201208183224/https://github.com/microsoft/TypeScript/issues/41317
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

'in' should not operate on primitive types #41317

Open
millsp opened this issue Oct 29, 2020 · 7 comments
Open

'in' should not operate on primitive types #41317

millsp opened this issue Oct 29, 2020 · 7 comments

Comments

@millsp
Copy link
Contributor

@millsp millsp commented Oct 29, 2020

TypeScript Version: 4.1.0-dev

Search Terms:
operator in exception crash unhandled

Code

const hasKey = <A, K extends string | number | symbol>(
    thing: A,
    key: K,
): boolean => {
    return key in thing;
};

hasKey(123, 'hello'); // TypeError exception

Expected behavior:

TS should expect A to be an object

Actual behavior:

TS didn't detect the potential crash

Playground Link: https://www.typescriptlang.org/play?ts=4.1.0-beta#code/MYewdgzgLgBAFgQwgaQKYE8YF4YB4CCANDMjKgB5SpgAmEM0ATgJZgDmMAPjGAK4C2AI1SMuDdEJAAbAHwAKAFAxlMKHFZsAXDCJKVAawzbkhBQEptgkNNQIw2GTADee5Y1RReje4cytV6uwA3AoAviEKiCgYcgCMAEwAzMQA5HCoUlIgKWZBMAD0+TAAKugADqgAooyMIKIUwKhlUMzgCkA

Related Issues:

@millsp millsp changed the title Potential crash of operator `in` could be avoided Potential crash of `in` operator could be avoided Oct 29, 2020
@millsp millsp changed the title Potential crash of `in` operator could be avoided Potential crash of `in` operator Oct 29, 2020
@DanielRosenwasser DanielRosenwasser changed the title Potential crash of `in` operator 'in' should not operate on primitive types Oct 29, 2020
@DanielRosenwasser
Copy link
Member

@DanielRosenwasser DanielRosenwasser commented Oct 29, 2020

This is weird because we'd potentially allow String but not string - but that's probably okay.

@DanielRosenwasser
Copy link
Member

@DanielRosenwasser DanielRosenwasser commented Nov 2, 2020

Approved.

To avoid breakage on unconstrained type parameters, we're thinking to check if the type is assignable to the union of all primitive types - or if it contains any primitive type (e.g. number, string, boolean, bigint, symbol, null, undefined). isTypeAssignableToKind might be one thing to use here, if not, just use isAssignableTo.

If that's still too breaky, maybeTypeOfKind is a helper that can do this pretty quickly.

The more thorough version of this would be to check whether the type on the right side of the in has to be assignable to object. We could run that on the RWC.

@DanielRosenwasser
Copy link
Member

@DanielRosenwasser DanielRosenwasser commented Nov 3, 2020

To be explicit, this is all about the type on the right of the in operator. The left can be unconstrained, and maybe we should reconsider that at a later date.

@orange4glace
Copy link

@orange4glace orange4glace commented Nov 6, 2020

If possible, I would like to make a PR for this.

@orange4glace
Copy link

@orange4glace orange4glace commented Nov 7, 2020

Which line should emit error, 1) or 2)?

const hasKey = <A, K extends string | number | symbol>(
    thing: A,
    key: K,
): boolean => {
    return key in thing;
                  ~~~~~~
                  1) TypeError exception
};

hasKey(123, 'hello');
       ~~~
       2) TypeError exception
@andrewbranch
Copy link
Member

@andrewbranch andrewbranch commented Dec 7, 2020

@orange4glace the former. Are you still interested in doing this? The change would be in checker.ts around line 29940:

function checkInExpression(left: Expression, right: Expression, leftType: Type, rightType: Type): Type {
    if (leftType === silentNeverType || rightType === silentNeverType) {
        return silentNeverType;
    }
    leftType = checkNonNullType(leftType, left);
    rightType = checkNonNullType(rightType, right);
    // TypeScript 1.0 spec (April 2014): 4.15.5
    // The in operator requires the left operand to be of type Any, the String primitive type, or the Number primitive type,
    // and the right operand to be of type Any, an object type, or a type parameter type.
    // The result is always of the Boolean primitive type.
    if (!(allTypesAssignableToKind(leftType, TypeFlags.StringLike | TypeFlags.NumberLike | TypeFlags.ESSymbolLike) ||
          isTypeAssignableToKind(leftType, TypeFlags.Index | TypeFlags.TemplateLiteral | TypeFlags.StringMapping | TypeFlags.TypeParameter))) {
        error(left, Diagnostics.The_left_hand_side_of_an_in_expression_must_be_of_type_any_string_number_or_symbol);
    }
    // 👇 here
    if (!allTypesAssignableToKind(rightType, TypeFlags.NonPrimitive | TypeFlags.InstantiableNonPrimitive)) {
        error(right, Diagnostics.The_right_hand_side_of_an_in_expression_must_be_of_type_any_an_object_type_or_a_type_parameter);
    }
    return booleanType;
@orange4glace
Copy link

@orange4glace orange4glace commented Dec 8, 2020

@andrewbranch Sure! It would be very happy if I can do this.
Thanks for the detailed guidance! I'll investgate on this ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.