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

Parameter type inference/capture depends on the order of declaration of fields in an object. #56297

Closed
MatAtBread opened this issue Nov 2, 2023 · 8 comments
Labels
Duplicate An existing issue was already created

Comments

@MatAtBread
Copy link

MatAtBread commented Nov 2, 2023

🔎 Search Terms

parameter type inference capture
order of fields
order of declaration

🕗 Version & Regression Information

  • This changed between versions 4.6.4 and 4.7.4

In 4.6.4 is was consistently broken in that it just didn't work irrespective of the order the fields are declared. From 4.7.4 onwards it works if methods are declared before init, but not if init is declared before methods.

It remains with this inconsistent behaviour in 5.3.0-beta

⏯ Playground Link

https://www.typescriptlang.org/play?ts=5.2.2#code/CYUwxgNghgTiAEAzArgOzAFwJYHtXxAA8MRVRgAeABQD4AKURALngG8AoeeAWxAwAscwAM4B+FlQDcneFlRYM4+HShMqASngBeGvCioAntIC+6lmgDWqHAHdU09mDzCM8ALLaCxUuTocuvAJCwiz+XPAARrB0mqzwcBjIMPgAjPDGMsYANDJyCiqxMlwJSfhQAHRRMDFcAPS1erLC8AAGcVUxLCmS6S2Z7KYOTqgu8ABynkQkZCDAfrnyGAVsRfF8pXqV0Zr1jVjNLZbWdn1c2TKBgiKhqx2xa4nJ8GkZZwPq0kA

💻 Code

declare function extended<P>(def: {
  methods?: P;
  init?: (a:P) => any;
}): unknown;

const M = extended({
  methods: {
    bar() { return 1 }
  },
  init(a) {
    return a.bar()  // a is `{ bar(): 1; }`
  }
});

const N = extended({
  init(a) {
    return a.bar() // a is `unknown`
  },
  methods: {
    bar() { return 1 }
  }
});

🙁 Actual behavior

const M works as expected.

const N has errors dereferencing a in all locations. Hovering reveals that in N, a is unknown

🙂 Expected behavior

In N.init(), a should be:

a: {
    bar(): 1;
}

...as it is in M, not unknown

Additional information about the issue

This is a heavily simplified snippet from a codebase that defines a form of class/object hierarchy.

I've never come across an issue where the order that fields are declared in an object affect how types are inferred or captured.

Any ideas where I might start looking into a fix, or a workaround (other than swapping the order in all my declarations!).

@MatAtBread
Copy link
Author

Thinking about it, this is probably a result of where the parser first finds a reference to P, in order to infer it's type. In N it is a naked, untyped parameter ('unknown' is sensible default). In the former, the first reference is to the methods member which is typed through capturing the definition.

Perhaps the type inference engine should have some algorithm to evaluate all the references to a type parameter, and use the "narrowest" definition by default, rather than just the first?

Although I'm a reasonably experienced TS user, my knowledge of the compiler is pretty rudimentary. If someone more knowledgeable knows where this is implemented, please point me at it, and I'll have a go at a PR.

@MatAtBread
Copy link
Author

The other possibility is more syntax to highlight where a type parameter should be captured (or ignored) if it appears more than once, so that I can give the compiler a hint, e.g:

declare function extended<P>(def: {
  methods?: infer P;  // TELL THE COMPILER THE P WE'RE INTERESTED IN IS HERE
  init?: (a:P) => any; // THIS IS JUST A REFERENCE TO THE CAPTURED P
}): unknown;

@Andarist
Copy link
Contributor

Andarist commented Nov 2, 2023

This is a current limitation of the so-called intra-expression inference (introduced in TS 4.7). The producer of a type has to come up before the consumer. The current "catch-all" issue for further improvements of this algorithm can be found here

@MatAtBread
Copy link
Author

Thanks for the swift feedback!

I didn't find that class of issues when I was searching, but I agree it's a similar case. I'm not sure why the fix (as described here) doesn't cover my case, but it doesn't.

I have a different example where the overriding issue - being able to specify the capture site - would again avoid the problem (it's completely different: a recursive type that parses ElasticSearch aggregation specifications to generate the appropriate response types).

Perhaps that's a more generic (no pun intended), if somewhat manual, solution to this class of problem?

@Andarist
Copy link
Contributor

Andarist commented Nov 3, 2023

I'm not sure why the fix (as described here) doesn't cover my case, but it doesn't.

It's how it's implemented. The output of any function can be used as input to any other function. TypeScript doesn't form any form of a dependency between them and it cannot loop through the set of involved expressions indefinitely until no new information can be received (that would likely be a perf killer). So the algorithm is a one-way street - no backsies.

It has to settle the parameter type of a function at some point - so at that point it uses the information available to it at that point. And yes, it iterates through your expression object expression in the source order. So when it sees the producer first then it's able to settle its type and infer from its return type. When it gets to the consumer the inferred type for its parameter is already available and it works.

It doesn't work if you put your consumer before the producer because the consumer gets visited first and its type has to be settled. But since we haven't visited producer yet... well, there is no information from which we could infer this.

Remember that you could get some crazy cases like:

declare function test<T1, T2, T3, T4, T5>(obj: {
  a: (arg: T5) => T4;
  b: (arg: T4) => T1;
  c: (arg: T1) => T5;
  d: () => T1;
  e: (arg: T2) => T3;
  f: (arg: T4) => T2;
}): void;

Given that TS can't "sort" those in some kind of a topological order... this is impossible to fix without incurring a big perf cost on the algorithm.

I have a different example where the overriding issue - being able to specify the capture site - would again avoid the problem (it's completely different: a recursive type that parses ElasticSearch aggregation specifications to generate the appropriate response types).

Perhaps that's a more generic (no pun intended), if somewhat manual, solution to this class of problem?

I'd need some examples to comment on this

@MatAtBread
Copy link
Author

Interesting. I'll leave the recursive example for another time until I've cut it down to a reasonable size (or better understood the cause).

In the case we're considering, it works as expected if the definition is changed to bar:() => { return 1 }, which @ahejlsberg discusses in #48538 (comment), tho in all honesty I don't understand why this is less possible in contextual expressions.

It's complicated by the fact that in my production code, it's not a parameter to init that is typed, but actually this, so the context is a requirement and so as a workaround ()=>{} fails as it masks this.

I think don't understand why it doesn't work as methods is always a Record-like object, and never a function. If I add a constraint to P so that:

declare function extended<P extends Record<string,unknown>>(def: {
  methods?: P;
  init?: (a:P) => any;
}): unknown;

play link

...meaning that there's no ambiguity in the context of P as it's not the result of a function, but is (in practice) always captured from a literal.

If I change the member of this literal to a non-function, it works, but adding back any context-dependent function breaks all the references within the literal, not just the function here.

Even if the function within methods is not referenced in any way, prevents the type capture, which would appear an unnecessary restriction on the fix.

const N = extended({
  init(a) {
    return a.baz // a.baz is `unknown`
  },
  methods: {
    baz: 1,
    bar() { } // Remove this declaration, and a.baz above works as expected
  }
});

@MatAtBread
Copy link
Author

...but I totally accept this is a complicated issue, and that the resolution of a this kind of type-graph (where all nodes on the graph appear in different parts of the AST each of which must satisfy different constraints).

The common theme when I have this kind of issue is that an analyser will struggle to work out where the graph (esp if cyclic) "starts". This is often implicit in JS/TS, since it relies on the developer using convention to supply the information, hence my suggestion that the developer be able to specify where the type is to be initialised from.

@RyanCavanaugh RyanCavanaugh added the Duplicate An existing issue was already created label Nov 3, 2023
@typescript-bot
Copy link
Collaborator

This issue has been marked as "Duplicate" and has seen no recent activity. It has been automatically closed for house-keeping purposes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Duplicate An existing issue was already created
Projects
None yet
Development

No branches or pull requests

4 participants