GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Instantiate this for contextually typed type parameters by sandersn · Pull Request #9746 · microsoft/TypeScript

Conversation

@sandersn

@sandersn Nathan Shively-Sanders (sandersn) commented Jul 14, 2016

Copy link
Copy Markdown
Member

Fixes #9673

Previously this parameters were not instantiated when they came from a contextual signature. To fix this, I instantiate the signature in getThisTypeOfSignature. However, the mapper for the instantiation is only available much earlier, when checking the containing function. So I save the mapper on the containing function's signature — that signature is the one that will end up contextually typing this later.

This doesn't look right, though. The instantiation code is signature = instantiateSignature(signature, signature.mapper); and based on other instantiations in the code I don't think signatures with no target are even supposed to be instantiated, and not with their own mapper.

Anders Hejlsberg (@ahejlsberg) or Mohamed Hegazy (@mhegazy) any ideas on how to do this right? I thought about creating a new signature with the correct target/mapper setup, but I'm not sure how to make sure it gets used in place of the original one.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Nathan Shively-Sanders (sandersn) added 2 commits July 14, 2016 14:17
@sandersn
Instantiate contextual this parameters if needed
ad56220
@sandersn
Test that contextually typed generic this parameters are instantiated
78f807c
@msftclas Microsoft Contribution License Agreements (msftclas) added the cla-not-required label Jul 14, 2016
@sandersn Nathan Shively-Sanders (sandersn) mentioned this pull request Jul 14, 2016
Type inference error with generics and function 'this' typing #9673
Closed
@sandersn Nathan Shively-Sanders (sandersn) mentioned this pull request Aug 2, 2016
Generic type inference does not appear to fill in type of this parameter #10072
Closed
@sandersn

Nathan Shively-Sanders (sandersn) commented Aug 15, 2016

Copy link
Copy Markdown
Member Author

Anders Hejlsberg (@ahejlsberg) can you look at this? I fixed this right before you got back from vacation, then went on vacation myself and forgot about it.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@sandersn
Merge branch 'master' into instantiate-this-for-contextually-typed-ty…
271ffc8 
…pe-parameters
@sandersn

Nathan Shively-Sanders (sandersn) commented Aug 17, 2016

Copy link
Copy Markdown
Member Author

The problem may actually be that thisParameter is instantiated too early compared to other parameters on the signature.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@sandersn
Assign and instantiate contextual this type if not present
0f483d6
sandersn
sandersn reviewed Aug 17, 2016
View reviewed changes
Comment thread src/compiler/checker.ts Outdated
if (thisType) {
return thisType;
}
const type = getContextuallyTypedThisType(container);

@sandersn Nathan Shively-Sanders (sandersn) Aug 17, 2016

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I had to save this block for contextually-typed object literals like this:

interface CI {
  m: number
  method(this: this, n: number);
}
let o: CI = {
  m: 12,
  method(n) {
    return this.m + n;
  }
}

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@sandersn Nathan Shively-Sanders (sandersn) Aug 22, 2016

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Based on our in-person discussion, I checked on contextual typing for parameters. It's 1 of 9 cases in getTypeForVariableLikeDeclaration:

                const type = declaration.symbol.name === "this"
                    ? getContextuallyTypedThisType(func)
                    : getContextuallyTypedParameterType(<ParameterDeclaration>declaration);

The contextual typing for an undeclared this is basically the same thing here in checkThisExpression since there is no parameter to get the type of.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@HerringtonDarkholme Herrington Darkholme (HerringtonDarkholme) mentioned this pull request Aug 22, 2016
Type inference error with method generic 'this' typing #10468
Closed
@ahejlsberg Anders Hejlsberg (ahejlsberg) mentioned this pull request Aug 22, 2016
This-function parameter inference fails in object with other typing features #10461
Closed
@sandersn
Contextually type this in getDeclFromSig, not checkThisExpr
5aafc2c
@sandersn

Nathan Shively-Sanders (sandersn) commented Aug 22, 2016

Copy link
Copy Markdown
Member Author

I tried moving the contextual typing out of checkThisExpression to getSignatureFromDeclaration and it passes our tests. Anders Hejlsberg (@ahejlsberg) how does it look to you?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ahejlsberg
ahejlsberg reviewed Aug 22, 2016
View reviewed changes
Comment thread src/compiler/checker.ts Outdated
}

function getContextuallyTypedThisType(func: FunctionLikeDeclaration): Type {
function getContextuallyTypedThisParameter(func: FunctionLikeDeclaration): Symbol {

@ahejlsberg Anders Hejlsberg (ahejlsberg) Aug 22, 2016

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I think a better name would be getContextualThisParameter.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@sandersn Nathan Shively-Sanders (sandersn) Aug 22, 2016

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

done

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@ahejlsberg

Anders Hejlsberg (ahejlsberg) commented Aug 22, 2016

Copy link
Copy Markdown
Member

👍 Once you make the suggested name change.
HerringtonDarkholme reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@sandersn
Rename getContextuallyTypedThisParameter to getContextualThisParameter
fc1d6a8
@sandersn
Nathan Shively-Sanders (sandersn) merged commit f028fa3 into master Aug 22, 2016
@sandersn
Nathan Shively-Sanders (sandersn) deleted the instantiate-this-for-contextually-typed-type-parameters branch August 22, 2016 23:47
@sandersn Nathan Shively-Sanders (sandersn) mentioned this pull request Sep 19, 2016
This typing for callbacks typed as any #10972
Closed
Nathan Shively-Sanders (sandersn) added a commit that referenced this pull request Oct 13, 2016
@sandersn
Give correct info for contextually typed this
1aa2562 
This essentially undoes #9746, which only contextually typed object
literal methods, but did so early, during `getSignatureFromDeclaration`.
The compiler now contextually types everything, as it did before, but
only looks for a contextual type if there is no declared `this` type.

After #9746, a member list request from the language service would call
`getSignatureFromDeclaration`, but it would never get a contextual this-type,
because contextual typing was only enabled for object literal methods.
Nathan Shively-Sanders (sandersn) added a commit that referenced this pull request Oct 13, 2016
@sandersn
Give correct info for contextually typed this
853607f 
This essentially undoes #9746, which only contextually typed object
literal methods, but did so early, during `getSignatureFromDeclaration`.
The compiler now contextually types everything, as it did before, but
only looks for a contextual type if there is no declared `this` type.

After #9746, a member list request from the language service would call
`getSignatureFromDeclaration`, but it would never get a contextual this-type,
because contextual typing was only enabled for object literal methods.
@sandersn Nathan Shively-Sanders (sandersn) mentioned this pull request Oct 13, 2016
Give correct info for contextually typed this #11600
Closed
@microsoft Microsoft (microsoft) locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Type inference error with generics and function 'this' typing

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@sandersn @ahejlsberg @msftclas
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.