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

Fix type-only imports in interface 'extends' and import=/export= #36496

Merged
merged 15 commits into from
Jan 29, 2020
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
105 changes: 83 additions & 22 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2182,9 +2182,32 @@ namespace ts {

function getTargetOfImportEqualsDeclaration(node: ImportEqualsDeclaration, dontResolveAlias: boolean): Symbol | undefined {
if (node.moduleReference.kind === SyntaxKind.ExternalModuleReference) {
return resolveExternalModuleSymbol(resolveExternalModuleName(node, getExternalModuleImportEqualsDeclarationExpression(node)));
const immediate = resolveExternalModuleName(node, getExternalModuleImportEqualsDeclarationExpression(node));
const resolved = resolveExternalModuleSymbol(immediate);
if (!markSymbolOfAliasDeclarationIfResolvesToTypeOnly(node, immediate)) {
markSymbolOfAliasDeclarationIfResolvesToTypeOnly(node, resolved);
}
return resolved;
}
const resolved = getSymbolOfPartOfRightHandSideOfImportEquals(node.moduleReference, dontResolveAlias);
checkAndReportErrorForResolvingImportAliasToTypeOnlySymbol(node, resolved);
return resolved;
}

function checkAndReportErrorForResolvingImportAliasToTypeOnlySymbol(node: ImportEqualsDeclaration, resolved: Symbol | undefined) {
if (markSymbolOfAliasDeclarationIfResolvesToTypeOnly(node, resolved)) {
const typeOnlyDeclaration = getTypeOnlyAliasDeclaration(getSymbolOfNode(node))!;
const message = typeOnlyDeclaration.kind === SyntaxKind.ExportSpecifier
? Diagnostics.An_import_alias_cannot_reference_a_declaration_that_was_exported_using_export_type
: Diagnostics.An_import_alias_cannot_reference_a_declaration_that_was_imported_using_import_type;
const relatedMessage = typeOnlyDeclaration.kind === SyntaxKind.ExportSpecifier
? Diagnostics._0_was_exported_here
: Diagnostics._0_was_imported_here;
// Non-null assertion is safe because the optionality comes from ImportClause,
// but if an ImportClause was the typeOnlyDeclaration, it had to have a `name`.
const name = unescapeLeadingUnderscores(typeOnlyDeclaration.name!.escapedText);
addRelatedInfo(error(node.moduleReference, message), createDiagnosticForNode(typeOnlyDeclaration, relatedMessage, name));
}
return getSymbolOfPartOfRightHandSideOfImportEquals(node.moduleReference, dontResolveAlias);
}

function resolveExportByName(moduleSymbol: Symbol, name: __String, sourceNode: TypeOnlyCompatibleAliasDeclaration | undefined, dontResolveAlias: boolean) {
Expand All @@ -2193,8 +2216,11 @@ namespace ts {
return getPropertyOfType(getTypeOfSymbol(exportValue), name);
}
const exportSymbol = moduleSymbol.exports!.get(name);
markSymbolOfAliasDeclarationIfResolvesToTypeOnly(sourceNode, exportSymbol);
return resolveSymbol(exportSymbol, dontResolveAlias);
const resolved = resolveSymbol(exportSymbol, dontResolveAlias);
if (!markSymbolOfAliasDeclarationIfResolvesToTypeOnly(sourceNode, exportSymbol)) {
markSymbolOfAliasDeclarationIfResolvesToTypeOnly(sourceNode, resolved);
}
return resolved;
}

function isSyntacticDefault(node: Node) {
Expand Down Expand Up @@ -2234,7 +2260,7 @@ namespace ts {

function getTargetOfImportClause(node: ImportClause, dontResolveAlias: boolean): Symbol | undefined {
const moduleSymbol = resolveExternalModuleName(node, node.parent.moduleSpecifier);
markSymbolOfAliasDeclarationIfTypeOnly(node);
const isTypeOnly = markSymbolOfAliasDeclarationIfTypeOnly(node);

if (moduleSymbol) {
let exportDefaultSymbol: Symbol | undefined;
Expand Down Expand Up @@ -2277,16 +2303,26 @@ namespace ts {
}
else if (hasSyntheticDefault) {
// per emit behavior, a synthetic default overrides a "real" .default member if `__esModule` is not present
return resolveExternalModuleSymbol(moduleSymbol, dontResolveAlias) || resolveSymbol(moduleSymbol, dontResolveAlias);
const resolved = resolveExternalModuleSymbol(moduleSymbol, dontResolveAlias) || resolveSymbol(moduleSymbol, dontResolveAlias);
if (!isTypeOnly && !markSymbolOfAliasDeclarationIfResolvesToTypeOnly(node, moduleSymbol)) {
markSymbolOfAliasDeclarationIfResolvesToTypeOnly(node, resolved);
}
return resolved;
}
if (!isTypeOnly) markSymbolOfAliasDeclarationIfResolvesToTypeOnly(node, exportDefaultSymbol);
return exportDefaultSymbol;
}
}

function getTargetOfNamespaceImport(node: NamespaceImport, dontResolveAlias: boolean): Symbol | undefined {
const moduleSpecifier = node.parent.parent.moduleSpecifier;
markSymbolOfAliasDeclarationIfTypeOnly(node);
return resolveESModuleSymbol(resolveExternalModuleName(node, moduleSpecifier), moduleSpecifier, dontResolveAlias, /*suppressUsageError*/ false);
const isTypeOnly = markSymbolOfAliasDeclarationIfTypeOnly(node);
const immediate = resolveExternalModuleName(node, moduleSpecifier);
const resolved = resolveESModuleSymbol(immediate, moduleSpecifier, dontResolveAlias, /*suppressUsageError*/ false);
if (!isTypeOnly && !markSymbolOfAliasDeclarationIfResolvesToTypeOnly(node, immediate)) {
markSymbolOfAliasDeclarationIfResolvesToTypeOnly(node, resolved);
}
return resolved;
}

function getTargetOfNamespaceExport(node: NamespaceExport, dontResolveAlias: boolean): Symbol | undefined {
Expand Down Expand Up @@ -2332,8 +2368,11 @@ namespace ts {
if (symbol.flags & SymbolFlags.Module) {
const name = (specifier.propertyName ?? specifier.name).escapedText;
const exportSymbol = getExportsOfSymbol(symbol).get(name);
markSymbolOfAliasDeclarationIfResolvesToTypeOnly(specifier, exportSymbol);
return resolveSymbol(exportSymbol, dontResolveAlias);
const resolved = resolveSymbol(exportSymbol, dontResolveAlias);
if (!markSymbolOfAliasDeclarationIfResolvesToTypeOnly(specifier, exportSymbol)) {
markSymbolOfAliasDeclarationIfResolvesToTypeOnly(specifier, resolved);
}
return resolved;
}
}

Expand Down Expand Up @@ -2433,24 +2472,30 @@ namespace ts {
}

function getTargetOfImportSpecifier(node: ImportSpecifier, dontResolveAlias: boolean): Symbol | undefined {
markSymbolOfAliasDeclarationIfTypeOnly(node);
return getExternalModuleMember(node.parent.parent.parent, node, dontResolveAlias);
const isTypeOnly = markSymbolOfAliasDeclarationIfTypeOnly(node);
const resolved = getExternalModuleMember(node.parent.parent.parent, node, dontResolveAlias);
if (!isTypeOnly) markSymbolOfAliasDeclarationIfResolvesToTypeOnly(node, resolved);
return resolved;
}

function getTargetOfNamespaceExportDeclaration(node: NamespaceExportDeclaration, dontResolveAlias: boolean): Symbol {
return resolveExternalModuleSymbol(node.parent.symbol, dontResolveAlias);
}

function getTargetOfExportSpecifier(node: ExportSpecifier, meaning: SymbolFlags, dontResolveAlias?: boolean) {
markSymbolOfAliasDeclarationIfTypeOnly(node);
return node.parent.parent.moduleSpecifier ?
const isTypeOnly = markSymbolOfAliasDeclarationIfTypeOnly(node);
const resolved = node.parent.parent.moduleSpecifier ?
getExternalModuleMember(node.parent.parent, node, dontResolveAlias) :
resolveEntityName(node.propertyName || node.name, meaning, /*ignoreErrors*/ false, dontResolveAlias);
if (!isTypeOnly) markSymbolOfAliasDeclarationIfResolvesToTypeOnly(node, resolved);
return resolved;
}

function getTargetOfExportAssignment(node: ExportAssignment | BinaryExpression, dontResolveAlias: boolean): Symbol | undefined {
const expression = (isExportAssignment(node) ? node.expression : node.right) as EntityNameExpression | ClassExpression;
return getTargetOfAliasLikeExpression(expression, dontResolveAlias);
const resolved = getTargetOfAliasLikeExpression(expression, dontResolveAlias);
markSymbolOfAliasDeclarationIfResolvesToTypeOnly(node, resolved);
return resolved;
}

function getTargetOfAliasLikeExpression(expression: Expression, dontResolveAlias: boolean) {
Expand Down Expand Up @@ -2547,22 +2592,38 @@ namespace ts {
return links.target;
}

function markSymbolOfAliasDeclarationIfResolvesToTypeOnly(aliasDeclaration: TypeOnlyCompatibleAliasDeclaration | undefined, resolvesToSymbol: Symbol | undefined) {
if (!aliasDeclaration || !resolvesToSymbol) return;
/**
* If an alias symbol is not itself marked type-only, but resolves to a type-only alias
* somewhere in its resolution chain, save a reference to the type-only alias declaration
* so the alias _not_ marked type-only can be identified as _transitively_ type-only.
* @param aliasDeclaration The alias declaration not marked as type-only
* @param resolvesToSymbol A symbol somewhere in the resolution chain of the alias symbol
* @param overwriteEmpty Checks `resolvesToSymbol` for type-only declarations even if `aliasDeclaration`
* has already been marked as not resolving to a type-only alias. Used when recursively resolving qualified
* names of import aliases, e.g. `import C = a.b.C`. If namespace `a` is not found to be type-only, the
* import declaration will initially be marked as not resolving to a type-only symbol. But, namespace `b`
* must still be checked for a type-only marker, overwriting the previous negative result if found.
*/
function markSymbolOfAliasDeclarationIfResolvesToTypeOnly(aliasDeclaration: Declaration | undefined, resolvesToSymbol: Symbol | undefined, overwriteEmpty?: boolean): boolean {
if (!aliasDeclaration || !resolvesToSymbol) return false;
const sourceSymbol = getSymbolOfNode(aliasDeclaration);
const links = getSymbolLinks(sourceSymbol);
if (links.typeOnlyDeclaration === undefined) {
const typeOnly = find(resolvesToSymbol.declarations, isTypeOnlyImportOrExportDeclaration);
if (links.typeOnlyDeclaration === undefined || overwriteEmpty && links.typeOnlyDeclaration === false) {
resolvesToSymbol = resolvesToSymbol.exports?.get(InternalSymbolName.ExportEquals) ?? resolvesToSymbol;
Copy link
Member

Choose a reason for hiding this comment

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

I would declare a new variable just because I hate mutation so much:

Suggested change
resolvesToSymbol = resolvesToSymbol.exports?.get(InternalSymbolName.ExportEquals) ?? resolvesToSymbol;
const resolved = resolvesToSymbol.exports?.get(InternalSymbolName.ExportEquals) ?? resolvesToSymbol;

Not sure about the name.

Copy link
Member

Choose a reason for hiding this comment

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

Are there other kinds of aliases we might have to manually follow the same way as import =? This seems like a slightly odd place to manually get ExportEquals out of the exports, although I could be wrong.

Copy link
Member Author

Choose a reason for hiding this comment

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

Are there other kinds of aliases we might have to manually follow the same way as import =?

The answer is almost definitely “no,” but I’ve been struggling to concisely explain why for too long at this point, which makes me realize that the reason I’m confident that I’m right is mostly because of thorough test coverage.

const typeOnly = resolvesToSymbol.declarations && find(resolvesToSymbol.declarations, isTypeOnlyImportOrExportDeclaration);
andrewbranch marked this conversation as resolved.
Show resolved Hide resolved
links.typeOnlyDeclaration = typeOnly ?? getSymbolLinks(resolvesToSymbol).typeOnlyDeclaration ?? false;
}
return !!links.typeOnlyDeclaration;
}

function markSymbolOfAliasDeclarationIfTypeOnly(aliasDeclaration: TypeOnlyCompatibleAliasDeclaration) {
function markSymbolOfAliasDeclarationIfTypeOnly(aliasDeclaration: TypeOnlyCompatibleAliasDeclaration): boolean {
andrewbranch marked this conversation as resolved.
Show resolved Hide resolved
if (isTypeOnlyImportOrExportDeclaration(aliasDeclaration)) {
const symbol = getSymbolOfNode(aliasDeclaration);
const links = getSymbolLinks(symbol);
links.typeOnlyDeclaration = aliasDeclaration;
return true;
}
return false;
}

/** Indicates that a symbol directly or indirectly resolves to a type-only import or export. */
Expand Down Expand Up @@ -2700,8 +2761,8 @@ namespace ts {
throw Debug.assertNever(name, "Unknown entity name kind.");
}
Debug.assert((getCheckFlags(symbol) & CheckFlags.Instantiated) === 0, "Should never get an instantiated symbol here.");
if (isIdentifier(name) && symbol.flags & SymbolFlags.Alias) {
markSymbolOfAliasDeclarationIfResolvesToTypeOnly(getTypeOnlyCompatibleAliasDeclarationFromName(name), symbol);
if (isEntityName(name) && (symbol.flags & SymbolFlags.Alias || name.parent.kind === SyntaxKind.ExportAssignment)) {
markSymbolOfAliasDeclarationIfResolvesToTypeOnly(getAliasDeclarationFromName(name), symbol, /*overwriteEmpty*/ true);
}
return (symbol.flags & meaning) || dontResolveAlias ? symbol : resolveAlias(symbol);
}
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2011,4 +2011,4 @@ namespace ts {
}
}
}
}
}
andrewbranch marked this conversation as resolved.
Show resolved Hide resolved
8 changes: 8 additions & 0 deletions src/compiler/diagnosticMessages.json
Original file line number Diff line number Diff line change
Expand Up @@ -1127,6 +1127,14 @@
"category": "Error",
"code": 1378
},
"An import alias cannot reference a declaration that was exported using 'export type'.": {
"category": "Error",
"code": 1379
},
"An import alias cannot reference a declaration that was imported using 'import type'.": {
"category": "Error",
"code": 1380
},
andrewbranch marked this conversation as resolved.
Show resolved Hide resolved

"The types of '{0}' are incompatible between these types.": {
"category": "Error",
Expand Down
20 changes: 14 additions & 6 deletions src/compiler/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1792,9 +1792,10 @@ namespace ts {
return containerKind === SyntaxKind.InterfaceDeclaration || containerKind === SyntaxKind.TypeLiteral;
}

export function isFirstIdentifierOfImplementsClause(node: Node) {
return node.parent?.parent?.parent?.kind === SyntaxKind.HeritageClause
&& (node.parent.parent.parent as HeritageClause).token === SyntaxKind.ImplementsKeyword;
export function isFirstIdentifierOfNonEmittingHeritageClause(node: Node): boolean {
andrewbranch marked this conversation as resolved.
Show resolved Hide resolved
// Number of parents to climb from identifier is 2 for `implements I`, 3 for `implements x.I`
andrewbranch marked this conversation as resolved.
Show resolved Hide resolved
const heritageClause = tryCast(node.parent.parent, isHeritageClause) ?? tryCast(node.parent.parent?.parent, isHeritageClause);
andrewbranch marked this conversation as resolved.
Show resolved Hide resolved
return heritageClause?.token === SyntaxKind.ImplementsKeyword || heritageClause?.parent.kind === SyntaxKind.InterfaceDeclaration;
}

export function isExternalModuleImportEqualsDeclaration(node: Node): node is ImportEqualsDeclaration & { moduleReference: ExternalModuleReference } {
Expand Down Expand Up @@ -2765,13 +2766,20 @@ namespace ts {
node.kind === SyntaxKind.PropertyAssignment && isAliasableExpression((node as PropertyAssignment).initializer);
}

export function getTypeOnlyCompatibleAliasDeclarationFromName(node: Identifier): TypeOnlyCompatibleAliasDeclaration | undefined {
export function getAliasDeclarationFromName(node: EntityName): Declaration | undefined {
switch (node.parent.kind) {
case SyntaxKind.ImportClause:
case SyntaxKind.ImportSpecifier:
case SyntaxKind.NamespaceImport:
case SyntaxKind.ExportSpecifier:
return node.parent as TypeOnlyCompatibleAliasDeclaration;
case SyntaxKind.ExportAssignment:
case SyntaxKind.ImportEqualsDeclaration:
return node.parent as Declaration;
case SyntaxKind.QualifiedName:
do {
node = node.parent as QualifiedName;
} while (node.parent.kind === SyntaxKind.QualifiedName);
return getAliasDeclarationFromName(node);
}
}

Expand Down Expand Up @@ -6143,7 +6151,7 @@ namespace ts {
export function isValidTypeOnlyAliasUseSite(useSite: Node): boolean {
return !!(useSite.flags & NodeFlags.Ambient)
|| isPartOfTypeQuery(useSite)
|| isFirstIdentifierOfImplementsClause(useSite)
|| isFirstIdentifierOfNonEmittingHeritageClause(useSite)
|| isPartOfPossiblyValidTypeOrAbstractComputedPropertyName(useSite)
|| !isExpressionNode(useSite);
}
Expand Down
1 change: 1 addition & 0 deletions src/testRunner/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,7 @@
"unittests/tsserver/textStorage.ts",
"unittests/tsserver/telemetry.ts",
"unittests/tsserver/typeAquisition.ts",
"unittests/tsserver/typeOnlyImportChains.ts",
"unittests/tsserver/typeReferenceDirectives.ts",
"unittests/tsserver/typingsInstaller.ts",
"unittests/tsserver/untitledFiles.ts",
Expand Down
Loading