Skip to content

Commit d2443a5

Browse files
sandersnsandersn
authored
@typedef: Improve error spans from declaration emit (microsoft#42501)
* @typedef: Improve error spans from declaration emit This is a proof-of-concept fix. I think it could be expanded for all of jsdoc, but I only set it up for jsdoc type aliases. It could use a lot of polish too. * track error node in isSymbolAccessible instead * Switch to using enclosingDeclaration Remove trueErrorNode * add test of @callback and @enum * Better error + fix @enum error Since enums don't have a name property, you *have* to call `getNameOfDeclaration` to go looking through the AST for one.
1 parent 517f329 commit d2443a5

12 files changed

+88
-26
lines changed

‎src/compiler/checker.ts‎

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4183,7 +4183,8 @@ namespace ts {
41834183
return {
41844184
accessibility: SymbolAccessibility.CannotBeNamed,
41854185
errorSymbolName: symbolToString(symbol, enclosingDeclaration, meaning),
4186-
errorModuleName: symbolToString(symbolExternalModule)
4186+
errorModuleName: symbolToString(symbolExternalModule),
4187+
errorNode: isInJSFile(enclosingDeclaration) ? enclosingDeclaration : undefined,
41874188
};
41884189
}
41894190
}
@@ -4699,7 +4700,7 @@ namespace ts {
46994700

47004701
function createMappedTypeNodeFromType(type: MappedType) {
47014702
Debug.assert(!!(type.flags & TypeFlags.Object));
4702-
const readonlyToken = type.declaration.readonlyToken ? <ReadonlyToken | PlusToken | MinusToken>factory.createToken(type.declaration.readonlyToken.kind) : undefined;
4703+
const readonlyToken = type.declaration.readonlyToken ? <ReadonlyKeyword | PlusToken | MinusToken>factory.createToken(type.declaration.readonlyToken.kind) : undefined;
47034704
const questionToken = type.declaration.questionToken ? <QuestionToken | PlusToken | MinusToken>factory.createToken(type.declaration.questionToken.kind) : undefined;
47044705
let appropriateConstraintTypeNode: TypeNode;
47054706
if (isMappedTypeWithKeyofConstraintDeclaration(type)) {
@@ -6183,7 +6184,7 @@ namespace ts {
61836184
tracker: {
61846185
...oldcontext.tracker,
61856186
trackSymbol: (sym, decl, meaning) => {
6186-
const accessibleResult = isSymbolAccessible(sym, decl, meaning, /*computeALiases*/ false);
6187+
const accessibleResult = isSymbolAccessible(sym, decl, meaning, /*computeAliases*/ false);
61876188
if (accessibleResult.accessibility === SymbolAccessibility.Accessible) {
61886189
// Lookup the root symbol of the chain of refs we'll use to access it and serialize it
61896190
const chain = lookupSymbolChainWorker(sym, context, meaning);
@@ -6625,16 +6626,17 @@ namespace ts {
66256626
function addResult(node: Statement, additionalModifierFlags: ModifierFlags) {
66266627
if (canHaveModifiers(node)) {
66276628
let newModifierFlags: ModifierFlags = ModifierFlags.None;
6629+
const enclosingDeclaration = context.enclosingDeclaration &&
6630+
(isJSDocTypeAlias(context.enclosingDeclaration) ? getSourceFileOfNode(context.enclosingDeclaration) : context.enclosingDeclaration);
66286631
if (additionalModifierFlags & ModifierFlags.Export &&
6629-
context.enclosingDeclaration &&
6630-
(isExportingScope(context.enclosingDeclaration) || isModuleDeclaration(context.enclosingDeclaration)) &&
6632+
enclosingDeclaration && (isExportingScope(enclosingDeclaration) || isModuleDeclaration(enclosingDeclaration)) &&
66316633
canHaveExportModifier(node)
66326634
) {
66336635
// Classes, namespaces, variables, functions, interfaces, and types should all be `export`ed in a module context if not private
66346636
newModifierFlags |= ModifierFlags.Export;
66356637
}
66366638
if (addingDeclare && !(newModifierFlags & ModifierFlags.Export) &&
6637-
(!context.enclosingDeclaration || !(context.enclosingDeclaration.flags & NodeFlags.Ambient)) &&
6639+
(!enclosingDeclaration || !(enclosingDeclaration.flags & NodeFlags.Ambient)) &&
66386640
(isEnumDeclaration(node) || isVariableStatement(node) || isFunctionDeclaration(node) || isClassDeclaration(node) || isModuleDeclaration(node))) {
66396641
// Classes, namespaces, variables, enums, and functions all need `declare` modifiers to be valid in a declaration file top-level scope
66406642
newModifierFlags |= ModifierFlags.Ambient;
@@ -6657,6 +6659,8 @@ namespace ts {
66576659
const commentText = jsdocAliasDecl ? jsdocAliasDecl.comment || jsdocAliasDecl.parent.comment : undefined;
66586660
const oldFlags = context.flags;
66596661
context.flags |= NodeBuilderFlags.InTypeAlias;
6662+
const oldEnclosingDecl = context.enclosingDeclaration;
6663+
context.enclosingDeclaration = jsdocAliasDecl;
66606664
const typeNode = jsdocAliasDecl && jsdocAliasDecl.typeExpression
66616665
&& isJSDocTypeExpression(jsdocAliasDecl.typeExpression)
66626666
&& serializeExistingTypeNode(context, jsdocAliasDecl.typeExpression.type, includePrivateSymbol, bundled)
@@ -6666,6 +6670,7 @@ namespace ts {
66666670
!commentText ? [] : [{ kind: SyntaxKind.MultiLineCommentTrivia, text: "*\n * " + commentText.replace(/\n/g, "\n * ") + "\n ", pos: -1, end: -1, hasTrailingNewLine: true }]
66676671
), modifierFlags);
66686672
context.flags = oldFlags;
6673+
context.enclosingDeclaration = oldEnclosingDecl;
66696674
}
66706675

66716676
function serializeInterface(symbol: Symbol, symbolName: string, modifierFlags: ModifierFlags) {

‎src/compiler/diagnosticMessages.json‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3537,6 +3537,10 @@
35373537
"category": "Error",
35383538
"code": 4083
35393539
},
3540+
"Exported type alias '{0}' has or is using private name '{1}' from module {2}.": {
3541+
"category": "Error",
3542+
"code": 4084
3543+
},
35403544
"Conflicting definitions for '{0}' found at '{1}' and '{2}'. Consider installing a specific version of this library to resolve the conflict.": {
35413545
"category": "Error",
35423546
"code": 4090

‎src/compiler/transformers/declarations.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -217,12 +217,12 @@ namespace ts {
217217

218218
function transformDeclarationsForJS(sourceFile: SourceFile, bundled?: boolean) {
219219
const oldDiag = getSymbolAccessibilityDiagnostic;
220-
getSymbolAccessibilityDiagnostic = (s) => ({
220+
getSymbolAccessibilityDiagnostic = (s) => (s.errorNode && canProduceDiagnostics(s.errorNode) ? createGetSymbolAccessibilityDiagnosticForNode(s.errorNode)(s) : ({
221221
diagnosticMessage: s.errorModuleName
222222
? Diagnostics.Declaration_emit_for_this_file_requires_using_private_name_0_from_module_1_An_explicit_type_annotation_may_unblock_declaration_emit
223223
: Diagnostics.Declaration_emit_for_this_file_requires_using_private_name_0_An_explicit_type_annotation_may_unblock_declaration_emit,
224224
errorNode: s.errorNode || sourceFile
225-
});
225+
}));
226226
const result = resolver.getDeclarationStatementsForSourceFile(sourceFile, declarationEmitNodeBuilderFlags, symbolTracker, bundled);
227227
getSymbolAccessibilityDiagnostic = oldDiag;
228228
return result;

‎src/compiler/transformers/declarations/diagnostics.ts‎

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,10 @@ namespace ts {
2727
| TypeAliasDeclaration
2828
| ConstructorDeclaration
2929
| IndexSignatureDeclaration
30-
| PropertyAccessExpression;
30+
| PropertyAccessExpression
31+
| JSDocTypedefTag
32+
| JSDocCallbackTag
33+
| JSDocEnumTag;
3134

3235
export function canProduceDiagnostics(node: Node): node is DeclarationDiagnosticProducing {
3336
return isVariableDeclaration(node) ||
@@ -48,7 +51,8 @@ namespace ts {
4851
isTypeAliasDeclaration(node) ||
4952
isConstructorDeclaration(node) ||
5053
isIndexSignatureDeclaration(node) ||
51-
isPropertyAccessExpression(node);
54+
isPropertyAccessExpression(node) ||
55+
isJSDocTypeAlias(node);
5256
}
5357

5458
export function createGetSymbolAccessibilityDiagnosticForNodeName(node: DeclarationDiagnosticProducing) {
@@ -124,7 +128,7 @@ namespace ts {
124128
}
125129
}
126130

127-
export function createGetSymbolAccessibilityDiagnosticForNode(node: DeclarationDiagnosticProducing): (symbolAccessibilityResult: SymbolAccessibilityResult) => SymbolAccessibilityDiagnostic | undefined {
131+
export function createGetSymbolAccessibilityDiagnosticForNode(node: DeclarationDiagnosticProducing): GetSymbolAccessibilityDiagnostic {
128132
if (isVariableDeclaration(node) || isPropertyDeclaration(node) || isPropertySignature(node) || isPropertyAccessExpression(node) || isBindingElement(node) || isConstructorDeclaration(node)) {
129133
return getVariableDeclarationTypeVisibilityError;
130134
}
@@ -149,7 +153,7 @@ namespace ts {
149153
else if (isImportEqualsDeclaration(node)) {
150154
return getImportEntityNameVisibilityError;
151155
}
152-
else if (isTypeAliasDeclaration(node)) {
156+
else if (isTypeAliasDeclaration(node) || isJSDocTypeAlias(node)) {
153157
return getTypeAliasDeclarationVisibilityError;
154158
}
155159
else {
@@ -474,11 +478,13 @@ namespace ts {
474478
};
475479
}
476480

477-
function getTypeAliasDeclarationVisibilityError(): SymbolAccessibilityDiagnostic {
481+
function getTypeAliasDeclarationVisibilityError(symbolAccessibilityResult: SymbolAccessibilityResult): SymbolAccessibilityDiagnostic {
478482
return {
479-
diagnosticMessage: Diagnostics.Exported_type_alias_0_has_or_is_using_private_name_1,
480-
errorNode: (node as TypeAliasDeclaration).type,
481-
typeName: (node as TypeAliasDeclaration).name
483+
diagnosticMessage: symbolAccessibilityResult.errorModuleName
484+
? Diagnostics.Exported_type_alias_0_has_or_is_using_private_name_1_from_module_2
485+
: Diagnostics.Exported_type_alias_0_has_or_is_using_private_name_1,
486+
errorNode: isJSDocTypeAlias(node) ? Debug.checkDefined(node.typeExpression) : (node as TypeAliasDeclaration).type,
487+
typeName: isJSDocTypeAlias(node) ? getNameOfDeclaration(node) : (node as TypeAliasDeclaration).name,
482488
};
483489
}
484490
}

‎tests/baselines/reference/jsDeclarationsClassImplementsGenericsSerialization.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,4 +68,4 @@ export class Encoder<T> implements IEncoder<T> {
6868
*/
6969
encode(value: T): Uint8Array;
7070
}
71-
export type IEncoder<T> = import("./interface").Encoder<T>;
71+
export type IEncoder<T> = import('./interface').Encoder<T>;

‎tests/baselines/reference/jsDeclarationsImportAliasExposedWithinNamespace.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ export namespace myTypes {
6060
/**
6161
* - Prop 1.
6262
*/
63-
prop1: typeA;
63+
prop1: myTypes.typeA;
6464
/**
6565
* - Prop 2.
6666
*/

‎tests/baselines/reference/jsDeclarationsImportAliasExposedWithinNamespaceCjs.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ export namespace myTypes {
6868
/**
6969
* - Prop 1.
7070
*/
71-
prop1: typeA;
71+
prop1: myTypes.typeA;
7272
/**
7373
* - Prop 2.
7474
*/

‎tests/baselines/reference/jsDeclarationsParameterTagReusesInputNodeInEmit1.errors.txt‎

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,6 @@
1-
tests/cases/conformance/jsdoc/declarations/file.js(8,1): error TS9006: Declaration emit for this file requires using private name 'Base' from module '"tests/cases/conformance/jsdoc/declarations/base"'. An explicit type annotation may unblock declaration emit.
1+
tests/cases/conformance/jsdoc/declarations/file.js(1,5): error TS4084: Exported type alias 'BaseFactory' has or is using private name 'Base' from module "tests/cases/conformance/jsdoc/declarations/base".
2+
tests/cases/conformance/jsdoc/declarations/file.js(3,4): error TS4084: Exported type alias 'BaseFactoryFactory' has or is using private name 'Base' from module "tests/cases/conformance/jsdoc/declarations/base".
3+
tests/cases/conformance/jsdoc/declarations/file.js(6,5): error TS4084: Exported type alias 'couldntThinkOfAny' has or is using private name 'Base' from module "tests/cases/conformance/jsdoc/declarations/base".
24

35

46
==== tests/cases/conformance/jsdoc/declarations/base.js (0 errors) ====
@@ -14,17 +16,29 @@ tests/cases/conformance/jsdoc/declarations/file.js(8,1): error TS9006: Declarati
1416

1517
module.exports = BaseFactory;
1618

17-
==== tests/cases/conformance/jsdoc/declarations/file.js (1 errors) ====
19+
==== tests/cases/conformance/jsdoc/declarations/file.js (3 errors) ====
1820
/** @typedef {import('./base')} BaseFactory */
21+
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
22+
!!! error TS4084: Exported type alias 'BaseFactory' has or is using private name 'Base' from module "tests/cases/conformance/jsdoc/declarations/base".
23+
/**
24+
* @callback BaseFactoryFactory
25+
~~~~~~~~~~~~~~~~~~~~~~~~~~~~
26+
* @param {import('./base')} factory
27+
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
28+
*/
29+
~
30+
!!! error TS4084: Exported type alias 'BaseFactoryFactory' has or is using private name 'Base' from module "tests/cases/conformance/jsdoc/declarations/base".
31+
/** @enum {import('./base')} */
32+
~~~~~~~~~~~~~~~~~~~~~~~~
33+
!!! error TS4084: Exported type alias 'couldntThinkOfAny' has or is using private name 'Base' from module "tests/cases/conformance/jsdoc/declarations/base".
34+
const couldntThinkOfAny = {}
1935

2036
/**
2137
*
2238
* @param {InstanceType<BaseFactory["Base"]>} base
2339
* @returns {InstanceType<BaseFactory["Base"]>}
2440
*/
2541
const test = (base) => {
26-
~~~~~
27-
!!! error TS9006: Declaration emit for this file requires using private name 'Base' from module '"tests/cases/conformance/jsdoc/declarations/base"'. An explicit type annotation may unblock declaration emit.
2842
return base;
2943
};
3044

‎tests/baselines/reference/jsDeclarationsParameterTagReusesInputNodeInEmit1.js‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,12 @@ module.exports = BaseFactory;
1515

1616
//// [file.js]
1717
/** @typedef {import('./base')} BaseFactory */
18+
/**
19+
* @callback BaseFactoryFactory
20+
* @param {import('./base')} factory
21+
*/
22+
/** @enum {import('./base')} */
23+
const couldntThinkOfAny = {}
1824

1925
/**
2026
*
@@ -37,6 +43,12 @@ BaseFactory.Base = Base;
3743
module.exports = BaseFactory;
3844
//// [file.js]
3945
/** @typedef {import('./base')} BaseFactory */
46+
/**
47+
* @callback BaseFactoryFactory
48+
* @param {import('./base')} factory
49+
*/
50+
/** @enum {import('./base')} */
51+
const couldntThinkOfAny = {};
4052
/**
4153
*
4254
* @param {InstanceType<BaseFactory["Base"]>} base

‎tests/baselines/reference/jsDeclarationsParameterTagReusesInputNodeInEmit1.symbols‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -27,18 +27,25 @@ module.exports = BaseFactory;
2727

2828
=== tests/cases/conformance/jsdoc/declarations/file.js ===
2929
/** @typedef {import('./base')} BaseFactory */
30+
/**
31+
* @callback BaseFactoryFactory
32+
* @param {import('./base')} factory
33+
*/
34+
/** @enum {import('./base')} */
35+
const couldntThinkOfAny = {}
36+
>couldntThinkOfAny : Symbol(couldntThinkOfAny, Decl(file.js, 6, 5), Decl(file.js, 5, 4))
3037

3138
/**
3239
*
3340
* @param {InstanceType<BaseFactory["Base"]>} base
3441
* @returns {InstanceType<BaseFactory["Base"]>}
3542
*/
3643
const test = (base) => {
37-
>test : Symbol(test, Decl(file.js, 7, 5))
38-
>base : Symbol(base, Decl(file.js, 7, 14))
44+
>test : Symbol(test, Decl(file.js, 13, 5))
45+
>base : Symbol(base, Decl(file.js, 13, 14))
3946

4047
return base;
41-
>base : Symbol(base, Decl(file.js, 7, 14))
48+
>base : Symbol(base, Decl(file.js, 13, 14))
4249

4350
};
4451

0 commit comments

Comments
 (0)