Skip to content

Fixed contextually typed expando members #704

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

Closed
wants to merge 3 commits into from

Conversation

Andarist
Copy link
Contributor

No description provided.

if links.resolvedType == nil {
expando := c.getSymbolOfExpando(symbol.ValueDeclaration)
if expando != nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this ports Strada's mergeJSSymbols - but since JS is not really involved here right now, I decided to just inline its relevant parts here (at least for the time being)

const c: Combo = () => 1
>c : Combo
->() => 1 : { (): number; p: {}; }
+>() => 1 : { (): number; p: { [s: string]: number; }; }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have not investigated why this is different but I genuinely feel like this is an improvement. It's not even a user-facing - or I don't know how to position my cursor in this example to get such quick info.

If we think about types alone then we can test how this behaves if we start assigning properties to p (TS playground):

interface Combo {
  (): number;
  p?: { [s: string]: number };
}
const c: Combo = () => 1;
c.p = {};

c.p.testOk = 1; // ok
c.p.testError = "foo"; // error

So it's not really correct, in my eyes, to say that p has {} type. At any stage of the process. Either we know here about everything about expando members or we shouldn't know anything (if this would print a flow type or smth).

If I get the .types print for the above in Strada I end up with this:

interface Combo {
  (): number;
  p?: { [s: string]: number };
>p : { [s: string]: number; } | undefined
>  : ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>s : string
>  : ^^^^^^
}
const c: Combo = () => 1;
>c : Combo
>  : ^^^^^
>() => 1 : { (): number; p: {}; }
>        : ^^^^^^^^^^^^^^^^^^^^^^
>1 : 1
>  : ^

c.p = {};
>c.p = {} : {}
>         : ^^
>c.p : { [s: string]: number; }
>    : ^^^^^^^^^^^^^^^^^^^^^^^^
>c : Combo
>  : ^^^^^
>p : { [s: string]: number; }
>  : ^^^^^^^^^^^^^^^^^^^^^^^^
>{} : {}
>   : ^^

c.p.testOk = 1;
>c.p.testOk = 1 : 1
>               : ^
>c.p.testOk : number
>           : ^^^^^^
>c.p : { [s: string]: number; }
>    : ^^^^^^^^^^^^^^^^^^^^^^^^
>c : Combo
>  : ^^^^^
>p : { [s: string]: number; }
>  : ^^^^^^^^^^^^^^^^^^^^^^^^
>testOk : number
>       : ^^^^^^
>1 : 1
>  : ^

c.p.testError = "foo";
>c.p.testError = "foo" : "foo"
>                      : ^^^^^
>c.p.testError : number
>              : ^^^^^^
>c.p : { [s: string]: number; }
>    : ^^^^^^^^^^^^^^^^^^^^^^^^
>c : Combo
>  : ^^^^^
>p : { [s: string]: number; }
>  : ^^^^^^^^^^^^^^^^^^^^^^^^
>testError : number
>          : ^^^^^^
>"foo" : "foo"
>      : ^^^^^

@ahejlsberg
Copy link
Member

ahejlsberg commented Mar 25, 2025

I don't think we need any of this logic. I already have a fix for the errors in the propertyAssignmentUseParentType1.ts test and it will be in my next PR. Beyond that, the other test differences in this PR don't look right.

In case you're curious, the fix we need is in the canGetContextualTypeForAssignmentDeclaration function. The first line of code should be changed to:

symbol := c.getExportSymbolOfValueSymbolIfExported(c.getResolvedSymbol(node.Expression()))

@Andarist
Copy link
Contributor Author

Beyond that, the other test differences in this PR don't look right.

Hm, I took another look at them right now and both look pretty OK to me. expandoFunctionContextualTypes matches Strada here. The other one (typeFromPropertyAssignment30) also looks OK to me, I mentioned that in here. It would be interesting to hear your take on it (or both?) - perhaps my mental model for how this is supposed to work is off. Merged symbols and alikes are always somewhat confusing to me ;p

In case you're curious, the fix we need is in the canGetContextualTypeForAssignmentDeclaration function

Cool. That makes sense! I was looking at that function but I followed the Strada's compatibility~ (in terms of how some of those involved nodes/structures were connected) instead 😅

@Andarist Andarist closed this Mar 25, 2025
@ahejlsberg
Copy link
Member

Some of the code in Strada related to expando properties is overly complicated and at times incorrect, in particular when it comes to our support for inference in .js files. In general, the implementation of our support for .js and JSDoc is going to differ from Strada. For example, see #610.

Conceptually, in a construct like the following

const fn: SomeType = () => { ... }
fn.prop = someExpr

the type of the expression () => { ... } is augmented with the expando properties assigned to fn, regardless of any type annotation on fn. However, when a type annotation is present on fn, that type serves as the contextual type for the expression used to initialize expando properties. So above, someExpr is contextually typed by SomeType["prop"].

@Andarist Andarist deleted the fix/contextual-expandp branch March 25, 2025 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants