-
Notifications
You must be signed in to change notification settings - Fork 246
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
fix(jsii): build succeeds using Omit<T, K> #1708
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 |
---|---|---|
|
@@ -758,12 +758,14 @@ export class Assembler implements Emitter { | |
|
||
if (ts.isClassDeclaration(node) && _isExported(node)) { | ||
// export class Name { ... } | ||
this._validateHeritageClauses(node.heritageClauses); | ||
jsiiType = await this._visitClass( | ||
this._typeChecker.getTypeAtLocation(node), | ||
context, | ||
); | ||
} else if (ts.isInterfaceDeclaration(node) && _isExported(node)) { | ||
// export interface Name { ... } | ||
this._validateHeritageClauses(node.heritageClauses); | ||
jsiiType = await this._visitInterface( | ||
this._typeChecker.getTypeAtLocation(node), | ||
context, | ||
|
@@ -892,6 +894,45 @@ export class Assembler implements Emitter { | |
return [jsiiType]; | ||
} | ||
|
||
private _validateHeritageClauses(clauses?: ts.NodeArray<ts.HeritageClause>) { | ||
if (clauses == null || clauses.length === 0) { | ||
// Nothing to do. | ||
return; | ||
} | ||
for (const clause of clauses) { | ||
for (const node of clause.types) { | ||
const parentType = this._typeChecker.getTypeAtLocation(node); | ||
// For some reason, we cannot trust parentType.isClassOrInterface() | ||
const badDecl = parentType.symbol.declarations.find( | ||
(decl) => | ||
!ts.isClassDeclaration(decl) && // <-- local classes | ||
!ts.isInterfaceDeclaration(decl) && // <-- local interfaces | ||
!ts.isModuleDeclaration(decl), // <-- imported types | ||
); | ||
if (badDecl != null) { | ||
this._diagnostic( | ||
node, | ||
ts.DiagnosticCategory.Error, | ||
`Illegal "${clauseType(clause.token)}" value for an exported API: ${ | ||
ts.SyntaxKind[badDecl.kind] | ||
}`, | ||
Comment on lines
+916
to
+918
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 this error look like the Make sure it's easy to understand for an external contributor to the CDK. 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'd have wanted to make it more specific (mapped/utility types are not allowed here), however the type checker does not really give me a way to do this with confidence... 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 looks fairly good. |
||
); | ||
} | ||
} | ||
} | ||
|
||
function clauseType(token: ts.SyntaxKind): string { | ||
switch (token) { | ||
case ts.SyntaxKind.ExtendsKeyword: | ||
return 'extends'; | ||
case ts.SyntaxKind.ImplementsKeyword: | ||
return 'implements'; | ||
default: | ||
return ts.SyntaxKind[token]; | ||
} | ||
} | ||
} | ||
|
||
private declarationLocation(node: ts.Declaration): spec.SourceLocation { | ||
const file = node.getSourceFile(); | ||
const line = ts.getLineAndCharacterOfPosition(file, node.getStart()).line; | ||
|
@@ -1587,7 +1628,7 @@ export class Assembler implements Emitter { | |
for (const member of declaringType.getProperties()) { | ||
if ( | ||
!(declaringType.symbol.getDeclarations() ?? []).find( | ||
(decl) => decl === member.valueDeclaration.parent, | ||
(decl) => decl === member.valueDeclaration?.parent, | ||
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. The |
||
) | ||
) { | ||
continue; | ||
|
@@ -2104,7 +2145,7 @@ export class Assembler implements Emitter { | |
this._diagnostic( | ||
declaration, | ||
ts.DiagnosticCategory.Error, | ||
'Only string index maps are supported', | ||
'Only string-indexed map types are supported', | ||
); | ||
elementtype = spec.CANONICAL_ANY; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -246,6 +246,7 @@ export class Compiler implements Emitter { | |
diagnostics.push(...assmEmit.diagnostics); | ||
} catch (e) { | ||
LOG.error(`Error during type model analysis: ${e}\n${e.stack}`); | ||
hasErrors = true; | ||
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 previously caused exceptions thrown from the assembler to be "swallowed" (after logging to |
||
} | ||
|
||
return { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
///!MATCH_ERROR: Illegal "extends" value for an exported API | ||
|
||
export interface FooBar { | ||
readonly foo: string; | ||
readonly bar: string; | ||
} | ||
|
||
// Illegal attempt to extend Omit<T, Key> | ||
export interface BarBaz extends Omit<FooBar, 'foo'> { | ||
readonly baz: number; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
///!MATCH_ERROR: Illegal "implements" value for an exported API | ||
|
||
export interface FooBar { | ||
readonly foo: string; | ||
readonly bar: string; | ||
} | ||
|
||
// Illegal attempt to implement Omit<T, Key> | ||
export class BarBaz implements Omit<FooBar, 'foo'> { | ||
public readonly bar = 'BAR!'; | ||
public readonly baz: number = 1337; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
///!MATCH_ERROR: Only string-indexed map types are supported | ||
|
||
export interface FooBar { | ||
readonly foo: string; | ||
readonly bar: string; | ||
} | ||
|
||
// Illegal attempt to return Omit<T, Key> | ||
export interface IReturner { | ||
bar(): Omit<FooBar, 'foo'>; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
///!MATCH_ERROR: Only string-indexed map types are supported | ||
|
||
export interface FooBar { | ||
readonly foo: string; | ||
readonly bar: string; | ||
} | ||
|
||
// Illegal attempt to accept Omit<T, Key> | ||
export interface IReturner { | ||
bar(opts: Omit<FooBar, 'foo'>): void; | ||
} |
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.
Expand a bit with an example of when it has lost your trust?
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.
Basically, imported types respond
false
to allis*
methods. :(