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

Fix2 get constraint of indexed access #17912

Merged
merged 12 commits into from Jan 10, 2018
Merged

Conversation

sandersn
Copy link
Member

@sandersn sandersn commented Aug 18, 2017

Fixes #17069
Fixes #17166
Fixes #15371
Fixes #19298
Fixes some regressions in RWC.

  1. T[K] now correctly produces number when K extends string, T extends Record<K, number>.
  2. T[K] no longer allows any type to be assigned to it when T extends object, K extends keyof T.

Previously both of these cases failed in getConstraintOfIndexedAccessType because both bases followed K's base constraint to string and then incorrectly produced any for types (like object) with no string index signature. In (1), this produced an error in checkBinaryLikeExpression. In (2), this failed to produce an error in checkTypeRelatedTo.

The fix has some additional pieces.

In order to fix (1), not only does the index type parameter need to be kept around, the base constraint of T extends Record<K, number> needs to be Record<K, number> not {} so that Record<K, number>[K] produces number. So I changed computeBaseConstraint to only chase T extends Record<K, number> back to Record<K, number>, not all the way to {}.

In order to fix (2), any index type whose base constraint is string, whether it came from keyof T or not, is going to (incorrectly) produce any when indexing a type that doesn't have a string index signature -- the test case specifically tries object[string]. So I added an early exit to getConstraintOfIndexedAccessType that returns undefined in this case.

(This is a redo of #17870 after it got merged early by mistake.)

No longer fixes #17847, but previous commits contained a fix for it.

1. `T[K]` now correctly produces `number` when
   `K extends string, T extends Record<K, number>`.

2. `T[K]` no longer allows any type to be assigned to it when
   `T extends object, K extends keyof T`.

Previously both of these cases failed in
getConstraintOfIndexedAccessType because both bases followed `K`'s base
constraint to `string` and then incorrectly produced `any` for types
(like `object`) with no string index signature. In (1), this produced an
error in checkBinaryLikeExpression`. In (2), this failed to produce an
error in `checkTypeRelatedTo`.
@sandersn
Copy link
Member Author

@ahejlsberg this undoes a change you added in the fix for #17521, but doesn't break any tests. I looked at the commit and the associated tests and the rest of the change looks like it handles types with index signatures, not mapped types. Regardless, you should take a look and then we can talk about the right way to fix this bug (#17069) at the same time as #17521's fix.

@weswigham
Copy link
Member

weswigham commented Aug 21, 2017

This fixes some new, erroneous errors from our RWC tests, as in this example:

// @declaration: true
// @jsx: react
// @module: commonjs
// @target: es5
// @experimentalDecorators: true
// @filename: index.ts
declare function cloneDeep(x: any): any;

abstract class ClonableModel<ActualType, CreatableType> {
    constructor(objectToEncapsulate?: CreatableType) {
        if (objectToEncapsulate) {
            for (var key in objectToEncapsulate) {
                if (objectToEncapsulate.hasOwnProperty(key)) {
                    this[key] = objectToEncapsulate[key];
                }
            }
        }
    }

    clone(): ActualType {
        let newT = new (Object.getPrototypeOf(this).constructor)();
        for (var key in this) {
            if (this.hasOwnProperty(key)) {
                const thisProp = this[key];
                if (thisProp instanceof ClonableModel) { // Right now, this line gives an error, but should not
                    newT[key] = thisProp.clone();
                } else {
                    newT[key] = cloneDeep(thisProp);
                }
            }
        }
        return newT;
    }
}

export default ClonableModel;

@DanielRosenwasser
Copy link
Member

Can you look at #17166 and decide whether it is related to this issue?

@sandersn
Copy link
Member Author

It's related but this fix doesn't cover it. It the no-constraint exit explicitly only applies to Ts that have constraints, but whose constraints don't have a string index signature. Of course unconstrained type parameters definitely don't have a constraint with an index signature either. I changed the check slightly and it now catches the first bug.

I still need to look at the second bug in #17166, but it appears to be different.

@sandersn
Copy link
Member Author

The second bug in #17166 is not a bug. It's by design that A[K] is the union of all types of A, for some object type A.

@sandersn
Copy link
Member Author

sandersn commented Dec 4, 2017

@ahejlsberg mind taking a look at this? It sounds like people keep running into it.

After merging with master, even erroneous tests generate types and
symbols baselines
Copy link
Member

@ahejlsberg ahejlsberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved with the suggested name change.

@@ -8350,7 +8356,7 @@ namespace ts {

// Transform an indexed access to a simpler form, if possible. Return the simpler form, or return
// undefined if no transformation is possible.
function getTransformedIndexedAccessType(type: IndexedAccessType): Type {
function simplifyIndexedAccessType(type: IndexedAccessType): Type {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getSimplifiedIndexAccessType

@sandersn sandersn merged commit 40b896a into master Jan 10, 2018
@sandersn sandersn deleted the fix2-getConstraintOfIndexedAccess branch January 10, 2018 23:17
@zpdDG4gta8XKpMCd
Copy link

could this pr break #12215 (comment) ?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Breaking Change Would introduce errors in existing code
Projects
None yet
7 participants