From 8ace7b826f3bf53b503d5cb870befc1a4deca69b Mon Sep 17 00:00:00 2001 From: Andy Date: Mon, 5 Jun 2017 14:23:39 -0700 Subject: [PATCH] importFixes: Support missing "React" at a JSXOpeningElement (#16066) * importFixes: Support missing "React" at a JSXOpeningElement * Fix nextLineRule linting * Rename to resolveJsxNamespaceAtLocation * Expose getJsxNamespace and resolveNameAtLocation separately --- scripts/tslint/nextLineRule.ts | 4 +- src/compiler/checker.ts | 12 ++++-- src/compiler/program.ts | 3 +- src/compiler/types.ts | 3 ++ src/harness/fourslash.ts | 5 ++- src/harness/unittests/telemetry.ts | 3 +- src/services/codefixes/importFixes.ts | 38 +++++++++++++------ src/services/findAllReferences.ts | 3 +- src/services/importTracker.ts | 3 +- .../importNameCodeFixUMDGlobalReact0.ts | 28 ++++++++++++++ .../importNameCodeFixUMDGlobalReact1.ts | 28 ++++++++++++++ .../importNameCodeFixUMDGlobalReact2.ts | 21 ++++++++++ 12 files changed, 130 insertions(+), 21 deletions(-) create mode 100644 tests/cases/fourslash/importNameCodeFixUMDGlobalReact0.ts create mode 100644 tests/cases/fourslash/importNameCodeFixUMDGlobalReact1.ts create mode 100644 tests/cases/fourslash/importNameCodeFixUMDGlobalReact2.ts diff --git a/scripts/tslint/nextLineRule.ts b/scripts/tslint/nextLineRule.ts index b149d09eb38a8..fa03746afc6b9 100644 --- a/scripts/tslint/nextLineRule.ts +++ b/scripts/tslint/nextLineRule.ts @@ -18,7 +18,7 @@ export class Rule extends Lint.Rules.AbstractRule { function walk(ctx: Lint.WalkContext, checkCatch: boolean, checkElse: boolean): void { const { sourceFile } = ctx; - function recur(node: ts.Node): void { + ts.forEachChild(sourceFile, function recur(node) { switch (node.kind) { case ts.SyntaxKind.IfStatement: checkIf(node as ts.IfStatement); @@ -28,7 +28,7 @@ function walk(ctx: Lint.WalkContext, checkCatch: boolean, checkElse: boole break; } ts.forEachChild(node, recur); - } + }); function checkIf(node: ts.IfStatement): void { const { thenStatement, elseStatement } = node; diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index 1cc91550dc031..af876e3246828 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -210,6 +210,11 @@ namespace ts { getSuggestionForNonexistentProperty, getSuggestionForNonexistentSymbol, getBaseConstraintOfType, + getJsxNamespace, + resolveNameAtLocation(location: Node, name: string, meaning: SymbolFlags): Symbol | undefined { + location = getParseTreeNode(location); + return resolveName(location, name, meaning, /*nameNotFoundMessage*/ undefined, name); + }, }; const tupleTypes: GenericType[] = []; @@ -847,7 +852,7 @@ namespace ts { location: Node | undefined, name: string, meaning: SymbolFlags, - nameNotFoundMessage: DiagnosticMessage, + nameNotFoundMessage: DiagnosticMessage | undefined, nameArg: string | Identifier, suggestedNameNotFoundMessage?: DiagnosticMessage): Symbol { return resolveNameHelper(location, name, meaning, nameNotFoundMessage, nameArg, getSymbol, suggestedNameNotFoundMessage); @@ -5841,7 +5846,8 @@ namespace ts { } } return arrayFrom(props.values()); - } else { + } + else { return getPropertiesOfType(type); } } @@ -14151,7 +14157,7 @@ namespace ts { checkJsxPreconditions(node); // The reactNamespace/jsxFactory's root symbol should be marked as 'used' so we don't incorrectly elide its import. // And if there is no reactNamespace/jsxFactory's symbol in scope when targeting React emit, we should issue an error. - const reactRefErr = compilerOptions.jsx === JsxEmit.React ? Diagnostics.Cannot_find_name_0 : undefined; + const reactRefErr = diagnostics && compilerOptions.jsx === JsxEmit.React ? Diagnostics.Cannot_find_name_0 : undefined; const reactNamespace = getJsxNamespace(); const reactSym = resolveName(node.tagName, reactNamespace, SymbolFlags.Value, reactRefErr, reactNamespace); if (reactSym) { diff --git a/src/compiler/program.ts b/src/compiler/program.ts index 00df31ed5ab52..39d82b8606d7e 100644 --- a/src/compiler/program.ts +++ b/src/compiler/program.ts @@ -1479,7 +1479,8 @@ namespace ts { } } return sourceFile; - } else { + } + else { const sourceFileNoExtension = options.allowNonTsExtensions && getSourceFile(fileName); if (sourceFileNoExtension) return sourceFileNoExtension; diff --git a/src/compiler/types.ts b/src/compiler/types.ts index ed1d5dcbe2313..83aa50fb803e8 100644 --- a/src/compiler/types.ts +++ b/src/compiler/types.ts @@ -2605,6 +2605,9 @@ namespace ts { * Does not include properties of primitive types. */ /* @internal */ getAllPossiblePropertiesOfType(type: Type): Symbol[]; + + /* @internal */ getJsxNamespace(): string; + /* @internal */ resolveNameAtLocation(location: Node, name: string, meaning: SymbolFlags): Symbol | undefined; } export enum NodeBuilderFlags { diff --git a/src/harness/fourslash.ts b/src/harness/fourslash.ts index 4045c738aa035..4b5af197a3690 100644 --- a/src/harness/fourslash.ts +++ b/src/harness/fourslash.ts @@ -2372,7 +2372,10 @@ namespace FourSlash { const codeFixes = this.getCodeFixActions(this.activeFile.fileName, errorCode); if (!codeFixes || codeFixes.length === 0) { - this.raiseError("No codefixes returned."); + if (expectedTextArray.length !== 0) { + this.raiseError("No codefixes returned."); + } + return; } const actualTextArray: string[] = []; diff --git a/src/harness/unittests/telemetry.ts b/src/harness/unittests/telemetry.ts index d3811edf25196..f250c732c0b71 100644 --- a/src/harness/unittests/telemetry.ts +++ b/src/harness/unittests/telemetry.ts @@ -252,7 +252,8 @@ namespace ts.projectSystem { } getEvent(eventName: T["eventName"], mayBeMore = false): T["data"] { - if (mayBeMore) assert(this.events.length !== 0); else assert.equal(this.events.length, 1); + if (mayBeMore) { assert(this.events.length !== 0); } + else { assert.equal(this.events.length, 1); } const event = this.events.shift(); assert.equal(event.eventName, eventName); return event.data; diff --git a/src/services/codefixes/importFixes.ts b/src/services/codefixes/importFixes.ts index 53ec99fac81a4..c2d26da4392d1 100644 --- a/src/services/codefixes/importFixes.ts +++ b/src/services/codefixes/importFixes.ts @@ -138,8 +138,23 @@ namespace ts.codefix { const currentTokenMeaning = getMeaningFromLocation(token); if (context.errorCode === Diagnostics._0_refers_to_a_UMD_global_but_the_current_file_is_a_module_Consider_adding_an_import_instead.code) { - const symbol = checker.getAliasedSymbol(checker.getSymbolAtLocation(token)); - return getCodeActionForImport(symbol, /*isDefault*/ false, /*isNamespaceImport*/ true); + const umdSymbol = checker.getSymbolAtLocation(token); + let symbol: ts.Symbol; + let symbolName: string; + if (umdSymbol.flags & ts.SymbolFlags.Alias) { + symbol = checker.getAliasedSymbol(umdSymbol); + symbolName = name; + } + else if (isJsxOpeningLikeElement(token.parent) && token.parent.tagName === token) { + // The error wasn't for the symbolAtLocation, it was for the JSX tag itself, which needs access to e.g. `React`. + symbol = checker.getAliasedSymbol(checker.resolveNameAtLocation(token, checker.getJsxNamespace(), SymbolFlags.Value)); + symbolName = symbol.name; + } + else { + Debug.fail("Either the symbol or the JSX namespace should be a UMD global if we got here"); + } + + return getCodeActionForImport(symbol, symbolName, /*isDefault*/ false, /*isNamespaceImport*/ true); } const candidateModules = checker.getAmbientModules(); @@ -159,7 +174,7 @@ namespace ts.codefix { if (localSymbol && localSymbol.name === name && checkSymbolHasMeaning(localSymbol, currentTokenMeaning)) { // check if this symbol is already used const symbolId = getUniqueSymbolId(localSymbol); - symbolIdActionMap.addActions(symbolId, getCodeActionForImport(moduleSymbol, /*isDefault*/ true)); + symbolIdActionMap.addActions(symbolId, getCodeActionForImport(moduleSymbol, name, /*isDefault*/ true)); } } @@ -167,7 +182,7 @@ namespace ts.codefix { const exportSymbolWithIdenticalName = checker.tryGetMemberInModuleExports(name, moduleSymbol); if (exportSymbolWithIdenticalName && checkSymbolHasMeaning(exportSymbolWithIdenticalName, currentTokenMeaning)) { const symbolId = getUniqueSymbolId(exportSymbolWithIdenticalName); - symbolIdActionMap.addActions(symbolId, getCodeActionForImport(moduleSymbol)); + symbolIdActionMap.addActions(symbolId, getCodeActionForImport(moduleSymbol, name)); } } @@ -218,7 +233,7 @@ namespace ts.codefix { return declarations ? some(symbol.declarations, decl => !!(getMeaningFromDeclaration(decl) & meaning)) : false; } - function getCodeActionForImport(moduleSymbol: Symbol, isDefault?: boolean, isNamespaceImport?: boolean): ImportCodeAction[] { + function getCodeActionForImport(moduleSymbol: Symbol, symbolName: string, isDefault?: boolean, isNamespaceImport?: boolean): ImportCodeAction[] { const existingDeclarations = getImportDeclarations(moduleSymbol); if (existingDeclarations.length > 0) { // With an existing import statement, there are more than one actions the user can do. @@ -375,10 +390,10 @@ namespace ts.codefix { const moduleSpecifierWithoutQuotes = stripQuotes(moduleSpecifier || getModuleSpecifierForNewImport()); const changeTracker = createChangeTracker(); const importClause = isDefault - ? createImportClause(createIdentifier(name), /*namedBindings*/ undefined) + ? createImportClause(createIdentifier(symbolName), /*namedBindings*/ undefined) : isNamespaceImport - ? createImportClause(/*name*/ undefined, createNamespaceImport(createIdentifier(name))) - : createImportClause(/*name*/ undefined, createNamedImports([createImportSpecifier(/*propertyName*/ undefined, createIdentifier(name))])); + ? createImportClause(/*name*/ undefined, createNamespaceImport(createIdentifier(symbolName))) + : createImportClause(/*name*/ undefined, createNamedImports([createImportSpecifier(/*propertyName*/ undefined, createIdentifier(symbolName))])); const importDecl = createImportDeclaration(/*decorators*/ undefined, /*modifiers*/ undefined, importClause, createLiteral(moduleSpecifierWithoutQuotes)); if (!lastImportDeclaration) { changeTracker.insertNodeAt(sourceFile, sourceFile.getStart(), importDecl, { suffix: `${context.newLineCharacter}${context.newLineCharacter}` }); @@ -392,7 +407,7 @@ namespace ts.codefix { // are there are already a new line seperating code and import statements. return createCodeAction( Diagnostics.Import_0_from_1, - [name, `"${moduleSpecifierWithoutQuotes}"`], + [symbolName, `"${moduleSpecifierWithoutQuotes}"`], changeTracker.getChanges(), "NewImport", moduleSpecifierWithoutQuotes @@ -412,8 +427,9 @@ namespace ts.codefix { removeFileExtension(getRelativePath(moduleFileName, sourceDirectory)); function tryGetModuleNameFromAmbientModule(): string { - if (moduleSymbol.valueDeclaration.kind !== SyntaxKind.SourceFile) { - return moduleSymbol.name; + const decl = moduleSymbol.valueDeclaration; + if (isModuleDeclaration(decl) && isStringLiteral(decl.name)) { + return decl.name.text; } } diff --git a/src/services/findAllReferences.ts b/src/services/findAllReferences.ts index df499cbd38a42..c1a7408fae710 100644 --- a/src/services/findAllReferences.ts +++ b/src/services/findAllReferences.ts @@ -185,7 +185,8 @@ namespace ts.FindAllReferences { if (entry.type === "node") { const { node } = entry; return { textSpan: getTextSpan(node), fileName: node.getSourceFile().fileName, ...implementationKindDisplayParts(node, checker) }; - } else { + } + else { const { textSpan, fileName } = entry; return { textSpan, fileName, kind: ScriptElementKind.unknown, displayParts: [] }; } diff --git a/src/services/importTracker.ts b/src/services/importTracker.ts index f20c80c14ef96..585af7ebd31aa 100644 --- a/src/services/importTracker.ts +++ b/src/services/importTracker.ts @@ -530,7 +530,8 @@ namespace ts.FindAllReferences { if (parent.kind === SyntaxKind.VariableDeclaration) { const p = parent as ts.VariableDeclaration; return p.parent.kind === ts.SyntaxKind.CatchClause ? undefined : p.parent.parent.kind === SyntaxKind.VariableStatement ? p.parent.parent : undefined; - } else { + } + else { return parent; } } diff --git a/tests/cases/fourslash/importNameCodeFixUMDGlobalReact0.ts b/tests/cases/fourslash/importNameCodeFixUMDGlobalReact0.ts new file mode 100644 index 0000000000000..260c34d10a604 --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFixUMDGlobalReact0.ts @@ -0,0 +1,28 @@ +/// + +// @jsx: react + +// @Filename: /node_modules/@types/react/index.d.ts +////export = React; +////export as namespace React; +////declare namespace React { +//// export class Component { render(): JSX.Element | null; } +////} +////declare global { +//// namespace JSX { +//// interface Element {} +//// } +////} + +// @Filename: /a.tsx +////[|import { Component } from "react"; +////export class MyMap extends Component { } +////;|] + +goTo.file("/a.tsx"); + +verify.importFixAtPosition([ +`import { Component } from "react"; +import * as React from "react"; +export class MyMap extends Component { } +;`]); diff --git a/tests/cases/fourslash/importNameCodeFixUMDGlobalReact1.ts b/tests/cases/fourslash/importNameCodeFixUMDGlobalReact1.ts new file mode 100644 index 0000000000000..ccd69c501997a --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFixUMDGlobalReact1.ts @@ -0,0 +1,28 @@ +/// + +// @jsx: react + +// @Filename: /node_modules/@types/react/index.d.ts +////export = React; +////export as namespace React; +////declare namespace React { +//// export class Component { render(): JSX.Element | null; } +////} +////declare global { +//// namespace JSX { +//// interface Element {} +//// } +////} + +// @Filename: /a.tsx +////[|import { Component } from "react"; +////export class MyMap extends Component { } +////;|] + +goTo.file("/a.tsx"); + +verify.importFixAtPosition([ +`import { Component } from "react"; +import * as React from "react"; +export class MyMap extends Component { } +;`]); diff --git a/tests/cases/fourslash/importNameCodeFixUMDGlobalReact2.ts b/tests/cases/fourslash/importNameCodeFixUMDGlobalReact2.ts new file mode 100644 index 0000000000000..a3c8253fbd1a7 --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFixUMDGlobalReact2.ts @@ -0,0 +1,21 @@ +/// + +// https://github.com/Microsoft/TypeScript/issues/16065 + +// @jsx: react +// @jsxFactory: factory + +// @Filename: /factory.ts +////export function factory() { return {}; } +////declare global { +//// namespace JSX { +//// interface Element {} +//// } +////} + +// @Filename: /a.tsx +////[|
|] + +goTo.file("/a.tsx"); +verify.not +verify.importFixAtPosition([]);