-
Notifications
You must be signed in to change notification settings - Fork 13k
Fix auto import crash on weird JS aliasing #46490
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3656,8 +3656,8 @@ namespace ts { | |
if (exportEquals !== moduleSymbol) { | ||
const type = getTypeOfSymbol(exportEquals); | ||
if (shouldTreatPropertiesOfExternalModuleAsExports(type)) { | ||
getPropertiesOfType(type).forEach(symbol => { | ||
cb(symbol, symbol.escapedName); | ||
forEachPropertyOfType(type, (symbol, escapedName) => { | ||
cb(symbol, escapedName); | ||
}); | ||
} | ||
} | ||
|
@@ -4033,13 +4033,17 @@ namespace ts { | |
function getNamedMembers(members: SymbolTable): Symbol[] { | ||
let result: Symbol[] | undefined; | ||
members.forEach((symbol, id) => { | ||
if (!isReservedMemberName(id) && symbolIsValue(symbol)) { | ||
if (isNamedMember(symbol, id)) { | ||
(result || (result = [])).push(symbol); | ||
} | ||
}); | ||
return result || emptyArray; | ||
} | ||
|
||
function isNamedMember(member: Symbol, escapedName: __String) { | ||
return !isReservedMemberName(escapedName) && symbolIsValue(member); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what does There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It just checks |
||
} | ||
|
||
function getNamedOrIndexSignatureMembers(members: SymbolTable): Symbol[] { | ||
const result = getNamedMembers(members); | ||
const index = getIndexSymbolFromSymbolTable(members); | ||
|
@@ -11571,6 +11575,17 @@ namespace ts { | |
getPropertiesOfObjectType(type); | ||
} | ||
|
||
function forEachPropertyOfType(type: Type, action: (symbol: Symbol, escapedName: __String) => void): void { | ||
type = getReducedApparentType(type); | ||
if (type.flags & TypeFlags.StructuredType) { | ||
resolveStructuredTypeMembers(type as StructuredType).members.forEach((symbol, escapedName) => { | ||
if (isNamedMember(symbol, escapedName)) { | ||
action(symbol, escapedName); | ||
} | ||
}); | ||
} | ||
} | ||
|
||
function isTypeInvalidDueToUnionDiscriminant(contextualType: Type, obj: ObjectLiteralExpression | JsxAttributes): boolean { | ||
const list = obj.properties as NodeArray<ObjectLiteralElementLike | JsxAttributeLike>; | ||
return list.some(property => { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -79,10 +79,14 @@ namespace ts { | |
} | ||
const isDefault = exportKind === ExportKind.Default; | ||
const namedSymbol = isDefault && getLocalSymbolForExportDefault(symbol) || symbol; | ||
// A re-export merged with an export from a module augmentation can result in `symbol` | ||
// being an external module symbol; the name it is re-exported by will be `symbolTableKey` | ||
// (which comes from the keys of `moduleSymbol.exports`.) | ||
const importedName = isExternalModuleSymbol(namedSymbol) | ||
// 1. A named export must be imported by its key in `moduleSymbol.exports` or `moduleSymbol.members`. | ||
// 2. A re-export merged with an export from a module augmentation can result in `symbol` | ||
// being an external module symbol; the name it is re-exported by will be `symbolTableKey` | ||
// (which comes from the keys of `moduleSymbol.exports`.) | ||
// 3. Otherwise, we have a default/namespace import that can be imported by any name, and | ||
// `symbolTableKey` will be something undesirable like `export=` or `default`, so we try to | ||
// get a better name. | ||
const importedName = exportKind === ExportKind.Named || isExternalModuleSymbol(namedSymbol) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is fine, but it feels a little backward compared to the usual approach of checking for special names first There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I probably agree—this is a refactor of a refactor of a refactor of... it has probably accumulated some unnecessary roundabout logic that could be simplified if rewritten fresh. |
||
? unescapeLeadingUnderscores(symbolTableKey) | ||
: getNameForExportedSymbol(namedSymbol, scriptTarget); | ||
const moduleName = stripQuotes(moduleSymbol.name); | ||
|
@@ -321,7 +325,7 @@ namespace ts { | |
let moduleCount = 0; | ||
forEachExternalModuleToImportFrom(program, host, /*useAutoImportProvider*/ true, (moduleSymbol, moduleFile, program, isFromPackageJson) => { | ||
if (++moduleCount % 100 === 0) cancellationToken?.throwIfCancellationRequested(); | ||
const seenExports = new Map<Symbol, true>(); | ||
const seenExports = new Map<__String, true>(); | ||
const checker = program.getTypeChecker(); | ||
const defaultInfo = getDefaultLikeExportInfo(moduleSymbol, checker, compilerOptions); | ||
// Note: I think we shouldn't actually see resolved module symbols here, but weird merges | ||
|
@@ -339,7 +343,7 @@ namespace ts { | |
checker); | ||
} | ||
checker.forEachExportAndPropertyOfModule(moduleSymbol, (exported, key) => { | ||
if (exported !== defaultInfo?.symbol && isImportableSymbol(exported, checker) && addToSeen(seenExports, exported)) { | ||
if (exported !== defaultInfo?.symbol && isImportableSymbol(exported, checker) && addToSeen(seenExports, key)) { | ||
cache.add( | ||
importingFile.path, | ||
exported, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
/// <reference path="../fourslash.ts" /> | ||
|
||
// @Filename: /tsconfig.json | ||
//// { "compilerOptions": { "module": "commonjs", "allowJs": true } } | ||
|
||
// @Filename: /third_party/marked/src/defaults.js | ||
//// function getDefaults() { | ||
//// return { | ||
//// baseUrl: null, | ||
//// }; | ||
//// } | ||
//// | ||
//// function changeDefaults(newDefaults) { | ||
//// module.exports.defaults = newDefaults; | ||
//// } | ||
//// | ||
//// module.exports = { | ||
//// defaults: getDefaults(), | ||
//// getDefaults, | ||
//// changeDefaults | ||
//// }; | ||
|
||
// @Filename: /index.ts | ||
//// /**/ | ||
|
||
format.setOption("newLineCharacter", "\n") | ||
goTo.marker(""); | ||
|
||
// Create the exportInfoMap | ||
verify.completions({ marker: "", preferences: { includeCompletionsForModuleExports: true } }); | ||
|
||
// Create a new program and reuse the exportInfoMap from the last request | ||
edit.insert("d"); | ||
verify.completions({ | ||
marker: "", | ||
excludes: ["newDefaults"], | ||
includes: [{ | ||
name: "defaults", | ||
source: "/third_party/marked/src/defaults", | ||
hasAction: true, | ||
sortText: completion.SortText.AutoImportSuggestions, | ||
}], | ||
preferences: { includeCompletionsForModuleExports: true } | ||
}); | ||
|
||
verify.applyCodeActionFromCompletion("", { | ||
name: "defaults", | ||
source: "/third_party/marked/src/defaults", | ||
description: `Import 'defaults' from module "./third_party/marked/src/defaults"`, | ||
data: { | ||
exportName: "defaults", | ||
fileName: "/third_party/marked/src/defaults.js", | ||
}, | ||
newFileContent: `import { defaults } from "./third_party/marked/src/defaults"; | ||
|
||
d` | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is
forEachExportAndPropertyOfModule
only used in the services layer?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep.