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

Contextually type IIFE params by their arguments #8483

Merged
merged 3 commits into from
May 6, 2016

Conversation

sandersn
Copy link
Member

@sandersn sandersn commented May 5, 2016

Fixes #4142.

Right now rest parameters look at the first matching argument to get their contextual type. This might be wrong.

@@ -8540,6 +8551,13 @@ namespace ts {
return undefined;
}

function isIife(func: FunctionExpression | MethodDeclaration) {
return (func.kind === SyntaxKind.FunctionExpression || func.kind === SyntaxKind.ArrowFunction) &&
func.parent.kind === SyntaxKind.ParenthesizedExpression &&
Copy link
Contributor

Choose a reason for hiding this comment

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

not necessarily. this is a valid iife (function f() {)} ()).

Copy link
Member

Choose a reason for hiding this comment

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

I suggest using the skipParenthesizedNodes function before checking for a call expression at the top.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, although skipParenthesizedNodes actually goes down, not up (expression = expression.expression), so I just wrote an inline loop to go up (parent = parent.parent)

And address bug with contextually typed arguments that the PR changes
exposed.
@sandersn
Copy link
Member Author

sandersn commented May 5, 2016

@ahejlsberg once I fixed the parenthesis walker, I had trouble with contextually typed lambdas, eg (f => f)(i => i). So I short-circuited contextual typing by passing identityMapper as the contextual mapper, which trips the anyFunctionType short-circuit in checkFunctionExpressionOrObjectLiteralMethod. Is there a better way to do this?

The compile passes and fails correctly, but the .types files stringifies anyFunctionType as {}, which is weird. See the last two statements in the test files.

const indexOfParameter = indexOf(func.parameters, parameter);
if (iife.arguments && indexOfParameter < iife.arguments.length) {
if (parameter.dotDotDotToken) {
return createArrayType(getUnionType(map(iife.arguments.slice(indexOfParameter), getTypeOfExpression)));
Copy link
Contributor

Choose a reason for hiding this comment

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

do you really need the array allocation here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Switched to a for loop. As we discussed in person, it would be nice to have a map over a range for general use.

Also allocate once instead of twice.
@mhegazy
Copy link
Contributor

mhegazy commented May 6, 2016

Lgtm

@sandersn sandersn merged commit bc6d6ea into master May 6, 2016
@sandersn sandersn deleted the contextually-type-iife-parameters branch May 6, 2016 03:41
@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.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants