Check resolution of tslib per file #58654
Conversation
typescript-bot
added
Author: Team
For Uncommitted Bug
|
@typescript-bot test it |
src/compiler/checker.ts
Outdated
| const uncheckedHelpers = helpers & ~requestedExternalEmitHelpers; | ||
| const uncheckedHelpers = helpers & ~links.requestedExternalEmitHelpers; | ||
| for (let helper = ExternalEmitHelpers.FirstEmitHelper; helper <= ExternalEmitHelpers.LastEmitHelper; helper <<= 1) { |
There was a problem hiding this comment.
If I’m understanding this correctly, this checking is now happening once for every source file that needs to access helpers, but the checking itself only depends on the helpersModule. Maybe requestedExternalEmitHelpers could be stored on getSymbolLinks(helpersModule) instead of getNodeLinks(sourceFile) and only happen once per instance of helpers.
However, the error reporting does reference the location being passed in, and maybe that matters for your region-based diagnostics work? Although, we’re still only doing this once per requested helper function per source file, so the location of the errors could be check order dependent both before and after this PR, as it currently is 🤔
There was a problem hiding this comment.
If I’m understanding this correctly, this checking is now happening once for every source file that needs to access helpers, but the checking itself only depends on the
helpersModule. MayberequestedExternalEmitHelperscould be stored ongetSymbolLinks(helpersModule)instead ofgetNodeLinks(sourceFile)and only happen once per instance of helpers.
Good point. So then requestedExternalEmitHelperNames would be irrelevant.
However, the error reporting does reference the location being passed in, and maybe that matters for your region-based diagnostics work? Although, we’re still only doing this once per requested helper function per source file, so the location of the errors could be check order dependent both before and after this PR, as it currently is 🤔
Yeah, I'm planning on dealing with the error location varying in a future PR. But with this PR, we already address the problem of this error possibly jumping to a different, unopened file and never showing up for the user in the editor.
There was a problem hiding this comment.
So then
requestedExternalEmitHelperNameswould be irrelevant.
Hmm, would it? A given instance of a helpers module might have checked __extends in one call, and then the same instance needs to check __assign in a later call, and ought to be able to skip checking __extends again.
There was a problem hiding this comment.
Ah, I mean requestedExternalEmitHelperNames and requestedExternalEmitHelpers should represent the same set, if requestedExternalEmitHelpers is now stored at the symbol level, so we only need one or the other.
There was a problem hiding this comment.
Ok, so I think I confused myself again.
Say we have two different typescript files that need the emit helper called __awaiter. Say those two files share the same tslib. Should we have an error on both typescript files saying their tslib is missing the emit helper called __awaiter? Or only on one of the files?
There was a problem hiding this comment.
I think it would be annoying on the CLI, or in any list of project-wide diagnostics, to get tslib-related errors duplicated across many/all files. I’m not sure how we might want to handle that in an editor scenario, though.
There was a problem hiding this comment.
I think it would be annoying on the CLI, or in any list of project-wide diagnostics, to get tslib-related errors duplicated across many/all files.
That, at least, matches what we currently do (i.e., only error on the first occurrence).
There was a problem hiding this comment.
I added test tslibMultipleMissingHelper.ts that shows the specific helper checking behavior.
|
Hey @gabritto, the results of running the DT tests are ready. Everything looks the same! |
|
@gabritto Here are the results of running the user tests comparing Everything looks good! |
|
@gabritto Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@gabritto Here are the results of running the top 400 repos comparing Everything looks good! |
|
@typescript-bot test it |
|
Hey @gabritto, the results of running the DT tests are ready. Everything looks the same! |
|
@gabritto Here are the results of running the user tests comparing Everything looks good! |
|
@gabritto Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@gabritto Here are the results of running the top 400 repos comparing Everything looks good! |
This issue was raised by @andrewbranch during design meeting.
When the compiler option
importHelpersis on, when we emit a module file, we will insert an appropriate import fromtslibwhen an emit helper is needed.To make sure this emitted import will be valid, during type checking, we try to resolve
tslibwhenever we detect we will need an emit helper, and error if we can't resolve it.However, that resolution and respective error is currently not done per every file that will need the emit helper during emit, it is only done once per type checker and the resolution result is cached. This means that we could be missing errors in situations where for the first file we check
tslibresolution, it resolves, but for a future file it would not resolve.The newly added test case
tslibNotFoundDifferentModulesillustrates this: we have two packages in a project, one of them (package1) that hastslibin itsnode_modules/, and the other (package2) that doesn't. We first check the file inproject1, and there we are able to resolvetslib, and so when we go and check the file inproject2, we don't error sayingtslibis missing, but we should.This PR fixes this problem by making this
tslibresolution check for every file an emit helper will be used.