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

Fix #7224, #7441 - Replace TypeFlags.Narrowable #7235

Merged
merged 8 commits into from
Jun 2, 2016

Conversation

weswigham
Copy link
Member

@weswigham weswigham commented Feb 25, 2016

Fixes #7224, #7441

Previously we would only narrow object types, union types, type parameters, and any.

I looked through the history of the area - this check had been here since the inception of type guards (not even user-defined ones), but I'm going to guess that as they evolved nobody ever really noticed that it would need to be removed with the advent of intersection types - given them, we can potentially need to narrow everything (since anything can be intersected with and therefore can have a subtype to narrow to).

I've replaced TypeFlags.Narrowable with TypeFlags.NotNarrowable - to make narrowing of a type class opt-out, rather than opt-in, as literally any type can be narrowed to an intersection with that type unless we've expressly choosen to forbid it. I've added Void to this list, as it's in-line with how we already treat void.

In essence, TypeFlags.NotNarrowable holds the set of types for which this function:

declare function isTagged<T>(x: T): x is (T & {__tagged: any});

does not narrow as expected, which, as far as I can tell, should only be void.

@tinganho
Copy link
Contributor

@weswigham I believe that check was there too limit the narrowing logic for perf reasons. I guess when your removed it, it will walk up the branches and narrow it on all types?

@DanielRosenwasser
Copy link
Member

This will end up fixing #7441, so if we take this in, we'll need two small tests (in separate files) - one for narrowing a string to a string literal, and the other from a string to a union of string literals.

@DanielRosenwasser
Copy link
Member

As @tinganho mentioned, we'll need to check what the impact is here in terms of perf, but I imagine long term we'll need to do this anyway.

@weswigham weswigham changed the title Remove check narrowing only certain types, add test showing issues wi… Fix #7224, #7441 - Remove TypeFlags.Narrowable Apr 26, 2016
@weswigham weswigham changed the title Fix #7224, #7441 - Remove TypeFlags.Narrowable Fix #7224, #7441 - Replace TypeFlags.Narrowable Apr 26, 2016
@weswigham
Copy link
Member Author

@DanielRosenwasser I've reconciled the change with master and added the test you've asked for (and updated the OP to reflect the changes as they now stand).

@sandersn
Copy link
Member

sandersn commented Jun 2, 2016

👍

@sandersn sandersn merged commit ef0f6c8 into microsoft:master Jun 2, 2016
@sandersn
Copy link
Member

sandersn commented Jun 2, 2016

Looks like @weswigham doesn't have his commit rights back yet. So I went ahead and merged it.

@sandersn sandersn deleted the narrow-all-types branch June 2, 2016 17:01
@zpdDG4gta8XKpMCd
Copy link

Thanks!

@ahejlsberg
Copy link
Member

👎 Whoa! I'd like to review this before we merge it.

@zpdDG4gta8XKpMCd
Copy link

oh no

@ahejlsberg
Copy link
Member

As was already mentioned in the discussion here this potentially has negative performance impact. Specifically, this causes us to do a full walk of the control flow graph even for primtive type variables, which are very common and obviously can't be narrowed. We used to not do this and I definitely don't want this code merged until we know what the impact is.

@sandersn Please revert the merge and let's dicuss how to proceed.

@mhegazy FYI.

@sandersn
Copy link
Member

sandersn commented Jun 2, 2016

I'll revert it. But I did test the performance on RWC and observed no significant difference for TFS and a small but statistically significant speedup for Monaco -- about 1.5%.

sandersn added a commit that referenced this pull request Jun 2, 2016
This reverts commit ef0f6c8, reversing
changes made to 9f087cb.
@sandersn
Copy link
Member

sandersn commented Jun 2, 2016

Here's the summary:

Project Baseline Current Delta Best Worst
Monaco - node - Parse Time 7.65s (± 1.69%) 7.53s (± 1.55%) -1.69% 7.21s 8.10s
Monaco - node - Bind Time 1.45s (± 3.49%) 1.42s (± 2.68%) -1.52% 1.31s 1.65s
Monaco - node - Check Time 9.48s (± 1.39%) 9.34s (± 1.26%) -1.53% 8.81s 9.74s
Monaco - node - Emit Time 1.94s (± 2.52%) 1.88s (± 1.64%) -3.17% 1.80s 2.00s
Monaco - node - Total Time 20.52s (± 1.19%) 20.16s (± 1.00%) -1.75% 19.39s 21.11s
TFS - node - Parse Time 3.57s (± 3.42%) 3.56s (± 3.64%) -0.22% 3.19s 4.00s
TFS - node - Bind Time 1.29s (± 2.91%) 1.26s (± 3.42%) -2.74% 1.13s 1.49s
TFS - node - Check Time 7.25s (± 0.81%) 7.28s (± 0.72%) +0.41% 7.14s 7.48s
TFS - node - Emit Time 1.96s (± 0.75%) 1.99s (± 1.31%) +1.48% 1.91s 2.16s
TFS - node - Total Time 14.07s (± 1.27%) 14.08s (± 1.14%) +0.12% 13.48s 14.95s

I calculated statistical significance based on 20 iterations each of Monaco and TFS. Monaco was signficantly different (p < 0.01) and TFS was not.

sandersn added a commit that referenced this pull request Jun 2, 2016
Revert "Merge pull request #7235 from weswigham/narrow-all-types"
@ahejlsberg
Copy link
Member

Hmm, I'm seeing a 2-3% slowdown in check time with Monaco on node.js v6.2 (Windows, 32-bit) which is more in line with what I expected (though not as much as I had feared). There's no doubt we're doing more work so I'm puzzled about your measured speed up.

I do agree that #7441 ought to be fixed.

@sandersn
Copy link
Member

sandersn commented Jun 2, 2016

I measured on node.js 6.2 (Linux, x64). I'm using @rbuckton's new performance measurement code which I believe is quite close to the existing perf measurement (but with better benchmark-saving capability and the ability to run remotely on Linux).

@weswigham
Copy link
Member Author

Is it possible to reopen this PR so it can be un-reverted at some point?

yuit added a commit that referenced this pull request Jun 3, 2016
* Remove check narrowing only certain types, add test showing issues with this

* string literal case test

* Reconcile fix with CFA work

* Defaultable -> NotNarrowable to align with use

* Missed a defaultable in comments

* Add test for narrowing to unions of string literals

* Actually merge from master

* Run fixupParentReferences when parsing isolated jsDocComment

* initial revision of unit test support for project system in tsserver

* Add non-widening forms of null and undefined

* Create separate control flows for property declarations with initializers

* Add regression test

* Add tests

* Remove unused variable

* Add null check and CR feedback

* Revert "Merge pull request #7235 from weswigham/narrow-all-types"

This reverts commit ef0f6c8, reversing
changes made to 9f087cb.

* reuse the fixupParentReferences function

* Fix up error from merging with master
weswigham added a commit to weswigham/TypeScript that referenced this pull request Jun 6, 2016
@sandersn
Copy link
Member

sandersn commented Jun 6, 2016

This code (slightly modified) is now merged in #8993.

yuit added a commit that referenced this pull request Jun 14, 2016
* Remove check narrowing only certain types, add test showing issues with this

* string literal case test

* Reconcile fix with CFA work

* Defaultable -> NotNarrowable to align with use

* Missed a defaultable in comments

* Add test for narrowing to unions of string literals

* Rewrite isInStringLiteral to accomodate for unterminated strings

* Refactor signatureHelp to expose helper functions

* Add support for completion in string literals

* Remove unused check

* Use const instead of let

* Fix error

* Formatting changes

* Use shorthand properties

* Add failing test for #8738

* Sort baseline reference identifier by name

* Detects assignment to internal module export clause, fixes #8738

* add SharedArrayBuffer

fix

* Factor out assignment op check

* Add test for composite assignment

* Factor out the behaviour and handles x++ and ++x

* Handles ES3 default as identifier name

* Fix missing else statement

* isNameOfExportedDeclarationInNonES6Module

* Reorder options alphabetically

* Mark diagnostics, and skipDefaultLibCheck as internal

* Allow an import of "foo.js" to be matched by a file "foo.ts"

* Improve loadModuleFromFile code

* Respond to PR comments

* Respond to more PR comments

* Fix test

* Actually merge from master

* Revert to old tryLoad implementation

* Run fixupParentReferences when parsing isolated jsDocComment

* initial revision of unit test support for project system in tsserver

* Allow wildcard ("*") patterns in ambient module declarations

* Add non-widening forms of null and undefined

* Create separate control flows for property declarations with initializers

* Add regression test

* Allow trailing commas in function parameter and argument lists

* Add tests

* Remove unused variable

* Add null check and CR feedback

* Support shorthand ambient module declarations

* Revert "Merge pull request #7235 from weswigham/narrow-all-types"

This reverts commit ef0f6c8, reversing
changes made to 9f087cb.

* reuse the fixupParentReferences function

* Improve typing of && operator with --strictNullChecks

* Add test

* Respond to PR comments

* Respond to PR comments

* Add merging tests

* Use a function `stringify` to simplify calls to `JSON.stringify(xyz, undefined, 2)`

* Update tests

* Fix mistake

* Include indent in navigation bar protocol

Previously navbar01 test had indents when run in the browser but not when run from node. Now they run the same.

* Remove unnecessary restrictions in property access narrowing

* Fix fourslash test

* Add regression test

* Consider property declarations to be control flow containers

* Adding regression test

* Remove restriction on --target es5 and --module es6

* change type definition for Object.create

* Fix signature help

* Add "implicit any" warning for shorthand ambient modules

* Remove trailing whitespace

* Support using string values in enums for CompilerOptions in transpile methods

* Remove trailing whitespace in jakefile

* Make `jake runtests-browser` support test regexes with spaces

For example: `jake runtests-browser t="transpile .js files"` now works.

* Add another test

* factor out isJsxOrTsxExtension

* Move to a conformance test

* Revert "Revert "Merge pull request #7235 from weswigham/narrow-all-types""

This reverts commit fc3e040.

* Use inclusive flag, as originally done, but include almost everything

* Add additional tests

* Respond to PR comments

* Fix typo

* add tests for tsserver project system

* Fix test

* Allow case comparison to undefined and null in strict null checking mode

* Remove incorrectly added tests

* check if moduleResolution when verifying that program can be reused

* more tests for module resolution change and exclude

* Fix linting issues

* Merge JSDoc of assignments from function expressions

* Allow nested assignments in type guards

* Add tests

* Improve order of parameter's merged jsdoc

* Force LF newlines for LKG builds/non debug builds
Fixes 6630

* Create intersection types in type guards for unrelated types

* Split commentsFunction test into expr/decl

And renumber.

* Remove TODO comments

* Accept new baselines

* Add tests

* Remove comments

* Fix test helper

* Recognize relative path using in outDir property (#9025)

* Recognize relative path using in outDir property

* Add projects tests

* Add project .json files

* Update baselines

* Add comments

* Add test case

The test passes in 1.8 and fails in master.

* Return trace when exception happens

* Remove Long-Done TODO

AFAIK, the harness sources have been concatenated into `run.js` for as long as I've known. This stops executing them twice (and in turn makes debugging tests much easier, since you no longer have to debug into eval'd code).

* Allow primitive type guards with typeof on right

Previously, only type guards of the form `typeof x === 'string'` were
allowed. Now you can write `'string' === typeof x`.

* Primitive type guards are now order independent

* Fix comments in tests

* Add handleing for classes

* Add more tests for target=es5 module=es6

* addExportToArgumentListKind

* Accept baseline

* Add more tests

* wip-fixing transforms

* Adds progress indicators to the runtests-parallel build task.

* Fixed typo

* Fix comment

* Add test for out-of-range error

* Use proper method of not resolving alias

* Fix module loading error

(commandLineOptions_stringToEnum would be undefined if optionDeclarations wasn't loaded yet)

* Port 8739

* Update tests

* Update baselines

* Contextually type return statement in async function

* Remove stale files

* Undo change

* Improve perf

* Improve tests

* Fix sourcemaps for debugging tests

* Allow --sourceRoot with --inlineSources option
Fixes #8445

* this in parameter initializers resolves to class

Accept baselines now that the test passes.

* Add tests for more kinds of import/export

* Fix7334 Disallow async in functionExpression and ArrowFunction (#9062)

* Error when using async modifier in function-expression and arrow-function when target es5

* Add tests and baselines

* Resolve function-this in parameter initialisers when explicitly provided

* Allow null/undefined guard with null/undefined on left

Also add a test with baselines.

* Code review comments

* Update more diagnostic messages ES6->2015

Fix #8996 CC @mhegazy.

* Fixes an issue with runtests-parallel when global mocha is not installed.

* Update LKG

* Add tests

* fix baselines

* Recommend runtests-parallel in CONTRIBUTING

* Only inlineSourceMap when debugging through jake-browser (#9080)

* Only inlineSourceMap when debugging through jake-browser

* Address PR: fix typo in opt's property

* Manually port tests from PR 8470

* minor fix: add missing return clause

* Support using string values in enums for CompilerOptions in transpile methods

* Support using string values in enums for CompilerOptions in transpile methods

# Conflicts:
#	tests/cases/unittests/transpile.ts

* Fix test helper

* Add test for out-of-range error

* Fix module loading error

(commandLineOptions_stringToEnum would be undefined if optionDeclarations wasn't loaded yet)

* Use camel-case instead of snake-case (#9134)

* Manually add tests for PR 8988

* Allow wildcard ("*") patterns in ambient module declarations

* Respond to PR comments

* Add another test

* Improve perf

* Improve tests

* Update baseline from merging with master

* Address PR comment

* Update baseline

* Refactor how we retrieve binding-name cache in module transformer

* Temporary accept so we get a clean run-tests result
yuit added a commit that referenced this pull request Jun 14, 2016
* Remove check narrowing only certain types, add test showing issues with this

* string literal case test

* Reconcile fix with CFA work

* Defaultable -> NotNarrowable to align with use

* Missed a defaultable in comments

* Add test for narrowing to unions of string literals

* Rewrite isInStringLiteral to accomodate for unterminated strings

* Refactor signatureHelp to expose helper functions

* Add support for completion in string literals

* Remove unused check

* Use const instead of let

* Fix error

* Formatting changes

* Use shorthand properties

* Add failing test for #8738

* Sort baseline reference identifier by name

* Detects assignment to internal module export clause, fixes #8738

* add SharedArrayBuffer

fix

* Factor out assignment op check

* Add test for composite assignment

* Factor out the behaviour and handles x++ and ++x

* Handles ES3 default as identifier name

* Fix missing else statement

* isNameOfExportedDeclarationInNonES6Module

* Reorder options alphabetically

* Mark diagnostics, and skipDefaultLibCheck as internal

* Allow an import of "foo.js" to be matched by a file "foo.ts"

* Improve loadModuleFromFile code

* Respond to PR comments

* Respond to more PR comments

* Fix test

* Actually merge from master

* Revert to old tryLoad implementation

* Run fixupParentReferences when parsing isolated jsDocComment

* initial revision of unit test support for project system in tsserver

* Allow wildcard ("*") patterns in ambient module declarations

* Add non-widening forms of null and undefined

* Create separate control flows for property declarations with initializers

* Add regression test

* Allow trailing commas in function parameter and argument lists

* Add tests

* Remove unused variable

* Add null check and CR feedback

* Support shorthand ambient module declarations

* Revert "Merge pull request #7235 from weswigham/narrow-all-types"

This reverts commit ef0f6c8, reversing
changes made to 9f087cb.

* reuse the fixupParentReferences function

* Improve typing of && operator with --strictNullChecks

* Add test

* Respond to PR comments

* Respond to PR comments

* Add merging tests

* Use a function `stringify` to simplify calls to `JSON.stringify(xyz, undefined, 2)`

* Update tests

* Fix mistake

* Include indent in navigation bar protocol

Previously navbar01 test had indents when run in the browser but not when run from node. Now they run the same.

* Remove unnecessary restrictions in property access narrowing

* Fix fourslash test

* Add regression test

* Consider property declarations to be control flow containers

* Adding regression test

* Remove restriction on --target es5 and --module es6

* change type definition for Object.create

* Fix signature help

* Add "implicit any" warning for shorthand ambient modules

* Remove trailing whitespace

* Support using string values in enums for CompilerOptions in transpile methods

* Remove trailing whitespace in jakefile

* Make `jake runtests-browser` support test regexes with spaces

For example: `jake runtests-browser t="transpile .js files"` now works.

* Add another test

* factor out isJsxOrTsxExtension

* Move to a conformance test

* Revert "Revert "Merge pull request #7235 from weswigham/narrow-all-types""

This reverts commit fc3e040.

* Use inclusive flag, as originally done, but include almost everything

* Add additional tests

* Respond to PR comments

* Fix typo

* add tests for tsserver project system

* Fix test

* Allow case comparison to undefined and null in strict null checking mode

* Remove incorrectly added tests

* check if moduleResolution when verifying that program can be reused

* more tests for module resolution change and exclude

* Fix linting issues

* Merge JSDoc of assignments from function expressions

* Allow nested assignments in type guards

* Add tests

* Improve order of parameter's merged jsdoc

* Force LF newlines for LKG builds/non debug builds
Fixes 6630

* Create intersection types in type guards for unrelated types

* Split commentsFunction test into expr/decl

And renumber.

* Remove TODO comments

* Accept new baselines

* Add tests

* Remove comments

* Fix test helper

* Recognize relative path using in outDir property (#9025)

* Recognize relative path using in outDir property

* Add projects tests

* Add project .json files

* Update baselines

* Add comments

* Add test case

The test passes in 1.8 and fails in master.

* Return trace when exception happens

* Remove Long-Done TODO

AFAIK, the harness sources have been concatenated into `run.js` for as long as I've known. This stops executing them twice (and in turn makes debugging tests much easier, since you no longer have to debug into eval'd code).

* Allow primitive type guards with typeof on right

Previously, only type guards of the form `typeof x === 'string'` were
allowed. Now you can write `'string' === typeof x`.

* Primitive type guards are now order independent

* Fix comments in tests

* Add handleing for classes

* Add more tests for target=es5 module=es6

* addExportToArgumentListKind

* Accept baseline

* Add more tests

* Adds progress indicators to the runtests-parallel build task.

* Fixed typo

* Fix comment

* Add test for out-of-range error

* Use proper method of not resolving alias

* Fix module loading error

(commandLineOptions_stringToEnum would be undefined if optionDeclarations wasn't loaded yet)

* Update tests

* Contextually type return statement in async function

* Remove stale files

* Undo change

* Improve perf

* Improve tests

* Fix sourcemaps for debugging tests

* Allow --sourceRoot with --inlineSources option
Fixes #8445

* this in parameter initializers resolves to class

Accept baselines now that the test passes.

* Add tests for more kinds of import/export

* Fix7334 Disallow async in functionExpression and ArrowFunction (#9062)

* Error when using async modifier in function-expression and arrow-function when target es5

* Add tests and baselines

* Resolve function-this in parameter initialisers when explicitly provided

* Allow null/undefined guard with null/undefined on left

Also add a test with baselines.

* Code review comments

* Update more diagnostic messages ES6->2015

Fix #8996 CC @mhegazy.

* Fixes an issue with runtests-parallel when global mocha is not installed.

* Update LKG

* Add tests

* fix baselines

* Salsa: get members of variables whose initialisers are functions

* Test adding members to JS variables whose initialisers are functions

* Recommend runtests-parallel in CONTRIBUTING

* Allow empty lists on command line

* Remove single-comma empty array form

* Remove trailing whitespace

* Implicit type inclusion changes

* Only inlineSourceMap when debugging through jake-browser (#9080)

* Only inlineSourceMap when debugging through jake-browser

* Address PR: fix typo in opt's property

* minor fix: add missing return clause

* Use camel-case instead of snake-case (#9134)

* Baseline fix, CR comments, lint

* CR changes

* Add test for jsdoc in navigation bar

* Fixes runtests-parallel not reporting failure for failed tests.

* Fix decorator metadata emit for rest arg with no type

* Add isDefinition to ReferenceEntry

Clients can now easily tell if the reference is to a definition or a
usage.

* Test isDefinition

* Add option to bail out of `jake runtests` when one test fails

* Absolute-ify paths in both places

* Refactor

* Add unit test

* lint

* Added tests.

* Accepted baselines.

* Emit 'exports.' if the shorthand is a general export.

* Accepted baselines.

* Emit 'Promise' decorator metadata return type for async methods

* Respond to PR comment

* Unescape identifiers used in code completion

* Make isDefinition required.

For the deprecated getOccurrencesAtPosition, isDefinition is always false.

* Add more isDefinition tests and fix computed property bug

* Fix bug: do unescaping in the right place, so that it only affects escaped javascript identifiers

* Use `isLiteralComputedPropertyDeclarationName`
@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.

7 participants