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
62 changes: 52 additions & 10 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1837,8 +1837,9 @@ 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,
reportErrors = true): Symbol | undefined {
return resolveNameHelper(location, name, meaning, nameNotFoundMessage, nameArg, isUse, excludeGlobals, getSpellingSuggestions, getSymbol, reportErrors);
}

function resolveNameHelper(
Expand All @@ -1850,7 +1851,8 @@ namespace ts {
isUse: boolean,
excludeGlobals: boolean,
getSpellingSuggestions: boolean,
lookup: typeof getSymbol): Symbol | undefined {
lookup: typeof getSymbol,
reportErrors = true): Symbol | undefined {
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

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. I'd change it and see if it breaks anything.

const originalLocation = location; // needed for did-you-mean error reporting, which gathers candidates starting from the original location
let result: Symbol | undefined;
let lastLocation: Node | undefined;
Expand Down Expand Up @@ -2011,7 +2013,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 (reportErrors) {
error(errorLocation, Diagnostics.Static_members_cannot_reference_class_type_parameters);
}
return undefined;
}
break loop;
Expand All @@ -2029,7 +2033,7 @@ namespace ts {
if (lastLocation === (location as ExpressionWithTypeArguments).expression && (location.parent as HeritageClause).token === SyntaxKind.ExtendsKeyword) {
const container = location.parent.parent;
if (isClassLike(container) && (result = lookup(getSymbolOfNode(container).members!, name, meaning & SymbolFlags.Type))) {
if (nameNotFoundMessage) {
if (reportErrors && nameNotFoundMessage) {
error(errorLocation, Diagnostics.Base_class_expressions_cannot_reference_class_type_parameters);
}
return undefined;
Expand All @@ -2049,7 +2053,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 (reportErrors) {
error(errorLocation, Diagnostics.A_computed_property_name_cannot_reference_a_type_parameter_from_its_containing_type);
}
return undefined;
}
}
Expand Down Expand Up @@ -2206,7 +2212,7 @@ namespace ts {
}

if (!result) {
if (nameNotFoundMessage) {
if (reportErrors && nameNotFoundMessage) {
addLazyDiagnostic(() => {
if (!errorLocation ||
!checkAndReportErrorForMissingPrefix(errorLocation, name, nameArg!) && // TODO: GH#18217
Expand Down Expand Up @@ -2259,12 +2265,12 @@ namespace ts {
}
return undefined;
}
else if (checkAndReportErrorForInvalidInitializer()) {
else if (reportErrors && checkAndReportErrorForInvalidInitializer()) {
return undefined;
}

// Perform extra checks only if error reporting was requested
if (nameNotFoundMessage) {
if (reportErrors && nameNotFoundMessage) {
addLazyDiagnostic(() => {
// Only check for block-scoped variable if we have an error location and are looking for the
// name with variable meaning
Expand Down Expand Up @@ -43070,7 +43076,8 @@ namespace ts {
}
const node = getParseTreeNode(nodeIn, isIdentifier);
if (node) {
const symbol = getReferencedValueSymbol(node);
const symbol = getReferencedSymbol(node, /*startInDeclarationContainer*/ undefined);

// 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 +43481,41 @@ 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 getReferencedSymbol(reference: Identifier, startInDeclarationContainer?: boolean): Symbol | undefined {
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.

const resolvedSymbol = getNodeLinks(reference).resolvedSymbol;
if (resolvedSymbol && resolvedSymbol !== unknownSymbol) {
return resolvedSymbol;
}

let location: Node = 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.

if (startInDeclarationContainer) {
// When resolving the name of a declaration as a value, we need to start resolution
// at a point outside of the declaration.
const parent = reference.parent;
if (isDeclaration(parent) && reference === parent.name) {
location = getDeclarationContainer(parent);
}
}

return resolveName(
location,
reference.escapedText,
SymbolFlags.Value | SymbolFlags.ExportValue | SymbolFlags.Alias,
/*nodeNotFoundMessage*/ undefined,
/*nameArg*/ undefined,
/*isUse*/ true,
/*excludeGlobals*/ undefined,
/*getSpellingSuggestions*/ undefined,
/*reportErrors*/ false);
}

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
Original file line number Diff line number Diff line change
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);
}
}
}
Original file line number Diff line number Diff line change
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;
26 changes: 26 additions & 0 deletions tests/baselines/reference/elidedJSImport.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
//// [tests/cases/compiler/elidedJSImport.ts] ////

//// [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'


//// [index.d.ts]
declare module "@truffle/contract" {
// import { ContractObject } from "@truffle/contract-schema";
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'
40 changes: 40 additions & 0 deletions tests/baselines/reference/elidedJSImport.symbols
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 : 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))

// import { ContractObject } from "@truffle/contract-schema";
interface ContractObject {
>ContractObject : Symbol(ContractObject, Decl(index.d.ts, 0, 36))

foo: number;
>foo : Symbol(ContractObject.foo, Decl(index.d.ts, 2, 30))
}
namespace TruffleContract {
>TruffleContract : Symbol(TruffleContract, Decl(index.d.ts, 4, 5))

export type Contract = ContractObject;
>Contract : Symbol(Contract, Decl(index.d.ts, 5, 31))
>ContractObject : Symbol(ContractObject, Decl(index.d.ts, 0, 36))
}
export default TruffleContract;
>TruffleContract : Symbol(TruffleContract, Decl(index.d.ts, 4, 5))
}
41 changes: 41 additions & 0 deletions tests/baselines/reference/elidedJSImport.types
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
=== tests/cases/compiler/caller.js ===
import * as fs from 'fs';
>fs : error

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 : error

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 : error
>TruffleContract : error


=== tests/cases/compiler/node_modules/@truffle/contract/index.d.ts ===
declare module "@truffle/contract" {
>"@truffle/contract" : typeof import("@truffle/contract")

// import { ContractObject } from "@truffle/contract-schema";
interface ContractObject {
foo: number;
>foo : number
}
namespace TruffleContract {
export type Contract = ContractObject;
>Contract : ContractObject
}
export default TruffleContract;
>TruffleContract : any
}
31 changes: 31 additions & 0 deletions tests/baselines/reference/elidedJSImport1.errors.txt
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.
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
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
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
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))
}
Loading