microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
100.77k stars 12.46k forks source link

generic type parameters, for generic call expressions, not reduced to canonical form, causing type inference malfuction and errors #56702

Closed craigphicks closed 10 months ago

craigphicks commented 10 months ago

πŸ”Ž Search Terms

Couldn't find any issues that match this one sufficiently.

πŸ•— Version & Regression Information

This is the behavior in every version I tried up to 5.3.2.

⏯ Playground Link

playground

πŸ’» Code

import { default as $ } from 'jquery';
import JQuery from 'jquery';

const t1: string | JQuery<HTMLElement> | undefined = $(); // no error

const t2: ((string | JQuery<HTMLElement>) & (string | JQuery<HTMLElement> | undefined)) | undefined = $(); // error
//    ~~
// Type 'JQuery<string | HTMLElement>' is not assignable to type '((string | JQuery<HTMLElement>) & (string | JQuery<HTMLElement> | undefined)) | undefined'.
//  Type 'JQuery<string | HTMLElement>' is not assignable to type 'JQuery<HTMLElement>'.
//    Type 'string | HTMLElement' is not assignable to type 'HTMLElement'.
//      Type 'string' is not assignable to type 'HTMLElement'.(2322)

const t11: string | JQuery<HTMLElement> = $(); // no error

const t12: (string | JQuery<HTMLElement>) & (string | JQuery<HTMLElement> | undefined) = $(); // error
//    ~~
// Type 'JQuery<string | HTMLElement>' is not assignable to type 'string | JQuery<HTMLElement> | (string & JQuery<HTMLElement>) | (JQuery<HTMLElement> & string)'.
//  Type 'JQuery<string | HTMLElement>' is not assignable to type 'JQuery<HTMLElement>'.

const JQuery_string = JQuery<string>; // error
// Type 'string' does not satisfy the constraint 'Element'.(2344)
// Type 'string' does not satisfy the constraint 'HTMLElement'.(2344)
// Type 'string' does not satisfy the constraint 'PlainObject<any>'.(2344)

πŸ™ Actual behavior

  1. Even though the type declarations for t1 and t2 are the same (after reduction), only t2 shows an error. The same relationship exists between t11 and t12.

  2. The return type of assigned to t1 is string | JQuery<HTMLElement> | undefined.

πŸ™‚ Expected behavior

  1. t1 and t2 should have the same error status. Likewise for t11 and t12.

  2. The jQuery function $() has a generic signature <TElement=HTMLElement>() JQuery<TElement>. So the return type assigned to t1 should be JQuery<string> | JQuery<HTMLElement> | JQuery<undefined>.

  3. As shown at the bottom of the code, JQuery<string> is judged to be an invalid instantiation of JQuery. If so, the lines with t1 and t2 should both be errors, but they are not. However, the actual declaration for the interface JQuery seems to be

    interface JQuery<TElement = HTMLElement> extends Iterable<TElement>

    which actually doesn't look as though it should constraint the domain of JQuery to HTMLElement, so perhaps JQuery<string> should not be an error? Furthermore, from within a DefinitelyTyped/semantic-ui test file, JQuery<string> does not incur an error.

Additional information about the issue

Origin of strange declaration type

I explain where the weird declaration types (e.g. (string | JQuery<HTMLElement>) & (string | JQuery<HTMLElement> | undefined)) came from.

The following is quoted and condensed from filles under DefinitelyTyped/semantic-ui-search

import { default as $ } from 'jquery';

interface _Impl {
    stateContext: string | JQuery;
};

type Param =
    & (
        | Pick<_Impl, "stateContext">
    )
    & Partial<Pick<_Impl, keyof _Impl>>;

The type of Param["stateContext"] is indeed the weird type:

type StateContext = Param["stateContext"];
//   ^?  (string | JQuery<HTMLElement>) & (string | JQuery<HTMLElement> | undefined)

The error can then be incurred as follows:

const p: Param = {
    stateContext: $()
//  ~~
// Type 'JQuery<string | HTMLElement>' is not assignable 
// to type '(string | JQuery<HTMLElement>) & (string | JQuery<HTMLElement> | undefined)'.(2322)
}

playground

Relation to issue #56013 and pull #56373

The submitted pull pull #56373 for the bug in issue #56013 resulted in similar errors to this report. That's because the bug #56013 is due to improper caching of generic call expression arguments, resulting in the call expression not be re-evaluated when the context changed. When the fix allows that later evaluation, those latent errors appear in the existing test code under DefinitelyTyped/semantic-ui.

An obvious "patchy" fix would be to write new code to force reduction just in time at the critical point - but it could be called multiple times. A better fix would be to do the reduction once when the declaration is parsed - but there is likely some reason it wasn't done that way. Some feedback from the original designers would be helpful.


UPDATE:

Stand alone code with pseudo jQuery

interface JQuery<TElement = HTMLElement> {
    something: any;
    // Change `[Symbol.iterator]` to `other` and the error goes away
    [Symbol.iterator]: () => {
        // Change `next` to `foo` and the error goes away
        next(): {
            value: TElement;
        }
        | {
            done: true;
            value: any;
        };
    }
}

declare function jQuery<TElement = HTMLElement>(): JQuery<TElement>;

const t11: string | JQuery<HTMLElement> = jQuery(); // no error

const t12: (string | JQuery<HTMLElement>) & (string | JQuery<HTMLElement> | undefined) = jQuery(); // error
//    ~~
// Type 'JQuery<string | HTMLElement>' is not assignable to type '(string | JQuery<HTMLElement>) & (string | JQuery<HTMLElement> | undefined)'.
//   Type 'JQuery<string | HTMLElement>' is not assignable to type 'JQuery<HTMLElement>'.ts(2322)

const ta: string  = jQuery(); // error
//                  ?^ jQuery<HTMLElement>(): JQuery<HTMLElement>
const tb: JQuery<HTMLElement>  = jQuery(); // no error
//                               ^? jQuery<HTMLElement>(): JQuery<HTMLElement>
const tc: string & JQuery<HTMLElement>  = jQuery(); // error
//                                        ^? // jQuery<HTMLElement>(): JQuery<HTMLElement>
const td: JQuery<HTMLElement> & string  = jQuery(); // error
//                                        ?^ // jQuery<string>(): JQuery<string>

playground

Debugging analyis using the above code for the psuedo-JQuery

In the declaration

t12: (string | JQuery<HTMLElement>) & (string | JQuery<HTMLElement> | undefined) = jQuery()

the type

(string | JQuery<HTMLElement>) & (string | JQuery<HTMLElement> | undefined)

is reduced to union of these 4 types

inferTypes tries to infer TElement in

<TElement = HTMLElement>jQuery(): JQuery<TElement>

by independently inferring for TElement from each of the 4 types above, to get the following candidates:

Then it takes the union string | HTMLElement so the final return type is JQuery<string | HTMLElement>.

The most noticeable problem is the result for:

That happens because inferTypes calls getApparentType(JQuery<HTMLElement> & string) which promotes string to String.

Then JQuery<HTMLElement> & String is object whose properties are compared to the properties of JQuery<TElemet>.

They both share [[Symbol.iterator]] as a property the the properties

become a valid source-target pair for inference in the function

function inferFromSignatures(source: Type, target: Type, kind: SignatureKind) {
     const sourceSignatures = getSignaturesOfType(source, kind);
     const sourceLen = sourceSignatures.length;
     if (sourceLen > 0) {
          // We match source and target signatures from the bottom up, and if the source has fewer signatures
          // than the target, we infer from the first source signature to the excess target signatures.
          const targetSignatures = getSignaturesOfType(target, kind);
          const targetLen = targetSignatures.length;
          for (let i = 0; i < targetLen; i++) {
              const sourceIndex = Math.max(sourceLen - targetLen + i, 0);
              inferFromSignature(getBaseSignature(sourceSignatures[sourceIndex]), getErasedSignature(targetSignatures[i]));
          }
     }
}

Each match potentially contributes a candidates. In this case the deep match stack is as follows:

-

and so string gets added as an inferred candidate for TElement.

There are a couple of remarkable things going on here.

One remarkable thing

Using the the return type of the objects iterator for to infer the typeArgument for the object itself. By coincide String and JQuery both have iterators, but the inference seems to be an extraordinary overreach.

Another remarkable thing, that does not directly cause the main topic bug

There are a lot assumptions in inferFromSignatures, and because of that these two inferences give difference results;

The result should not depend upon the order around the & symbol. (However, changes to this "remarkable thing" would not prevent the bug which is the main topic of this post.)

fatcerberus commented 10 months ago

Wow, jQuery is still a thing?

craigphicks commented 10 months ago

@fatcerberus -

The TypeScript CI tests includes DefinitelyTyped which includes semantic-ui which depends on jQuery.

I submitted a fix for #56013 in pull #56373.

The bug in #56013 is improperly caching values of generic call expressions, preventing later correct reevaluation of those call expressions with different generic contexts.

I just so happens that the bug fix #56373 triggered correct reevaluations that revealed the bug in this current bug report. The error reports occurred in the typescript CI tests for DefinitelyTypes/semantics-ui - you can see the list of errors here.

Just to be clear, this bug report #56702 does not use the code in pull #56373 at all, it is only using already released versions.

I've analyzed this pre-existing bug and condensed it down a simple example.

Maybe the original title was better? changed.

fatcerberus commented 10 months ago

@craigphicks To be clear, my comment was a joke - jQuery is very old (2006) and it’s rare that JS middleware remains relevant (let alone actively maintained) for that long! πŸ˜„

craigphicks commented 10 months ago

@fatcerberus - npm jQuery still seems to have million of downloads, whereas semantic-ui is only the thousands, and it is particular usage of $() by semantic-ui in which the latent TypeScript bug is revealed as shown here.

RyanCavanaugh commented 10 months ago

We need a repro of this that doesn't depend on something external as large as jquery

craigphicks commented 10 months ago

@RyanCavanaugh - added as an update to original post. Note the accompanying debugger analysis in case it is helpful.

craigphicks commented 10 months ago

I am closing this because I am adding a little more debugging analysis. When finished I will repost.