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

Forbid augmentation of untyped module #11962

Merged
merged 2 commits into from
Oct 31, 2016
Merged
Show file tree
Hide file tree
Changes from all 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
47 changes: 21 additions & 26 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -498,7 +498,7 @@ namespace ts {
const moduleNotFoundError = !isInAmbientContext(moduleName.parent.parent)
? Diagnostics.Invalid_module_name_in_augmentation_module_0_cannot_be_found
: undefined;
let mainModule = resolveExternalModuleNameWorker(moduleName, moduleName, moduleNotFoundError);
let mainModule = resolveExternalModuleNameWorker(moduleName, moduleName, moduleNotFoundError, /*isForAugmentation*/ true);
if (!mainModule) {
return;
}
Expand Down Expand Up @@ -1074,7 +1074,7 @@ namespace ts {
const moduleSymbol = resolveExternalModuleName(node, (<ImportDeclaration>node.parent).moduleSpecifier);

if (moduleSymbol) {
const exportDefaultSymbol = isUntypedModuleSymbol(moduleSymbol) ?
const exportDefaultSymbol = isShorthandAmbientModuleSymbol(moduleSymbol) ?
moduleSymbol :
moduleSymbol.exports["export="] ?
getPropertyOfType(getTypeOfSymbol(moduleSymbol.exports["export="]), "default") :
Expand Down Expand Up @@ -1150,7 +1150,7 @@ namespace ts {
if (targetSymbol) {
const name = specifier.propertyName || specifier.name;
if (name.text) {
if (isUntypedModuleSymbol(moduleSymbol)) {
if (isShorthandAmbientModuleSymbol(moduleSymbol)) {
return moduleSymbol;
}

Expand Down Expand Up @@ -1351,16 +1351,16 @@ namespace ts {
return resolveExternalModuleNameWorker(location, moduleReferenceExpression, Diagnostics.Cannot_find_module_0);
}

function resolveExternalModuleNameWorker(location: Node, moduleReferenceExpression: Expression, moduleNotFoundError: DiagnosticMessage): Symbol {
function resolveExternalModuleNameWorker(location: Node, moduleReferenceExpression: Expression, moduleNotFoundError: DiagnosticMessage, isForAugmentation = false): Symbol {
if (moduleReferenceExpression.kind !== SyntaxKind.StringLiteral) {
return;
}

const moduleReferenceLiteral = <LiteralExpression>moduleReferenceExpression;
return resolveExternalModule(location, moduleReferenceLiteral.text, moduleNotFoundError, moduleReferenceLiteral);
return resolveExternalModule(location, moduleReferenceLiteral.text, moduleNotFoundError, moduleReferenceLiteral, isForAugmentation);
}

function resolveExternalModule(location: Node, moduleReference: string, moduleNotFoundError: DiagnosticMessage, errorNode: Node): Symbol {
function resolveExternalModule(location: Node, moduleReference: string, moduleNotFoundError: DiagnosticMessage, errorNode: Node, isForAugmentation = false): Symbol {
// Module names are escaped in our symbol table. However, string literal values aren't.
// Escape the name in the "require(...)" clause to ensure we find the right symbol.
const moduleName = escapeIdentifier(moduleReference);
Expand Down Expand Up @@ -1403,24 +1403,19 @@ namespace ts {

// May be an untyped module. If so, ignore resolutionDiagnostic.
if (!isRelative && resolvedModule && !extensionIsTypeScript(resolvedModule.extension)) {
if (compilerOptions.noImplicitAny) {
if (moduleNotFoundError) {
error(errorNode,
Diagnostics.Could_not_find_a_declaration_file_for_module_0_1_implicitly_has_an_any_type,
moduleReference,
resolvedModule.resolvedFileName);
}
return undefined;
}

// Create a new symbol to represent the untyped module and store it in globals.
// This provides a name to the module. See the test tests/cases/fourslash/untypedModuleImport.ts
const newSymbol = createSymbol(SymbolFlags.ValueModule, quotedName);
// Module symbols are expected to have 'exports', although since this is an untyped module it can be empty.
newSymbol.exports = createMap<Symbol>();
// Cache it so subsequent accesses will return the same module.
globals[quotedName] = newSymbol;
return newSymbol;
if (isForAugmentation) {
Debug.assert(!!moduleNotFoundError);
const diag = Diagnostics.Invalid_module_name_in_augmentation_Module_0_resolves_to_an_untyped_module_at_1_which_cannot_be_augmented;
error(errorNode, diag, moduleName, resolvedModule.resolvedFileName);
}
else if (compilerOptions.noImplicitAny && moduleNotFoundError) {
error(errorNode,
Diagnostics.Could_not_find_a_declaration_file_for_module_0_1_implicitly_has_an_any_type,
moduleReference,
resolvedModule.resolvedFileName);
}
// Failed imports and untyped modules are both treated in an untyped manner; only difference is whether we give a diagnostic first.
return undefined;
}

if (moduleNotFoundError) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still not convinced why do we need to create a new symbol here an stick it into globals? AFAIR returning undefined will be should be properly interpreted downstream and converted into unknownSymbol

Expand Down Expand Up @@ -3490,7 +3485,7 @@ namespace ts {
function getTypeOfFuncClassEnumModule(symbol: Symbol): Type {
const links = getSymbolLinks(symbol);
if (!links.type) {
if (symbol.flags & SymbolFlags.Module && isUntypedModuleSymbol(symbol)) {
if (symbol.flags & SymbolFlags.Module && isShorthandAmbientModuleSymbol(symbol)) {
links.type = anyType;
}
else {
Expand Down Expand Up @@ -19063,7 +19058,7 @@ namespace ts {

function moduleExportsSomeValue(moduleReferenceExpression: Expression): boolean {
let moduleSymbol = resolveExternalModuleName(moduleReferenceExpression.parent, moduleReferenceExpression);
if (!moduleSymbol || isUntypedModuleSymbol(moduleSymbol)) {
if (!moduleSymbol || isShorthandAmbientModuleSymbol(moduleSymbol)) {
// If the module is not found or is shorthand, assume that it may export a value.
return true;
}
Expand Down
4 changes: 4 additions & 0 deletions src/compiler/diagnosticMessages.json
Original file line number Diff line number Diff line change
Expand Up @@ -1839,6 +1839,10 @@
"category": "Error",
"code": 2664
},
"Invalid module name in augmentation. Module '{0}' resolves to an untyped module at '{1}', which cannot be augmented.": {
"category": "Error",
"code": 2665
},
"Exports and export assignments are not permitted in module augmentations.": {
"category": "Error",
"code": 2666
Expand Down
4 changes: 2 additions & 2 deletions src/compiler/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -403,8 +403,8 @@ namespace ts {
}

/** Given a symbol for a module, checks that it is either an untyped import or a shorthand ambient module. */
export function isUntypedModuleSymbol(moduleSymbol: Symbol): boolean {
return !moduleSymbol.valueDeclaration || isShorthandAmbientModule(moduleSymbol.valueDeclaration);
export function isShorthandAmbientModuleSymbol(moduleSymbol: Symbol): boolean {
return isShorthandAmbientModule(moduleSymbol.valueDeclaration);
}

function isShorthandAmbientModule(node: Node): boolean {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/a.ts(1,16): error TS2665: Invalid module name in augmentation. Module 'foo' resolves to an untyped module at '/node_modules/foo/index.js', which cannot be augmented.


==== /a.ts (1 errors) ====
declare module "foo" {
~~~~~
!!! error TS2665: Invalid module name in augmentation. Module 'foo' resolves to an untyped module at '/node_modules/foo/index.js', which cannot be augmented.
export const x: number;
}
import { x } from "foo";
x;

==== /node_modules/foo/index.js (0 errors) ====
// This tests that augmenting an untyped module is forbidden.

This file is not processed.

19 changes: 19 additions & 0 deletions tests/baselines/reference/untypedModuleImport_withAugmentation.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
//// [tests/cases/conformance/moduleResolution/untypedModuleImport_withAugmentation.ts] ////

//// [index.js]
// This tests that augmenting an untyped module is forbidden.

This file is not processed.

//// [a.ts]
declare module "foo" {
export const x: number;
}
import { x } from "foo";
x;


//// [a.js]
"use strict";
var foo_1 = require("foo");
foo_1.x;
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// @noImplicitReferences: true
// @currentDirectory: /
// This tests that augmenting an untyped module is forbidden.

// @filename: /node_modules/foo/index.js
This file is not processed.

// @filename: /a.ts
declare module "foo" {
export const x: number;
}
import { x } from "foo";
x;
2 changes: 1 addition & 1 deletion tests/cases/fourslash/untypedModuleImport.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ verify.numberOfErrorsInCurrentFile(0);

goTo.marker("fooModule");
verify.goToDefinitionIs([]);
verify.quickInfoIs('module "foo"');
verify.quickInfoIs("");
verify.referencesAre([])

goTo.marker("foo");
Expand Down