镜像站点 · 本页由第三方 GitHub 只读镜像提供,非 GitHub 官方站点,不接受任何登录或凭据输入。前往 github.com
Skip to content

Remove redundant call to checkNodeDeferred - #22516

Merged
2 commits merged into
masterfrom
checkNodeDeferred
Mar 13, 2018
Merged

2 commits merged into
masterfrom
checkNodeDeferred

Conversation

@ghost

@ghost Deleted user (ghost) commented Mar 13, 2018 •

Copy link
Copy Markdown

Fixes #22491
If I have this right, we will always do one more check after all SkipContextSensitive checks.
Before this PR, the assertion (also added in this PR) would have failed for most arrow functions in call expressions.

For posterity, a simple test case that failed was:

// @noUnusedLocals: true
// @noUnusedParameters: true
declare function f<T>(a: (s: string) => T): void;
f(s => s);

(we have lots of test cases like this already, so didn't see a need to add a new one.)

@weswigham Wesley Wigham (weswigham) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Provided nothing's been obviously broken, it seems fine - I'm pretty sure any context sensitive node that gets checked in SkipContextSensitive will later on be checked in a context-sensitive way and use the following branches which also contain checkNodeDeferred. That these supposedly-one time checks are getting registered during inference (which is inherently multi-pass and shouldn't persist state unless inference succeeds) at all is kinda odd, though. Do we not visit function expressions/object literal methods outside of inference? Given that the grammar checks done in this method which also does inference things, I guess not, but still... odd. It means we recheck the grammar during every every inference pass, which seems wasteful.

@ghost

Copy link
Copy Markdown
Author

It means we recheck the grammar during every every inference pass, which seems wasteful.

I wonder if it wouldn't impact performance to just move all the grammar checking out to its own tree walk -- would certainly make the code easier to read.

@weswigham

Wesley Wigham (weswigham) commented Mar 13, 2018 •

Copy link
Copy Markdown
Member

Andy (Andrewkraft) (@Andy-MS) We effectively already do js-only grammar checks in a separate tree walk in program.ts, so it can't be that bad.

Comment thread src/compiler/checker.ts Outdated

function registerForUnusedIdentifiersCheck(node: Node) {
if (deferredUnusedIdentifierNodes) {
Debug.assert(!contains(deferredUnusedIdentifierNodes, node), "Registering unused identifier twice");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

actually, isn't the contains call slow? I imagine the list gets long.

@weswigham Wesley Wigham (weswigham) Mar 13, 2018 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah contains is a linear search - binarySearch might be a bit better (since the list is probably effectively sorted by node id), but best would just be using a map instead.

@ghost
ghost merged commit 23a64fe into master Mar 13, 2018
@ghost
ghost deleted the checkNodeDeferred branch March 13, 2018 22:46
@microsoft Microsoft (microsoft) locked and limited conversation to collaborators Jul 25, 2018
This pull request was closed.
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.

2 participants