Don't elide imports when transforming JS files #50404
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
typescript-bot
added
Author: Team
For Uncommitted Bug
|
@typescript-bot perf test this |
|
Heya @gabritto, I've started to run the perf test suite on this PR at 2aa5bfe. You can monitor the build here. Update: The results are in! |
|
@gabritto Here they are:
CompilerComparison Report - main..50404
System
Hosts
Scenarios
TSServerComparison Report - main..50404
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
gabritto
commented
Sep 6, 2022
|
|
||
| //// [caller.js] | ||
| import * as fs from 'fs'; | ||
| import TruffleContract from '@truffle/contract'; // Runtime err: this import is elided in transform |
There was a problem hiding this comment.
This import used to be elided, but now it isn't.
| !!! error TS2307: Cannot find module 'fs' or its corresponding type declarations. | ||
| import TruffleContract from '@truffle/contract'; // Runtime err: this import is elided in transform | ||
| ~~~~~~~~~~~~~~~ | ||
| !!! error TS18042: 'TruffleContract' is a type and cannot be imported in JavaScript files. Use 'import("@truffle/contract").TruffleContract' in a JSDoc type annotation. |
There was a problem hiding this comment.
Note that if checkJs is true, the user will still get the errors because we believe TruffleContract is a type and not a value.
| @@ -45,7 +45,7 @@ b_1["default"]; | |||
| "use strict"; | |||
| exports.__esModule = true; | |||
| var x = { x: "" }; | |||
| zzz; | |||
| a_1["default"]; | |||
There was a problem hiding this comment.
This change looks weird, but it is correct. However, there should be an import a_1 = require("./a") here, which is elided because of another bug: #50455
src/compiler/checker.ts
Outdated
| @@ -1850,7 +1851,8 @@ namespace ts { | |||
| isUse: boolean, | |||
| excludeGlobals: boolean, | |||
| getSpellingSuggestions: boolean, | |||
| lookup: typeof getSymbol): Symbol | undefined { | |||
| lookup: typeof getSymbol, | |||
| reportErrors = true): Symbol | undefined { | |||
There was a problem hiding this comment.
resolveNameHelper used to report errors even if nameNotFoundMessage was undefined. Since we now call resolveNameHelper in getReferencedValueOrAliasSymbol when skipping the cached resolved symbol, without this change we would get different number of pre-emit and post-emit reported errors, and they'd be useless/repeated errors anyways.
There was a problem hiding this comment.
resolveEntityName already takes an ignoreErrors flag and is seemingly what we use to resolve type names elsewhere - it just passes an undefined nameNotFoundMessage in those cases. Instead of adding a new parameter that also needs to be passed in at the resolveEntityName and maybe other callsites, maybe checking the existing nameNotFoundMessage parameter in more places works?
There was a problem hiding this comment.
Isn't the existing code in the checker somehow relying on the fact that we report some errors even when nameNotFoundMessage is undefined? I thought this was intentional and the check for an undefined nameNotFoundMessage just kept us from reporting some "not found" errors.
There was a problem hiding this comment.
probably not. I'd change it and see if it breaks anything.
| * This is because when caching the resolved symbol, we only consider value symbols, but here | ||
| * we want to also get an alias symbol if one exists. | ||
| */ | ||
| function getReferencedSymbol(reference: Identifier, startInDeclarationContainer?: boolean): Symbol | undefined { |
There was a problem hiding this comment.
We need to call resolveName here when the resolved symbol is unknown (i.e. there's a cache miss), because the resolved symbol is cached by doing a call to resolveName that only looks for symbols that have a value meaning.
Since we stopped eliding imports in JS files, that means some of the import symbols we want to find won't have a value meaning, but we still want to find them anyways, e.g. imports of types. Since those are imports, they'll have an alias meaning, hence we call resolveName with value or alias meaning.
There was a problem hiding this comment.
Could we know during cache construction whether we'll eventually want alias symbols? Would that interfere with other cache consumers?
There was a problem hiding this comment.
For (1), I think maybe that could be done. We want aliases for every identifier that goes through a module transform, worst case we need it for every identifier that survived the type erasure transform. But for (2) I think we would need a different cache, because changing it to cache alias-meaning symbols would interfere with the way the checker uses this cache.
There was a problem hiding this comment.
Probably not worth the extra complexity of maintaining another cache (conditional on no-emit?), especially before we measure a problem.
Comment on lines
+5
to
+6
| // @esModuleInterop: true | ||
| // @isolatedModules: true |
There was a problem hiding this comment.
I think those compiler options work ok with my current changes, but I am not sure. I also don't think any other compiler option could interact badly with this change, but I have limited knowledge of the many things that could go wrong with emit and imports, so please, let me know if there's something else I should be testing here.
typescript-bot
added
For Milestone Bug
gabritto
commented
Sep 6, 2022
| return resolvedSymbol; | ||
| } | ||
|
|
||
| return resolveName( |
There was a problem hiding this comment.
Since we're calling resolveName if the cache returns unknownSymbol, there could be a perf increase in emit time for JS files. I'm not sure if that is a big concern, or how to measure it, or even how to address it, so I'm looking for opinions here.
There was a problem hiding this comment.
Can this new call happen in any scenario that wasn't broken before your change?
There was a problem hiding this comment.
If I understand the change correctly (dubious), it sounds like the worst case is a file containing only unused imports? If so, could you make such a file with the top 100 npm packages and then measure before and after tsc time?
There was a problem hiding this comment.
Not unused imports, but identifiers that would resolve to unknown symbol in the cache before, which I think means identifiers that either don't have a value meaning (I guess in JS that would be imports we think refer to types), or identifiers that we couldn't resolve (i.e. imports we couldn't resolve).
By file with the top 100 npm packages do you mean a file that imports from the top 100 npm packages?
There was a problem hiding this comment.
Not unused imports, but identifiers that would resolve to unknown symbol in the cache before, which I think means identifiers that either don't have a value meaning (I guess in JS that would be imports we think refer to types), or identifiers that we couldn't resolve (i.e. imports we couldn't resolve).
From your tests, it looks like you could just typeof each imported symbol?
By file with the top 100 npm packages do you mean a file that imports from the top 100 npm packages?
Yes, that's what I meant.
There was a problem hiding this comment.
@gabritto What does your thumbs-up above mean? "No, it can't happen"?
There was a problem hiding this comment.
Ah, yes, I meant it can happen. But now that I'm thinking about it again, I guess both scenarios I pointed out in my comment above are considered "broken" (both imports that refer to types only and imports we couldn't resolve). So I guess the answer is no.
There was a problem hiding this comment.
Turns out I was wrong, and we hit the cache for unresolved imports. We don't hit the cache for unresolved identifiers.
|
@typescript-bot perf test this |
|
Heya @gabritto, I've started to run the perf test suite on this PR at 3b2c300. You can monitor the build here. Update: The results are in! |
gabritto
changed the title
|
@gabritto Here they are:
CompilerComparison Report - main..50404
System
Hosts
Scenarios
TSServerComparison Report - main..50404
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Can you give an example of an export that would be elided? |
It's the same rule as for TS files, since we don't seem to distinguish between JS and TS when transforming. If there's an export for something that resolves to a symbol that doesn't have a value meaning, then we elide it. If you consider the example from the issue/in // @Filename: caller.js
import * as fs from 'fs';
import TruffleContract from '@truffle/contract'; // TruffleContract resolves to a namespace only
console.log(fs);
console.log('TruffleContract is ', typeof TruffleContract, TruffleContract);
export { TruffleContract }; // This export will be elidedso the |
|
Hey @gabritto, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
This was referenced Dec 9, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #48588.
The fix, as discussed in the design meeting, is to never elide imports when transforming JS files.
Note that this breaks some assumptions that were being made by further transforms, such as looking up only value symbols when finding whether an identifier was an import.
Also note: this change is when transforming JS files only. And it only updates our transforms to not eliding imports. Exports in JS files will still be elided as before.