From 7aeb92f06fb96b85366aafdc92c7cad7de22faa8 Mon Sep 17 00:00:00 2001 From: Andrew Branch Date: Mon, 7 Mar 2022 17:24:20 -0800 Subject: [PATCH] Fix some go-to-def stuff --- src/compiler/checker.ts | 2 +- src/server/session.ts | 6 +- src/services/goToDefinition.ts | 100 +++++++++++------- src/services/types.ts | 1 + .../goToDefinitionExpandoElementAccess.ts | 2 +- .../goToSource7_conditionallyMinified.ts | 33 ++++++ 6 files changed, 101 insertions(+), 43 deletions(-) create mode 100644 tests/cases/fourslash/server/goToSource7_conditionallyMinified.ts diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index cb52a13176a..fcdac1191ab 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -9003,7 +9003,7 @@ namespace ts { } } const sourceTypes = some(constructorTypes, t => !!(t.flags & ~TypeFlags.Nullable)) ? constructorTypes : types; // TODO: GH#18217 - type = getUnionType(sourceTypes!, UnionReduction.Subtype); + type = getUnionType(sourceTypes!); } } const widened = getWidenedType(addOptionality(type, /*isProperty*/ false, definedInMethod && !definedInConstructor)); diff --git a/src/server/session.ts b/src/server/session.ts index 81308329e64..708221835a4 100644 --- a/src/server/session.ts +++ b/src/server/session.ts @@ -1233,6 +1233,8 @@ namespace ts.server { containerName: info.containerName, kind: info.kind, name: info.name, + isAliasTarget: info.isAliasTarget, + failedAliasResolution: info.failedAliasResolution, ...info.unverified && { unverified: info.unverified }, }; }); @@ -1283,7 +1285,7 @@ namespace ts.server { } let definitions = this.mapDefinitionInfoLocations(unmappedDefinitionAndBoundSpan.definitions, project).slice(); - const needsJsResolution = every(definitions.filter(d => d.isAliasTarget), d => !!d.isAmbient); + const needsJsResolution = every(definitions.filter(d => d.isAliasTarget), d => !!d.isAmbient) || some(definitions, d => !!d.failedAliasResolution); if (needsJsResolution) { project.withAuxiliaryProjectForFiles([file], auxiliaryProject => { const jsDefinitions = auxiliaryProject.getLanguageService().getDefinitionAndBoundSpan(file, position); @@ -1293,7 +1295,7 @@ namespace ts.server { }); } - definitions = definitions.filter(d => !d.isAmbient); + definitions = definitions.filter(d => !d.isAmbient && !d.failedAliasResolution); const { textSpan } = unmappedDefinitionAndBoundSpan; if (simplifiedResult) { diff --git a/src/services/goToDefinition.ts b/src/services/goToDefinition.ts index b4513fe6b75..dc6ed645d63 100644 --- a/src/services/goToDefinition.ts +++ b/src/services/goToDefinition.ts @@ -28,7 +28,7 @@ namespace ts.GoToDefinition { if (isStaticModifier(node) && isClassStaticBlockDeclaration(node.parent)) { const classDecl = node.parent.parent; - const { symbol, isAliasTarget } = getSymbol(classDecl, typeChecker); + const { symbol, isAliasTarget, failedAliasResolution } = getSymbol(classDecl, typeChecker); if (aliasesOnly && !isAliasTarget) return undefined; const staticBlocks = filter(classDecl.members, isClassStaticBlockDeclaration); @@ -37,11 +37,11 @@ namespace ts.GoToDefinition { return map(staticBlocks, staticBlock => { let { pos } = moveRangePastModifiers(staticBlock); pos = skipTrivia(sourceFile.text, pos); - return createDefinitionInfoFromName(typeChecker, staticBlock, ScriptElementKind.constructorImplementationElement, "static {}", containerName, isAliasTarget, { start: pos, length: "static".length }); + return createDefinitionInfoFromName(typeChecker, staticBlock, ScriptElementKind.constructorImplementationElement, "static {}", containerName, isAliasTarget, failedAliasResolution, { start: pos, length: "static".length }); }); } - const { symbol, isAliasTarget } = getSymbol(node, typeChecker); + const { symbol, isAliasTarget, failedAliasResolution } = getSymbol(node, typeChecker); if (!symbol && isModuleSpecifierLike(node)) { // We couldn't resolve the symbol as an external module, but it could // that module resolution succeeded but the target was not a module. @@ -55,6 +55,7 @@ namespace ts.GoToDefinition { kind: ScriptElementKind.scriptElement, textSpan: createTextSpan(0, 0), isAliasTarget: true, + failedAliasResolution, isAmbient: isDeclarationFileName(ref.resolvedFileName), }]; } @@ -71,14 +72,14 @@ namespace ts.GoToDefinition { const calledDeclaration = tryGetSignatureDeclaration(typeChecker, node); // Don't go to the component constructor definition for a JSX element, just go to the component definition. if (calledDeclaration && !(isJsxOpeningLikeElement(node.parent) && isConstructorLike(calledDeclaration))) { - const sigInfo = createDefinitionFromSignatureDeclaration(typeChecker, calledDeclaration, isAliasTarget); + const sigInfo = createDefinitionFromSignatureDeclaration(typeChecker, calledDeclaration, isAliasTarget, failedAliasResolution); // For a function, if this is the original function definition, return just sigInfo. // If this is the original constructor definition, parent is the class. if (typeChecker.getRootSymbols(symbol).some(s => symbolMatchesSignature(s, calledDeclaration))) { return [sigInfo]; } else { - const defs = getDefinitionFromSymbol(typeChecker, symbol, node, isAliasTarget, calledDeclaration) || emptyArray; + const defs = getDefinitionFromSymbol(typeChecker, symbol, node, isAliasTarget, failedAliasResolution, calledDeclaration) || emptyArray; // For a 'super()' call, put the signature first, else put the variable first. return node.kind === SyntaxKind.SuperKeyword ? [sigInfo, ...defs] : [...defs, sigInfo]; } @@ -91,7 +92,7 @@ namespace ts.GoToDefinition { // assignment. This case and others are handled by the following code. if (node.parent.kind === SyntaxKind.ShorthandPropertyAssignment) { const shorthandSymbol = typeChecker.getShorthandAssignmentValueSymbol(symbol.valueDeclaration); - const definitions = shorthandSymbol?.declarations ? shorthandSymbol.declarations.map(decl => createDefinitionInfo(decl, typeChecker, shorthandSymbol, node, isAliasTarget)) : emptyArray; + const definitions = shorthandSymbol?.declarations ? shorthandSymbol.declarations.map(decl => createDefinitionInfo(decl, typeChecker, shorthandSymbol, node, isAliasTarget, failedAliasResolution)) : emptyArray; return concatenate(definitions, getDefinitionFromObjectLiteralElement(typeChecker, node) || emptyArray); } @@ -116,7 +117,7 @@ namespace ts.GoToDefinition { }); } - return concatenate(fileReferenceDefinition, getDefinitionFromObjectLiteralElement(typeChecker, node) || getDefinitionFromSymbol(typeChecker, symbol, node, isAliasTarget)); + return concatenate(fileReferenceDefinition, getDefinitionFromObjectLiteralElement(typeChecker, node) || getDefinitionFromSymbol(typeChecker, symbol, node, isAliasTarget, failedAliasResolution)); } /** @@ -219,22 +220,22 @@ namespace ts.GoToDefinition { return undefined; } - const { symbol, isAliasTarget } = getSymbol(node, typeChecker); + const { symbol, isAliasTarget, failedAliasResolution } = getSymbol(node, typeChecker); if (!symbol) return undefined; const typeAtLocation = typeChecker.getTypeOfSymbolAtLocation(symbol, node); const returnType = tryGetReturnTypeOfFunction(symbol, typeAtLocation, typeChecker); - const fromReturnType = returnType && definitionFromType(returnType, typeChecker, node, isAliasTarget); + const fromReturnType = returnType && definitionFromType(returnType, typeChecker, node, isAliasTarget, failedAliasResolution); // If a function returns 'void' or some other type with no definition, just return the function definition. - const typeDefinitions = fromReturnType && fromReturnType.length !== 0 ? fromReturnType : definitionFromType(typeAtLocation, typeChecker, node, isAliasTarget); + const typeDefinitions = fromReturnType && fromReturnType.length !== 0 ? fromReturnType : definitionFromType(typeAtLocation, typeChecker, node, isAliasTarget, failedAliasResolution); return typeDefinitions.length ? typeDefinitions - : !(symbol.flags & SymbolFlags.Value) && symbol.flags & SymbolFlags.Type ? getDefinitionFromSymbol(typeChecker, skipAlias(symbol, typeChecker), node, isAliasTarget) + : !(symbol.flags & SymbolFlags.Value) && symbol.flags & SymbolFlags.Type ? getDefinitionFromSymbol(typeChecker, skipAlias(symbol, typeChecker), node, isAliasTarget, failedAliasResolution) : undefined; } - function definitionFromType(type: Type, checker: TypeChecker, node: Node, isAliasTarget: boolean): readonly DefinitionInfo[] { + function definitionFromType(type: Type, checker: TypeChecker, node: Node, isAliasTarget: boolean, failedAliasResolution: boolean | undefined): readonly DefinitionInfo[] { return flatMap(type.isUnion() && !(type.flags & TypeFlags.Enum) ? type.types : [type], t => - t.symbol && getDefinitionFromSymbol(checker, t.symbol, node, isAliasTarget)); + t.symbol && getDefinitionFromSymbol(checker, t.symbol, node, isAliasTarget, failedAliasResolution)); } function tryGetReturnTypeOfFunction(symbol: Symbol, type: Type, checker: TypeChecker): Type | undefined { @@ -282,13 +283,17 @@ namespace ts.GoToDefinition { // get the aliased symbol instead. This allows for goto def on an import e.g. // import {A, B} from "mod"; // to jump to the implementation directly. + let failedAliasResolution = false; if (symbol?.declarations && symbol.flags & SymbolFlags.Alias && shouldSkipAlias(node, symbol.declarations[0])) { const aliased = checker.getAliasedSymbol(symbol); if (aliased.declarations) { return { symbol: aliased, isAliasTarget: true }; } + else { + failedAliasResolution = true; + } } - return { symbol, isAliasTarget: !!(symbol && isExternalModuleSymbol(symbol)) }; + return { symbol, isAliasTarget: !!(symbol && isExternalModuleSymbol(symbol)), failedAliasResolution }; } // Go to the original declaration for cases: @@ -303,28 +308,44 @@ namespace ts.GoToDefinition { if (node.parent === declaration) { return true; } - switch (declaration.kind) { - case SyntaxKind.ImportClause: - case SyntaxKind.ImportEqualsDeclaration: - return true; - case SyntaxKind.ImportSpecifier: - return declaration.parent.kind === SyntaxKind.NamedImports; - case SyntaxKind.BindingElement: - case SyntaxKind.VariableDeclaration: - return isInJSFile(declaration) && isVariableDeclarationInitializedToBareOrAccessedRequire(declaration); - default: - return false; + if (declaration.kind === SyntaxKind.NamespaceImport) { + return false; } + return true; } - function getDefinitionFromSymbol(typeChecker: TypeChecker, symbol: Symbol, node: Node, isAliasTarget?: boolean, declarationNode?: Node): DefinitionInfo[] | undefined { - // There are cases when you extend a function by adding properties to it afterwards, - // we want to strip those extra properties. - // For deduping purposes, we also want to exclude any declarationNodes if provided. - const filteredDeclarations = - filter(symbol.declarations, d => d !== declarationNode && (!isAssignmentDeclaration(d) || d === symbol.valueDeclaration)) - || undefined; - return getConstructSignatureDefinition() || getCallSignatureDefinition() || map(filteredDeclarations, declaration => createDefinitionInfo(declaration, typeChecker, symbol, node, isAliasTarget)); + /** + * ```ts + * function f() {} + * f.foo = 0; + * ``` + * + * Here, `f` has two declarations: the function declaration, and the identifier in the next line. + * The latter is a declaration for `f` because it gives `f` the `SymbolFlags.Namespace` meaning so + * it can contain `foo`. However, that declaration is pretty uninteresting and not intuitively a + * "definition" for `f`. Ideally, the question we'd like to answer is "what SymbolFlags does this + * declaration contribute to the symbol for `f`?" If the answer is just `Namespace` and the + * declaration looks like an assignment, that declaration is in no sense a definition for `f`. + * But that information is totally lost during binding and/or symbol merging, so we need to do + * our best to reconstruct it or use other heuristics. This function (and the logic around its + * calling) covers our tests but feels like a hack, and it would be great if someone could come + * up with a more precise definition of what counts as a definition. + */ + function isExpandoDeclaration(node: Declaration): boolean { + if (!isAssignmentDeclaration(node)) return false; + const containingAssignment = findAncestor(node, p => { + if (isAssignmentExpression(p)) return true; + if (!isAssignmentDeclaration(p as Declaration)) return "quit"; + return false; + }) as AssignmentExpression | undefined; + return !!containingAssignment && getAssignmentDeclarationKind(containingAssignment) === AssignmentDeclarationKind.Property; + } + + function getDefinitionFromSymbol(typeChecker: TypeChecker, symbol: Symbol, node: Node, isAliasTarget?: boolean, failedAliasResolution?: boolean, excludeDeclaration?: Node): DefinitionInfo[] | undefined { + const filteredDeclarations = filter(symbol.declarations, d => d !== excludeDeclaration); + const withoutExpandos = filter(filteredDeclarations, d => !isExpandoDeclaration(d)); + const results = some(withoutExpandos) ? withoutExpandos : filteredDeclarations; + return getConstructSignatureDefinition() || getCallSignatureDefinition() || map(results, declaration => createDefinitionInfo(declaration, typeChecker, symbol, node, isAliasTarget, failedAliasResolution)); function getConstructSignatureDefinition(): DefinitionInfo[] | undefined { // Applicable only if we are in a new expression, or we are on a constructor declaration @@ -352,21 +373,21 @@ namespace ts.GoToDefinition { return declarations.length ? declarationsWithBody.length !== 0 ? declarationsWithBody.map(x => createDefinitionInfo(x, typeChecker, symbol, node)) - : [createDefinitionInfo(last(declarations), typeChecker, symbol, node, isAliasTarget)] + : [createDefinitionInfo(last(declarations), typeChecker, symbol, node, isAliasTarget, failedAliasResolution)] : undefined; } } /** Creates a DefinitionInfo from a Declaration, using the declaration's name if possible. */ - function createDefinitionInfo(declaration: Declaration, checker: TypeChecker, symbol: Symbol, node: Node, isAliasTarget?: boolean): DefinitionInfo { + function createDefinitionInfo(declaration: Declaration, checker: TypeChecker, symbol: Symbol, node: Node, isAliasTarget?: boolean, failedAliasResolution?: boolean): DefinitionInfo { const symbolName = checker.symbolToString(symbol); // Do not get scoped name, just the name of the symbol const symbolKind = SymbolDisplay.getSymbolKind(checker, symbol, node); const containerName = symbol.parent ? checker.symbolToString(symbol.parent, node) : ""; - return createDefinitionInfoFromName(checker, declaration, symbolKind, symbolName, containerName, isAliasTarget); + return createDefinitionInfoFromName(checker, declaration, symbolKind, symbolName, containerName, isAliasTarget, failedAliasResolution); } /** Creates a DefinitionInfo directly from the name of a declaration. */ - function createDefinitionInfoFromName(checker: TypeChecker, declaration: Declaration, symbolKind: ScriptElementKind, symbolName: string, containerName: string, isAliasTarget?: boolean, textSpan?: TextSpan): DefinitionInfo { + function createDefinitionInfoFromName(checker: TypeChecker, declaration: Declaration, symbolKind: ScriptElementKind, symbolName: string, containerName: string, isAliasTarget?: boolean, failedAliasResolution?: boolean, textSpan?: TextSpan): DefinitionInfo { const sourceFile = declaration.getSourceFile(); if (!textSpan) { const name = getNameOfDeclaration(declaration) || declaration; @@ -387,6 +408,7 @@ namespace ts.GoToDefinition { isLocal: !isDefinitionVisible(checker, declaration), isAmbient: !!(declaration.flags & NodeFlags.Ambient), isAliasTarget, + failedAliasResolution, }; } @@ -421,8 +443,8 @@ namespace ts.GoToDefinition { } } - function createDefinitionFromSignatureDeclaration(typeChecker: TypeChecker, decl: SignatureDeclaration, isAliasTarget?: boolean): DefinitionInfo { - return createDefinitionInfo(decl, typeChecker, decl.symbol, decl, isAliasTarget); + function createDefinitionFromSignatureDeclaration(typeChecker: TypeChecker, decl: SignatureDeclaration, isAliasTarget?: boolean, failedAliasResolution?: boolean): DefinitionInfo { + return createDefinitionInfo(decl, typeChecker, decl.symbol, decl, isAliasTarget, failedAliasResolution); } export function findReferenceInPosition(refs: readonly FileReference[], pos: number): FileReference | undefined { diff --git a/src/services/types.ts b/src/services/types.ts index fed54f2f785..89cc2d156d6 100644 --- a/src/services/types.ts +++ b/src/services/types.ts @@ -1033,6 +1033,7 @@ namespace ts { /* @internal */ isLocal?: boolean; /* @internal */ isAmbient?: boolean; /* @internal */ isAliasTarget?: boolean; + /* @internal */ failedAliasResolution?: boolean; } export interface DefinitionInfoAndBoundSpan { diff --git a/tests/cases/fourslash/goToDefinitionExpandoElementAccess.ts b/tests/cases/fourslash/goToDefinitionExpandoElementAccess.ts index 5deeb4c0253..67efdb195e6 100644 --- a/tests/cases/fourslash/goToDefinitionExpandoElementAccess.ts +++ b/tests/cases/fourslash/goToDefinitionExpandoElementAccess.ts @@ -4,4 +4,4 @@ ////f[/*0*/"x"] = 0; ////f[[|/*1*/"x"|]] = 1; -verify.goToDefinition("1", "0"); +verify.goToDefinition("1", ["0", "1"]); diff --git a/tests/cases/fourslash/server/goToSource7_conditionallyMinified.ts b/tests/cases/fourslash/server/goToSource7_conditionallyMinified.ts new file mode 100644 index 00000000000..6a0e13962ba --- /dev/null +++ b/tests/cases/fourslash/server/goToSource7_conditionallyMinified.ts @@ -0,0 +1,33 @@ +/// + +// @moduleResolution: node + +// @Filename: /node_modules/react/package.json +//// { "name": "react", "version": "16.8.6", "main": "index.js" } + +// @Filename: /node_modules/react/index.js +//// 'use strict'; +//// +//// if (process.env.NODE_ENV === 'production') { +//// module.exports = require('./cjs/react.production.min.js'); +//// } else { +//// module.exports = require('./cjs/react.development.js'); +//// } + +// @Filename: /node_modules/react/cjs/react.production.min.js +//// 'use strict';exports./*production*/useState=function(a){};exports.version='16.8.6'; + +// @Filename: /node_modules/react/cjs/react.development.js +//// 'use strict'; +//// if (process.env.NODE_ENV !== 'production') { +//// (function() { +//// function useState(initialState) {} +//// exports./*development*/useState = useState; +//// exports.version = '16.8.6'; +//// }()); +//// } + +// @Filename: /index.ts +//// import { [|/*start*/useState|] } from 'react'; + +verify.goToSourceDefinition("start", ["production", "development"]);