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

Don't elide imports when transforming JS files #50404

Merged
merged 11 commits into from Sep 19, 2022
42 changes: 36 additions & 6 deletions src/compiler/checker.ts
Expand Up @@ -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(
Expand All @@ -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(
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
}
Expand Down Expand Up @@ -2259,7 +2264,7 @@ namespace ts {
}
return undefined;
}
else if (checkAndReportErrorForInvalidInitializer()) {
else if (nameNotFoundMessage && checkAndReportErrorForInvalidInitializer()) {
return undefined;
}

Expand Down Expand Up @@ -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)) {
Expand Down Expand Up @@ -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;
Copy link
Member Author

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 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.

Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

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.

if (resolvedSymbol && resolvedSymbol !== unknownSymbol) {
return resolvedSymbol;
}

return resolveName(
reference,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

The 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?

Copy link
Member

Choose a reason for hiding this comment

The 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).

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.

Copy link
Member

Choose a reason for hiding this comment

The 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"?

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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);
Expand Down
8 changes: 5 additions & 3 deletions src/compiler/transformers/ts.ts
Expand Up @@ -2455,9 +2455,11 @@ namespace ts {
}

function shouldEmitAliasDeclaration(node: Node): boolean {
return compilerOptions.preserveValueImports
? resolver.isValueAliasDeclaration(node)
: resolver.isReferencedAliasDeclaration(node);
return isInJSFile(node)
amcasey marked this conversation as resolved.
Show resolved Hide resolved
? true
: compilerOptions.preserveValueImports
? resolver.isValueAliasDeclaration(node)
: resolver.isReferencedAliasDeclaration(node);
}
}
}
Expand Up @@ -45,7 +45,7 @@ b_1["default"];
"use strict";
exports.__esModule = true;
var x = { x: "" };
zzz;
a_1["default"];
Copy link
Member Author

Choose a reason for hiding this comment

The 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 import a_1 = require("./a") here, which is elided because of another bug: #50455

var b_1 = require("./b");
b_1["default"];
var y = x;
31 changes: 31 additions & 0 deletions tests/baselines/reference/elidedJSImport1.errors.txt
@@ -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.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that if checkJs is true, the user will still get the errors because we believe TruffleContract is a type and not a value.

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;
}
25 changes: 25 additions & 0 deletions tests/baselines/reference/elidedJSImport1.js
@@ -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
Copy link
Member Author

Choose a reason for hiding this comment

The 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'
39 changes: 39 additions & 0 deletions tests/baselines/reference/elidedJSImport1.symbols
@@ -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))
}
40 changes: 40 additions & 0 deletions tests/baselines/reference/elidedJSImport1.types
@@ -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
}
83 changes: 83 additions & 0 deletions tests/baselines/reference/elidedJSImport2(module=commonjs).js
@@ -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;
50 changes: 50 additions & 0 deletions tests/baselines/reference/elidedJSImport2(module=commonjs).symbols
@@ -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))
}