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

Check if imported file is a proper external module for imported node typings #4738

Merged
merged 4 commits into from
Sep 11, 2015
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
4 changes: 2 additions & 2 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -976,8 +976,8 @@ namespace ts {
}
}

let fileName = getResolvedModuleFileName(getSourceFile(location), moduleReferenceLiteral.text);
let sourceFile = fileName && host.getSourceFile(fileName);
let resolvedModule = getResolvedModule(getSourceFile(location), moduleReferenceLiteral.text);
let sourceFile = resolvedModule && host.getSourceFile(resolvedModule.resolvedFileName);
if (sourceFile) {
if (sourceFile.symbol) {
return sourceFile.symbol;
Expand Down
2 changes: 2 additions & 0 deletions src/compiler/diagnosticInformationMap.generated.ts
Original file line number Diff line number Diff line change
Expand Up @@ -428,6 +428,8 @@ namespace ts {
A_member_initializer_in_a_enum_declaration_cannot_reference_members_declared_after_it_including_members_defined_in_other_enums: { code: 2651, category: DiagnosticCategory.Error, key: "A member initializer in a enum declaration cannot reference members declared after it, including members defined in other enums." },
Merged_declaration_0_cannot_include_a_default_export_declaration_Consider_adding_a_separate_export_default_0_declaration_instead: { code: 2652, category: DiagnosticCategory.Error, key: "Merged declaration '{0}' cannot include a default export declaration. Consider adding a separate 'export default {0}' declaration instead." },
Non_abstract_class_expression_does_not_implement_inherited_abstract_member_0_from_class_1: { code: 2653, category: DiagnosticCategory.Error, key: "Non-abstract class expression does not implement inherited abstract member '{0}' from class '{1}'." },
Proper_external_module_that_carries_external_typings_cannot_contain_tripleslash_references: { code: 2654, category: DiagnosticCategory.Error, key: "Proper external module that carries external typings cannot contain tripleslash references." },
Proper_external_module_that_carries_external_typings_should_be_d_ts_file: { code: 2655, category: DiagnosticCategory.Error, key: "Proper external module that carries external typings should be '.d.ts' file." },
Import_declaration_0_is_using_private_name_1: { code: 4000, category: DiagnosticCategory.Error, key: "Import declaration '{0}' is using private name '{1}'." },
Type_parameter_0_of_exported_class_has_or_is_using_private_name_1: { code: 4002, category: DiagnosticCategory.Error, key: "Type parameter '{0}' of exported class has or is using private name '{1}'." },
Type_parameter_0_of_exported_interface_has_or_is_using_private_name_1: { code: 4004, category: DiagnosticCategory.Error, key: "Type parameter '{0}' of exported interface has or is using private name '{1}'." },
Expand Down
9 changes: 8 additions & 1 deletion src/compiler/diagnosticMessages.json
Original file line number Diff line number Diff line change
Expand Up @@ -1701,7 +1701,14 @@
"category": "Error",
"code": 2653
},

"Proper external module that carries external typings cannot contain tripleslash references.": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Exported external package typings file cannot contain tripleslash references. Please contact the package author to update the package definition.

"category": "Error",
"code": 2654
},
"Proper external module that carries external typings should be '.d.ts' file.": {
Copy link
Contributor

Choose a reason for hiding this comment

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

we do not have a test for this message.

Copy link
Contributor

Choose a reason for hiding this comment

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

Exported external package typings can only be in .d.ts files. Please contact the package author to update the package definition.

"category": "Error",
"code": 2655
},
"Import declaration '{0}' is using private name '{1}'.": {
"category": "Error",
"code": 4000
Expand Down
91 changes: 44 additions & 47 deletions src/compiler/program.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ namespace ts {
return normalizePath(referencedFileName);
}

export function resolveModuleName(moduleName: string, containingFile: string, compilerOptions: CompilerOptions, host: ModuleResolutionHost): ResolvedModule {
export function resolveModuleName(moduleName: string, containingFile: string, compilerOptions: CompilerOptions, host: ModuleResolutionHost): ResolvedModuleWithFailedLookupLocations {
let moduleResolution = compilerOptions.moduleResolution !== undefined
? compilerOptions.moduleResolution
: compilerOptions.module === ModuleKind.CommonJS ? ModuleResolutionKind.NodeJs : ModuleResolutionKind.Classic;
Expand All @@ -47,7 +47,7 @@ namespace ts {
}
}

export function nodeModuleNameResolver(moduleName: string, containingFile: string, host: ModuleResolutionHost): ResolvedModule {
export function nodeModuleNameResolver(moduleName: string, containingFile: string, host: ModuleResolutionHost): ResolvedModuleWithFailedLookupLocations {
let containingDirectory = getDirectoryPath(containingFile);

if (getRootLength(moduleName) !== 0 || nameStartsWithDotSlashOrDotDotSlash(moduleName)) {
Expand All @@ -56,11 +56,13 @@ namespace ts {
let resolvedFileName = loadNodeModuleFromFile(candidate, /* loadOnlyDts */ false, failedLookupLocations, host);

if (resolvedFileName) {
return { resolvedFileName, failedLookupLocations };
return { resolvedModule: { resolvedFileName }, failedLookupLocations };
}

resolvedFileName = loadNodeModuleFromDirectory(candidate, /* loadOnlyDts */ false, failedLookupLocations, host);
return { resolvedFileName, failedLookupLocations };
return resolvedFileName
? { resolvedModule: { resolvedFileName }, failedLookupLocations }
: { resolvedModule: undefined, failedLookupLocations };
}
else {
return loadModuleFromNodeModules(moduleName, containingDirectory, host);
Expand Down Expand Up @@ -117,7 +119,7 @@ namespace ts {
return loadNodeModuleFromFile(combinePaths(candidate, "index"), loadOnlyDts, failedLookupLocation, host);
}

function loadModuleFromNodeModules(moduleName: string, directory: string, host: ModuleResolutionHost): ResolvedModule {
function loadModuleFromNodeModules(moduleName: string, directory: string, host: ModuleResolutionHost): ResolvedModuleWithFailedLookupLocations {
let failedLookupLocations: string[] = [];
directory = normalizeSlashes(directory);
while (true) {
Expand All @@ -127,12 +129,12 @@ namespace ts {
let candidate = normalizePath(combinePaths(nodeModulesFolder, moduleName));
let result = loadNodeModuleFromFile(candidate, /* loadOnlyDts */ true, failedLookupLocations, host);
if (result) {
return { resolvedFileName: result, failedLookupLocations };
return { resolvedModule: { resolvedFileName: result, shouldBeProperExternalModule: true }, failedLookupLocations };
}

result = loadNodeModuleFromDirectory(candidate, /* loadOnlyDts */ true, failedLookupLocations, host);
if (result) {
return { resolvedFileName: result, failedLookupLocations };
return { resolvedModule: { resolvedFileName: result, shouldBeProperExternalModule: true }, failedLookupLocations };
}
}

Expand All @@ -144,47 +146,19 @@ namespace ts {
directory = parentPath;
}

return { resolvedFileName: undefined, failedLookupLocations };
}

export function baseUrlModuleNameResolver(moduleName: string, containingFile: string, baseUrl: string, host: ModuleResolutionHost): ResolvedModule {
Debug.assert(baseUrl !== undefined);

let normalizedModuleName = normalizeSlashes(moduleName);
let basePart = useBaseUrl(moduleName) ? baseUrl : getDirectoryPath(containingFile);
let candidate = normalizePath(combinePaths(basePart, moduleName));

let failedLookupLocations: string[] = [];

return forEach(supportedExtensions, ext => tryLoadFile(candidate + ext)) || { resolvedFileName: undefined, failedLookupLocations };

function tryLoadFile(location: string): ResolvedModule {
if (host.fileExists(location)) {
return { resolvedFileName: location, failedLookupLocations };
}
else {
failedLookupLocations.push(location);
return undefined;
}
}
return { resolvedModule: undefined, failedLookupLocations };
}

function nameStartsWithDotSlashOrDotDotSlash(name: string) {
let i = name.lastIndexOf("./", 1);
return i === 0 || (i === 1 && name.charCodeAt(0) === CharacterCodes.dot);
}

function useBaseUrl(moduleName: string): boolean {
// path is not rooted
// module name does not start with './' or '../'
return getRootLength(moduleName) === 0 && !nameStartsWithDotSlashOrDotDotSlash(moduleName);
}

export function classicNameResolver(moduleName: string, containingFile: string, compilerOptions: CompilerOptions, host: ModuleResolutionHost): ResolvedModule {
export function classicNameResolver(moduleName: string, containingFile: string, compilerOptions: CompilerOptions, host: ModuleResolutionHost): ResolvedModuleWithFailedLookupLocations {

// module names that contain '!' are used to reference resources and are not resolved to actual files on disk
if (moduleName.indexOf('!') != -1) {
return { resolvedFileName: undefined, failedLookupLocations: [] };
return { resolvedModule: undefined, failedLookupLocations: [] };
}

let searchPath = getDirectoryPath(containingFile);
Expand Down Expand Up @@ -222,7 +196,9 @@ namespace ts {
searchPath = parentPath;
}

return { resolvedFileName: referencedSourceFile, failedLookupLocations };
return referencedSourceFile
? { resolvedModule: { resolvedFileName: referencedSourceFile }, failedLookupLocations }
: { resolvedModule: undefined, failedLookupLocations };
}

/* @internal */
Expand Down Expand Up @@ -371,9 +347,9 @@ namespace ts {

host = host || createCompilerHost(options);

const resolveModuleNamesWorker =
host.resolveModuleNames ||
((moduleNames, containingFile) => map(moduleNames, moduleName => resolveModuleName(moduleName, containingFile, options, host).resolvedFileName));
const resolveModuleNamesWorker = host.resolveModuleNames
? ((moduleNames: string[], containingFile: string) => host.resolveModuleNames(moduleNames, containingFile))
: ((moduleNames: string[], containingFile: string) => map(moduleNames, moduleName => resolveModuleName(moduleName, containingFile, options, host).resolvedModule));

let filesByName = createFileMap<SourceFile>(fileName => host.getCanonicalFileName(fileName));

Expand Down Expand Up @@ -491,10 +467,17 @@ namespace ts {
let resolutions = resolveModuleNamesWorker(moduleNames, newSourceFile.fileName);
// ensure that module resolution results are still correct
for (let i = 0; i < moduleNames.length; ++i) {
let oldResolution = getResolvedModuleFileName(oldSourceFile, moduleNames[i]);
if (oldResolution !== resolutions[i]) {
let newResolution = resolutions[i];
let oldResolution = getResolvedModule(oldSourceFile, moduleNames[i]);
let resolutionChanged = oldResolution
? !newResolution ||
oldResolution.resolvedFileName !== newResolution.resolvedFileName ||
!!oldResolution.shouldBeProperExternalModule !== !!newResolution.shouldBeProperExternalModule
: newResolution;

if (resolutionChanged) {
return false;
}
}
}
}
// pass the cache of module resolutions from the old source file
Expand Down Expand Up @@ -864,9 +847,23 @@ namespace ts {
let resolutions = resolveModuleNamesWorker(moduleNames, file.fileName);
for (let i = 0; i < file.imports.length; ++i) {
let resolution = resolutions[i];
setResolvedModuleName(file, moduleNames[i], resolution);
setResolvedModule(file, moduleNames[i], resolution);
if (resolution && !options.noResolve) {
findModuleSourceFile(resolution, file.imports[i]);
const importedFile = findModuleSourceFile(resolution.resolvedFileName, file.imports[i]);
if (importedFile && resolution.shouldBeProperExternalModule) {
if (!isExternalModule(importedFile)) {
let start = getTokenPosOfNode(file.imports[i], file)
diagnostics.add(createFileDiagnostic(file, start, file.imports[i].end - start, Diagnostics.File_0_is_not_a_module, importedFile.fileName));
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add a new message to also indicate that it is the package author's responsibility

Copy link
Contributor

Choose a reason for hiding this comment

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

Exported external package typings can only be modules. Please contact the package author to update the package definition.

}
else if (!fileExtensionIs(importedFile.fileName, ".d.ts")) {
let start = getTokenPosOfNode(file.imports[i], file)
diagnostics.add(createFileDiagnostic(file, start, file.imports[i].end - start, Diagnostics.Proper_external_module_that_carries_external_typings_should_be_d_ts_file));
}
else if (importedFile.referencedFiles.length) {
let firstRef = importedFile.referencedFiles[0];
diagnostics.add(createFileDiagnostic(importedFile, firstRef.pos, firstRef.end - firstRef.pos, Diagnostics.Proper_external_module_that_carries_external_typings_cannot_contain_tripleslash_references));
}
}
}
}
}
Expand Down
17 changes: 13 additions & 4 deletions src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1284,7 +1284,7 @@ namespace ts {
// Stores a mapping 'external module reference text' -> 'resolved file name' | undefined
// It is used to resolve module names in the checker.
// Content of this fiels should never be used directly - use getResolvedModuleFileName/setResolvedModuleFileName functions instead
/* @internal */ resolvedModules: Map<string>;
/* @internal */ resolvedModules: Map<ResolvedModule>;
/* @internal */ imports: LiteralExpression[];
}

Expand Down Expand Up @@ -2269,11 +2269,20 @@ namespace ts {

export interface ResolvedModule {
resolvedFileName: string;
/*
* Denotes if 'resolvedFileName' should be proper external module:
* - be a .d.ts file
* - use top level imports\exports
* - don't use tripleslash references
*/
shouldBeProperExternalModule?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

i think this should be declarative. i.e. the name tells you what it is rather than telling you how it should be have. so something like
isExternalLibrary, or isExternalLibraryImport

}

export interface ResolvedModuleWithFailedLookupLocations {
resolvedModule: ResolvedModule;
failedLookupLocations: string[];
}

export type ModuleNameResolver = (moduleName: string, containingFile: string, options: CompilerOptions, host: ModuleResolutionHost) => ResolvedModule;

export interface CompilerHost extends ModuleResolutionHost {
getSourceFile(fileName: string, languageVersion: ScriptTarget, onError?: (message: string) => void): SourceFile;
getCancellationToken?(): CancellationToken;
Expand All @@ -2291,7 +2300,7 @@ namespace ts {
* If resolveModuleNames is implemented then implementation for members from ModuleResolutionHost can be just
* 'throw new Error("NotImplemented")'
*/
resolveModuleNames?(moduleNames: string[], containingFile: string): string[];
resolveModuleNames?(moduleNames: string[], containingFile: string): ResolvedModule[];
}

export interface TextSpan {
Expand Down
10 changes: 5 additions & 5 deletions src/compiler/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -99,20 +99,20 @@ namespace ts {
return true;
}

export function hasResolvedModuleName(sourceFile: SourceFile, moduleNameText: string): boolean {
export function hasResolvedModule(sourceFile: SourceFile, moduleNameText: string): boolean {
return sourceFile.resolvedModules && hasProperty(sourceFile.resolvedModules, moduleNameText);
}

export function getResolvedModuleFileName(sourceFile: SourceFile, moduleNameText: string): string {
return hasResolvedModuleName(sourceFile, moduleNameText) ? sourceFile.resolvedModules[moduleNameText] : undefined;
export function getResolvedModule(sourceFile: SourceFile, moduleNameText: string): ResolvedModule {
return hasResolvedModule(sourceFile, moduleNameText) ? sourceFile.resolvedModules[moduleNameText] : undefined;
}

export function setResolvedModuleName(sourceFile: SourceFile, moduleNameText: string, resolvedFileName: string): void {
export function setResolvedModule(sourceFile: SourceFile, moduleNameText: string, resolvedModule: ResolvedModule): void {
if (!sourceFile.resolvedModules) {
sourceFile.resolvedModules = {};
}

sourceFile.resolvedModules[moduleNameText] = resolvedFileName;
sourceFile.resolvedModules[moduleNameText] = resolvedModule;
}

// Returns true if this node contains a parse error anywhere underneath it.
Expand Down
4 changes: 2 additions & 2 deletions src/harness/harnessLanguageService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -225,8 +225,8 @@ module Harness.LanguageService {
let imports: ts.Map<string> = {};
for (let module of preprocessInfo.importedFiles) {
let resolutionInfo = ts.resolveModuleName(module.fileName, fileName, compilerOptions, moduleResolutionHost);
if (resolutionInfo.resolvedFileName) {
imports[module.fileName] = resolutionInfo.resolvedFileName;
if (resolutionInfo.resolvedModule) {
imports[module.fileName] = resolutionInfo.resolvedModule.resolvedFileName;
}
}
return JSON.stringify(imports);
Expand Down
Loading