-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Don't elide imports when transforming JS files #50404
Changes from all commits
1238db9
68021c1
2aa5bfe
0c6f9c7
ef5b062
589eb24
b5c0ec6
07ff4b7
feb0ab7
3b2c300
c2c4fd8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1827,6 +1827,7 @@ namespace ts { | |
* the nameNotFoundMessage argument is not undefined. Returns the resolved symbol, or undefined if no symbol with | ||
* the given name can be found. | ||
* | ||
* @param nameNotFoundMessage If defined, we will report errors found during resolve. | ||
* @param isUse If true, this will count towards --noUnusedLocals / --noUnusedParameters. | ||
*/ | ||
function resolveName( | ||
|
@@ -1837,8 +1838,8 @@ namespace ts { | |
nameArg: __String | Identifier | undefined, | ||
isUse: boolean, | ||
excludeGlobals = false, | ||
getSpellingSuggstions = true): Symbol | undefined { | ||
return resolveNameHelper(location, name, meaning, nameNotFoundMessage, nameArg, isUse, excludeGlobals, getSpellingSuggstions, getSymbol); | ||
getSpellingSuggestions = true): Symbol | undefined { | ||
return resolveNameHelper(location, name, meaning, nameNotFoundMessage, nameArg, isUse, excludeGlobals, getSpellingSuggestions, getSymbol); | ||
} | ||
|
||
function resolveNameHelper( | ||
|
@@ -2011,7 +2012,9 @@ namespace ts { | |
// TypeScript 1.0 spec (April 2014): 3.4.1 | ||
// The scope of a type parameter extends over the entire declaration with which the type | ||
// parameter list is associated, with the exception of static member declarations in classes. | ||
error(errorLocation, Diagnostics.Static_members_cannot_reference_class_type_parameters); | ||
if (nameNotFoundMessage) { | ||
error(errorLocation, Diagnostics.Static_members_cannot_reference_class_type_parameters); | ||
} | ||
return undefined; | ||
} | ||
break loop; | ||
|
@@ -2049,7 +2052,9 @@ namespace ts { | |
if (isClassLike(grandparent) || grandparent.kind === SyntaxKind.InterfaceDeclaration) { | ||
// A reference to this grandparent's type parameters would be an error | ||
if (result = lookup(getSymbolOfNode(grandparent as ClassLikeDeclaration | InterfaceDeclaration).members!, name, meaning & SymbolFlags.Type)) { | ||
error(errorLocation, Diagnostics.A_computed_property_name_cannot_reference_a_type_parameter_from_its_containing_type); | ||
if (nameNotFoundMessage) { | ||
error(errorLocation, Diagnostics.A_computed_property_name_cannot_reference_a_type_parameter_from_its_containing_type); | ||
} | ||
return undefined; | ||
} | ||
} | ||
|
@@ -2259,7 +2264,7 @@ namespace ts { | |
} | ||
return undefined; | ||
} | ||
else if (checkAndReportErrorForInvalidInitializer()) { | ||
else if (nameNotFoundMessage && checkAndReportErrorForInvalidInitializer()) { | ||
return undefined; | ||
} | ||
|
||
|
@@ -43070,7 +43075,8 @@ namespace ts { | |
} | ||
const node = getParseTreeNode(nodeIn, isIdentifier); | ||
if (node) { | ||
const symbol = getReferencedValueSymbol(node); | ||
const symbol = getReferencedValueOrAliasSymbol(node); | ||
|
||
// We should only get the declaration of an alias if there isn't a local value | ||
// declaration for the symbol | ||
if (isNonLocalAlias(symbol, /*excludes*/ SymbolFlags.Value) && !getTypeOnlyAliasDeclaration(symbol)) { | ||
|
@@ -43474,6 +43480,30 @@ namespace ts { | |
return resolveName(location, reference.escapedText, SymbolFlags.Value | SymbolFlags.ExportValue | SymbolFlags.Alias, /*nodeNotFoundMessage*/ undefined, /*nameArg*/ undefined, /*isUse*/ true); | ||
} | ||
|
||
/** | ||
* Get either a value-meaning symbol or an alias symbol. | ||
* Unlike `getReferencedValueSymbol`, if the cached resolved symbol is the unknown symbol, | ||
* we call `resolveName` to find a symbol. | ||
* 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 getReferencedValueOrAliasSymbol(reference: Identifier): Symbol | undefined { | ||
const resolvedSymbol = getNodeLinks(reference).resolvedSymbol; | ||
if (resolvedSymbol && resolvedSymbol !== unknownSymbol) { | ||
return resolvedSymbol; | ||
} | ||
|
||
return resolveName( | ||
reference, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since we're calling There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this new call happen in any scenario that wasn't broken before your change? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
From your tests, it looks like you could just
Yes, that's what I meant. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @gabritto What does your thumbs-up above mean? "No, it can't happen"? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Turns out I was wrong, and we hit the cache for unresolved imports. We don't hit the cache for unresolved identifiers. |
||
reference.escapedText, | ||
SymbolFlags.Value | SymbolFlags.ExportValue | SymbolFlags.Alias, | ||
/*nodeNotFoundMessage*/ undefined, | ||
/*nameArg*/ undefined, | ||
/*isUse*/ true, | ||
/*excludeGlobals*/ undefined, | ||
/*getSpellingSuggestions*/ undefined); | ||
} | ||
|
||
function getReferencedValueDeclaration(referenceIn: Identifier): Declaration | undefined { | ||
if (!isGeneratedIdentifier(referenceIn)) { | ||
const reference = getParseTreeNode(referenceIn, isIdentifier); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change looks weird, but it is correct. However, there should be an |
||
var b_1 = require("./b"); | ||
b_1["default"]; | ||
var y = x; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
tests/cases/compiler/caller.js(1,21): error TS2307: Cannot find module 'fs' or its corresponding type declarations. | ||
tests/cases/compiler/caller.js(2,8): error TS18042: 'TruffleContract' is a type and cannot be imported in JavaScript files. Use 'import("@truffle/contract").TruffleContract' in a JSDoc type annotation. | ||
tests/cases/compiler/caller.js(4,43): error TS2708: Cannot use namespace 'TruffleContract' as a value. | ||
tests/cases/compiler/caller.js(4,60): error TS2708: Cannot use namespace 'TruffleContract' as a value. | ||
|
||
|
||
==== tests/cases/compiler/caller.js (4 errors) ==== | ||
import * as fs from 'fs'; | ||
~~~~ | ||
!!! 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that if |
||
console.log(fs); | ||
console.log('TruffleContract is ', typeof TruffleContract, TruffleContract); // `TruffleContract` is considered 'unused' | ||
~~~~~~~~~~~~~~~ | ||
!!! error TS2708: Cannot use namespace 'TruffleContract' as a value. | ||
~~~~~~~~~~~~~~~ | ||
!!! error TS2708: Cannot use namespace 'TruffleContract' as a value. | ||
|
||
|
||
==== tests/cases/compiler/node_modules/@truffle/contract/index.d.ts (0 errors) ==== | ||
declare module "@truffle/contract" { | ||
interface ContractObject { | ||
foo: number; | ||
} | ||
namespace TruffleContract { | ||
export type Contract = ContractObject; | ||
} | ||
export default TruffleContract; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
//// [tests/cases/compiler/elidedJSImport1.ts] //// | ||
|
||
//// [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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This import used to be elided, but now it isn't. |
||
console.log(fs); | ||
console.log('TruffleContract is ', typeof TruffleContract, TruffleContract); // `TruffleContract` is considered 'unused' | ||
|
||
|
||
//// [index.d.ts] | ||
declare module "@truffle/contract" { | ||
interface ContractObject { | ||
foo: number; | ||
} | ||
namespace TruffleContract { | ||
export type Contract = ContractObject; | ||
} | ||
export default TruffleContract; | ||
} | ||
|
||
//// [caller.js] | ||
import * as fs from 'fs'; | ||
import TruffleContract from '@truffle/contract'; // Runtime err: this import is elided in transform | ||
console.log(fs); | ||
console.log('TruffleContract is ', typeof TruffleContract, TruffleContract); // `TruffleContract` is considered 'unused' |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
=== tests/cases/compiler/caller.js === | ||
import * as fs from 'fs'; | ||
>fs : Symbol(fs, Decl(caller.js, 0, 6)) | ||
|
||
import TruffleContract from '@truffle/contract'; // Runtime err: this import is elided in transform | ||
>TruffleContract : Symbol(TruffleContract, Decl(caller.js, 1, 6)) | ||
|
||
console.log(fs); | ||
>console.log : Symbol(Console.log, Decl(lib.dom.d.ts, --, --)) | ||
>console : Symbol(console, Decl(lib.dom.d.ts, --, --)) | ||
>log : Symbol(Console.log, Decl(lib.dom.d.ts, --, --)) | ||
>fs : Symbol(fs, Decl(caller.js, 0, 6)) | ||
|
||
console.log('TruffleContract is ', typeof TruffleContract, TruffleContract); // `TruffleContract` is considered 'unused' | ||
>console.log : Symbol(Console.log, Decl(lib.dom.d.ts, --, --)) | ||
>console : Symbol(console, Decl(lib.dom.d.ts, --, --)) | ||
>log : Symbol(Console.log, Decl(lib.dom.d.ts, --, --)) | ||
|
||
|
||
=== tests/cases/compiler/node_modules/@truffle/contract/index.d.ts === | ||
declare module "@truffle/contract" { | ||
>"@truffle/contract" : Symbol("@truffle/contract", Decl(index.d.ts, 0, 0)) | ||
|
||
interface ContractObject { | ||
>ContractObject : Symbol(ContractObject, Decl(index.d.ts, 0, 36)) | ||
|
||
foo: number; | ||
>foo : Symbol(ContractObject.foo, Decl(index.d.ts, 1, 30)) | ||
} | ||
namespace TruffleContract { | ||
>TruffleContract : Symbol(TruffleContract, Decl(index.d.ts, 3, 5)) | ||
|
||
export type Contract = ContractObject; | ||
>Contract : Symbol(Contract, Decl(index.d.ts, 4, 31)) | ||
>ContractObject : Symbol(ContractObject, Decl(index.d.ts, 0, 36)) | ||
} | ||
export default TruffleContract; | ||
>TruffleContract : Symbol(TruffleContract, Decl(index.d.ts, 3, 5)) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
=== tests/cases/compiler/caller.js === | ||
import * as fs from 'fs'; | ||
>fs : any | ||
|
||
import TruffleContract from '@truffle/contract'; // Runtime err: this import is elided in transform | ||
>TruffleContract : any | ||
|
||
console.log(fs); | ||
>console.log(fs) : void | ||
>console.log : (...data: any[]) => void | ||
>console : Console | ||
>log : (...data: any[]) => void | ||
>fs : any | ||
|
||
console.log('TruffleContract is ', typeof TruffleContract, TruffleContract); // `TruffleContract` is considered 'unused' | ||
>console.log('TruffleContract is ', typeof TruffleContract, TruffleContract) : void | ||
>console.log : (...data: any[]) => void | ||
>console : Console | ||
>log : (...data: any[]) => void | ||
>'TruffleContract is ' : "TruffleContract is " | ||
>typeof TruffleContract : "string" | "number" | "bigint" | "boolean" | "symbol" | "undefined" | "object" | "function" | ||
>TruffleContract : any | ||
>TruffleContract : any | ||
|
||
|
||
=== tests/cases/compiler/node_modules/@truffle/contract/index.d.ts === | ||
declare module "@truffle/contract" { | ||
>"@truffle/contract" : typeof import("@truffle/contract") | ||
|
||
interface ContractObject { | ||
foo: number; | ||
>foo : number | ||
} | ||
namespace TruffleContract { | ||
export type Contract = ContractObject; | ||
>Contract : ContractObject | ||
} | ||
export default TruffleContract; | ||
>TruffleContract : any | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,83 @@ | ||
//// [tests/cases/compiler/elidedJSImport2.ts] //// | ||
|
||
//// [index.js] | ||
import { Foo } from "./other.js"; | ||
import * as other from "./other.js"; | ||
import defaultFoo from "./other.js"; | ||
|
||
const x = new Foo(); | ||
const y = other.Foo(); | ||
const z = new defaultFoo(); | ||
|
||
//// [other.d.ts] | ||
export interface Foo { | ||
bar: number; | ||
} | ||
|
||
export default interface Bar { | ||
foo: number; | ||
} | ||
|
||
//// [other.js] | ||
export class Foo { | ||
bar = 2.4; | ||
} | ||
|
||
export default class Bar { | ||
foo = 1.2; | ||
} | ||
|
||
|
||
//// [index.js] | ||
"use strict"; | ||
var __createBinding = (this && this.__createBinding) || (Object.create ? (function(o, m, k, k2) { | ||
if (k2 === undefined) k2 = k; | ||
var desc = Object.getOwnPropertyDescriptor(m, k); | ||
if (!desc || ("get" in desc ? !m.__esModule : desc.writable || desc.configurable)) { | ||
desc = { enumerable: true, get: function() { return m[k]; } }; | ||
} | ||
Object.defineProperty(o, k2, desc); | ||
}) : (function(o, m, k, k2) { | ||
if (k2 === undefined) k2 = k; | ||
o[k2] = m[k]; | ||
})); | ||
var __setModuleDefault = (this && this.__setModuleDefault) || (Object.create ? (function(o, v) { | ||
Object.defineProperty(o, "default", { enumerable: true, value: v }); | ||
}) : function(o, v) { | ||
o["default"] = v; | ||
}); | ||
var __importStar = (this && this.__importStar) || function (mod) { | ||
if (mod && mod.__esModule) return mod; | ||
var result = {}; | ||
if (mod != null) for (var k in mod) if (k !== "default" && Object.prototype.hasOwnProperty.call(mod, k)) __createBinding(result, mod, k); | ||
__setModuleDefault(result, mod); | ||
return result; | ||
}; | ||
var __importDefault = (this && this.__importDefault) || function (mod) { | ||
return (mod && mod.__esModule) ? mod : { "default": mod }; | ||
}; | ||
exports.__esModule = true; | ||
var other_js_1 = require("./other.js"); | ||
var other = __importStar(require("./other.js")); | ||
var other_js_2 = __importDefault(require("./other.js")); | ||
var x = new other_js_1.Foo(); | ||
var y = other.Foo(); | ||
var z = new other_js_2["default"](); | ||
//// [other.js] | ||
"use strict"; | ||
exports.__esModule = true; | ||
exports.Foo = void 0; | ||
var Foo = /** @class */ (function () { | ||
function Foo() { | ||
this.bar = 2.4; | ||
} | ||
return Foo; | ||
}()); | ||
exports.Foo = Foo; | ||
var Bar = /** @class */ (function () { | ||
function Bar() { | ||
this.foo = 1.2; | ||
} | ||
return Bar; | ||
}()); | ||
exports["default"] = Bar; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
=== tests/cases/compiler/index.js === | ||
import { Foo } from "./other.js"; | ||
>Foo : Symbol(Foo, Decl(index.js, 0, 8)) | ||
|
||
import * as other from "./other.js"; | ||
>other : Symbol(other, Decl(index.js, 1, 6)) | ||
|
||
import defaultFoo from "./other.js"; | ||
>defaultFoo : Symbol(defaultFoo, Decl(index.js, 2, 6)) | ||
|
||
const x = new Foo(); | ||
>x : Symbol(x, Decl(index.js, 4, 5)) | ||
|
||
const y = other.Foo(); | ||
>y : Symbol(y, Decl(index.js, 5, 5)) | ||
>other : Symbol(other, Decl(index.js, 1, 6)) | ||
|
||
const z = new defaultFoo(); | ||
>z : Symbol(z, Decl(index.js, 6, 5)) | ||
|
||
=== tests/cases/compiler/other.d.ts === | ||
export interface Foo { | ||
>Foo : Symbol(Foo, Decl(other.d.ts, 0, 0)) | ||
|
||
bar: number; | ||
>bar : Symbol(Foo.bar, Decl(other.d.ts, 0, 22)) | ||
} | ||
|
||
export default interface Bar { | ||
>Bar : Symbol(Bar, Decl(other.d.ts, 2, 1)) | ||
|
||
foo: number; | ||
>foo : Symbol(Bar.foo, Decl(other.d.ts, 4, 30)) | ||
} | ||
|
||
=== tests/cases/compiler/other.js === | ||
export class Foo { | ||
>Foo : Symbol(Foo, Decl(other.js, 0, 0)) | ||
|
||
bar = 2.4; | ||
>bar : Symbol(Foo.bar, Decl(other.js, 0, 18)) | ||
} | ||
|
||
export default class Bar { | ||
>Bar : Symbol(Bar, Decl(other.js, 2, 1)) | ||
|
||
foo = 1.2; | ||
>foo : Symbol(Bar.foo, Decl(other.js, 4, 26)) | ||
} | ||
|
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 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 toresolveName
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not worth the extra complexity of maintaining another cache (conditional on no-emit?), especially before we measure a problem.