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

5.6 regression: Incorrect param type inference for type with all optional props #59656

Open
trevorade opened this issue Aug 16, 2024 · 9 comments Β· May be fixed by #59709
Open

5.6 regression: Incorrect param type inference for type with all optional props #59656

trevorade opened this issue Aug 16, 2024 · 9 comments Β· May be fixed by #59709
Labels
Bug A bug in TypeScript Help Wanted You can do this
Milestone

Comments

@trevorade
Copy link

trevorade commented Aug 16, 2024

πŸ”Ž Search Terms

inference, param

πŸ•— Version & Regression Information

  • This changed between versions 5.5.4 and 5.6.0

⏯ Playground Link

https://www.typescriptlang.org/play/?ts=5.6.0-dev.20240816#code/JYOwLgpgTgZghgYwgAgMoHsC2EAqBPABxQG8AoZC5AeiuQHkRkBWAOgDYWAGAGmQgA8iUYNnDIARnmSY4Aa1ABzZGAAWwAM7ICUdAWRQIARwCuwAwBMW5SsZDATEAAo6CAfgBcydWGEgFAblIAX1JScwgEABs4A2QEdBBvZHRxdU86VOgANzhxSIgAHgxsfCIAPkDSGFsEMGAE5BiFAHEIMHUAdR0-UogACgBKdMyoHLzC4iCy5DJKfTbjKEYU9RYCYCI+sDgCPqbkAF5p2bnKJsDTyhoAPVdrS5pkAFFBCMhzT2LcQhRQZhZWAAWe6nR4AQVqxjgkU8FEmyD+WVYHE4AFpxG04Mp0P8UajwlkWAAmThEwGcAAcAEY2Mg9mBkPk4N4BiCggMBoEQlUanUGk1Wu0AMLoKAGWq9QYzEEGMCLZapNYbfrbXb7I7Sy4Uc4guY3O5a6i0MGRADucDwmniYrebI5XNCoEgsEQKAy6myuXyBRwxxB6yIBTBZT6unSQjgYFFADFefUQD7eMGhvQRmNvcGHU7oPAkMgAKogGJ4WMgWrxxPIABKxzp6nQiyQnhwKar-mQIWzLrzdAjUagpfLCUrNb4-EgIHMmkLxcHfIT7s94x9ZV4i9GXsKNdrnfAOddyAAsgl0L1e9BIzG48PfWOJ1P6H2r2X55Xb8QO2EItFYtUX-HlB2Fc+hAAQwE8PockiYwIGbAZDmmLJ0GAcwU2PEBTx+c8oEvAdrwTX1AiAA

πŸ’» Code

interface SomeType {
    // On 5.6.0, experiment by making this prop required.
    uniqueProp?: string;
}

declare const obs: Observable<SomeType>;

function argGetsWrongType(): Observable<{}> {
    return obs.pipe(tap(arg => {
        arg;
        //^?
        // Expected: SomeType in 5.5.4
        // Actual:   {} in v5.6.0-beta to 5.6.0-dev.20240816 (at least)
    }));
}

function argGetsCorrectType() {
    return obs.pipe(tap(arg => {
        arg;
        //^?
        // Always correct
    }));
}

interface Observable<T> {
    pipe<A>(op: OperatorFunction<T, A>): Observable<A>;
}
interface UnaryFunction<T, R> { (source: T): R; }
interface OperatorFunction<T, R> extends UnaryFunction<Observable<T>, Observable<R>> { }
interface MonoTypeOperatorFunction<T> extends OperatorFunction<T, T> { }
declare function tap<T>(next: (value: T) => void): MonoTypeOperatorFunction<T>;

πŸ™ Actual behavior

arg is inferred as {}

πŸ™‚ Expected behavior

arg is inferred as SomeType as was consistent in v5.5.4

Additional information about the issue

(Google note: See http://cl/664889390 for local workaround)

@Andarist
Copy link
Contributor

Andarist commented Aug 16, 2024

Bisects to this diff, so the change likely was introduced by #57909

@trevorade
Copy link
Author

trevorade commented Aug 16, 2024

FWIW, in Google's codebase, I'm seeing other new inference related bugs popping up. For the life of me, I can't make a simplified repro for one particular case that I've been looking at for a while...

I'll try to add other repros when I can...

@Andarist
Copy link
Contributor

I’d appreciate any kind of repros - they could even be not-so-minimal πŸ˜‰

@RyanCavanaugh
Copy link
Member

A useful observation is that this can be fixed by a single variance annotation

interface Observable<in T> {

which implies we're likely measuring the variance wrong.

@RyanCavanaugh RyanCavanaugh added Bug A bug in TypeScript Help Wanted You can do this labels Aug 16, 2024
@RyanCavanaugh RyanCavanaugh added this to the Backlog milestone Aug 16, 2024
@trevorade
Copy link
Author

I’d appreciate any kind of repros - they could even be not-so-minimal πŸ˜‰

Yeah. This one is just some internal team who has produced a pipeline with some pretty complicated TS types and inference is breaking in a new way. I'll be looking more at overall compilation issues in the codebase and will try to highlight other similar issues when they seem relevant.

@Andarist
Copy link
Contributor

This changes the behavior even in 5.5:

interface Observable<T> {
  pipe<R>(op: OperatorFunction<T, R>): Observable<R>;
}

type OperatorFunction<T, R> = (source: Observable<T>) => Observable<R>;

-declare function tap<T>(next: (value: T) => void): OperatorFunction<T, T>;

+interface UnaryFunction<T, R> { (source: T): R; }
+declare function tap<T>(next: (value: T) => void): UnaryFunction<Observable<T>, Observable<T>>

declare const obs: Observable<{ a?: string; b?: number }>;

function test(): Observable<{ a?: string }> {
  return obs.pipe(
    tap((tapped) => {
      tapped;
      // ^?
    }),
  );
}

And, like Ryan suspected, T in Observable<T> is measured to have VarianceFlags.Independent here. That means it was first measured to have VarianceFlags.Bivariant (TS playground):

interface Observable<T> {
  pipe<R>(op: OperatorFunction<T, R>): Observable<R>;
}

type OperatorFunction<T, R> = (source: Observable<T>) => Observable<R>;

type Test1 = Observable<string | number> extends Observable<string> ? 1 : 0;
//   ^? type Test1 = 1
type Test2 = Observable<string> extends Observable<string | number> ? 1 : 0;
//   ^? type Test2 = 1

Declaring pipe as a property instead of a method doesn't change this (TS playground).

Clearly, we get different results here when either type arguments-based or structure-based inference is used. For now, I'll look into understanding what changed for the latter with my PR.

@trevorade
Copy link
Author

By the way, I've identified a number of other new compilation issues a little different from this that I believe is related to this issue.

Once there is a new 5.6.0 build with this PR, I can see if it resolves the other new issues I've been seeing.

In this example, basically, inference appears to be dropping optional parameters from a type or making them required. I haven't dug into it super deep. If this PR doesn't fix the issue, I'll try to boil it down to a repro.

@jakebailey
Copy link
Member

Can you try the build on #59709 (comment) to see if that fixes your issues?

@trevorade
Copy link
Author

Hey there. Confirmed that this fix does fix the specific issue noted here.

That said, I am still seeing other new inference-related bugs that I was hoping this fix would resolve. I'll try to construct repros in TS Playground for those ones and file new issues.

@trevorade trevorade changed the title 5.6.0 regression: Incorrect param type inference for type with all optional props 5.6 regression: Incorrect param type inference for type with all optional props Aug 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Help Wanted You can do this
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants