-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Dont look for properties of Object and Function type when looking to resolve named import from module with export=
#37964
Conversation
@typescript-bot test this |
Heya @sheetalkamat, I've started to run the extended test suite on this PR at 16b4153. You can monitor the build here. |
@typescript-bot user test this |
Heya @sheetalkamat, I've started to run the parallelized community code test suite on this PR at 16b4153. You can monitor the build here. |
@typescript-bot run dt |
Heya @sheetalkamat, I've started to run the parallelized Definitely Typed test suite on this PR at 16b4153. You can monitor the build here. |
…ort=` type to be resolved by named imports Fixes #37165
16b4153
to
5af482c
Compare
@typescript-bot test this |
Heya @sheetalkamat, I've started to run the extended test suite on this PR at 5af482c. You can monitor the build here. |
@typescript-bot run dt |
Heya @sheetalkamat, I've started to run the parallelized community code test suite on this PR at 5af482c. You can monitor the build here. |
Heya @sheetalkamat, I've started to run the parallelized Definitely Typed test suite on this PR at 5af482c. You can monitor the build here. |
Heya @sheetalkamat, I've started to run the extended test suite on this PR at 5af482c. You can monitor the build here. |
src/compiler/checker.ts
Outdated
@@ -2586,7 +2586,7 @@ namespace ts { | |||
let symbolFromVariable: Symbol | undefined; | |||
// First check if module was specified with "export=". If so, get the member from the resolved type | |||
if (moduleSymbol && moduleSymbol.exports && moduleSymbol.exports.get(InternalSymbolName.ExportEquals)) { | |||
symbolFromVariable = getPropertyOfType(getTypeOfSymbol(targetSymbol), name.escapedText); | |||
symbolFromVariable = getPropertyOfType(getTypeOfSymbol(targetSymbol), name.escapedText, /*skipObjectFunctionPropertyAugment*/ true); |
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.
Actually, rather than threading through this flag, and worrying about the safety of caching it and have two differently calculated symbols for a property lookup, couldn't we
- Initialize
globalFunctionType
andglobalObjectType
each to a unique empty type (which should naturally cause property lookups that ultimately may have depended on them to fail during global initialization). - Check if
symbolFromVariable.parent
here isglobalObjectType
orglobalFunctionType
and, if so, discard the result? (to ensure consistency with the initialization phase)
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.
- We want this to happen irrespective of augmenting that means any time we import the properties from object/function should be error so this might not happen at augmentation time.
- Will there be some kind of edge case where in valid scenario return globalfunctionType property symbol. That is what if getTypeOfSymbol(targetSymbol) is globalFunctionType or globalObjectType
@typescript-bot run dt |
Heya @sheetalkamat, I've started to run the parallelized community code test suite on this PR at a252ef9. You can monitor the build here. |
Heya @sheetalkamat, I've started to run the extended test suite on this PR at a252ef9. You can monitor the build here. |
Heya @sheetalkamat, I've started to run the parallelized Definitely Typed test suite on this PR at a252ef9. You can monitor the build here. |
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
@typescript-bot user test this |
Heya @sheetalkamat, I've started to run the parallelized community code test suite on this PR at a252ef9. You can monitor the build here. |
@typescript-bot run dt |
Heya @sheetalkamat, I've started to run the parallelized Definitely Typed test suite on this PR at a252ef9. You can monitor the build here. |
Heya @sheetalkamat, I've started to run the extended test suite on this PR at a252ef9. You can monitor the build here. |
All the tests passed. |
@weswigham looking at your last review, it looks like @sheetalkamat addressed your questions. Is this ready to merge once 4.1 RC is out? |
a9e4980
to
5a964bd
Compare
Will wait for master to become 4.2 before merging this one. |
Fixes #37165