From 4d2f268045045546b3c8dcd346780eb4f5abda11 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Tue, 23 Oct 2018 10:12:46 +0200 Subject: [PATCH] fix: require distinct argument and property names (#272) Restrict the usage of datatype parameters in functions, so that languages with keyword argument support can inline datatype parameters into the function declaration. This means it cannot share any field names with remaining function argument names. Fixes #268. --- packages/jsii/lib/assembler.ts | 83 ++++++++++++++++++- .../neg.mix-datatype-and-arg-name.ts | 15 ++++ 2 files changed, 95 insertions(+), 3 deletions(-) create mode 100644 packages/jsii/test/negatives/neg.mix-datatype-and-arg-name.ts diff --git a/packages/jsii/lib/assembler.ts b/packages/jsii/lib/assembler.ts index 4a88094027..d46dc342ac 100644 --- a/packages/jsii/lib/assembler.ts +++ b/packages/jsii/lib/assembler.ts @@ -190,7 +190,7 @@ export class Assembler implements Emitter { * * @returns the de-referenced type, if it was found, otherwise ``undefined``. */ - private _dereference(ref: spec.NamedTypeReference, referencingNode: ts.Node): spec.Type | undefined { + private _dereference(ref: spec.NamedTypeReference, referencingNode: ts.Node | null): spec.Type | undefined { const [assm, ] = ref.fqn.split('.'); let type; if (assm === this.projectInfo.name) { @@ -615,11 +615,13 @@ export class Assembler implements Emitter { this._diagnostic(declaration, ts.DiagnosticCategory.Error, `Unable to compute signature for ${type.fqn}#${symbol.name}`); return; } + const parameters = await Promise.all(signature.getParameters().map(p => this._toParameter(p))); + const returnType = signature.getReturnType(); const method: spec.Method = { abstract: _isAbstract(symbol, type), name: symbol.name, - parameters: await Promise.all(signature.getParameters().map(p => this._toParameter(p))), + parameters, protected: _isProtected(symbol), returns: _isVoid(returnType) ? undefined : await this._typeReference(returnType, declaration), static: _isStatic(symbol), @@ -628,6 +630,29 @@ export class Assembler implements Emitter { this._visitDocumentation(symbol, method); + // If the last parameter is a datatype, verify that it does not share any field names with + // other function arguments, so that it can be turned into keyword arguments by jsii frontends + // that support such. + const lastParamTypeRef = apply(last(parameters), x => x.type); + const lastParamSymbol = last(signature.getParameters()); + if (lastParamTypeRef && spec.isNamedTypeReference(lastParamTypeRef)) { + this._deferUntilTypesAvailable(symbol.name, [lastParamTypeRef], lastParamSymbol!.declarations[0], (lastParamType) => { + if (!spec.isInterfaceType(lastParamType) || !lastParamType.datatype) { return; } + + // Liftable datatype, make sure no parameter names match any of the properties in the datatype + const propNames = this.allProperties(lastParamType); + const paramNames = new Set(parameters.slice(0, parameters.length - 1).map(x => x.name)); + const sharedNames = intersection(propNames, paramNames); + + if (sharedNames.size > 0) { + this._diagnostic( + declaration, + ts.DiagnosticCategory.Error, + `Name occurs in both function arguments and in datatype properties, rename one: ${Array.from(sharedNames).join(', ')}`); + } + }); + } + type.methods = type.methods || []; type.methods.push(method); } @@ -867,6 +892,31 @@ export class Assembler implements Emitter { def.dependedFqns = def.dependedFqns.filter(fqns.has.bind(fqns)); } } + + /** + * Return the set of all (inherited) properties of an interface + */ + private allProperties(root: spec.InterfaceType): Set { + const self = this; + + const ret = new Set(); + recurse(root); + return ret; + + function recurse(int: spec.InterfaceType) { + for (const property of int.properties || []) { + ret.add(property.name); + } + + for (const baseRef of int.interfaces || []) { + const base = self._dereference(baseRef, null); + if (!base) { throw new Error('Impossible to have unresolvable base in allProperties()'); } + if (!spec.isInterfaceType(base)) { throw new Error('Impossible to have non-interface base in allProperties()'); } + + recurse(base); + } + } + } } /** @@ -1024,6 +1074,33 @@ interface DeferredRecord { cb: () => void; } +/** + * Return the last element from a list + */ +function last(xs: T[]): T | undefined { + return xs.length > 0 ? xs[xs.length - 1] : undefined; +} + +/** + * Apply a function to a value if it's not equal to undefined + */ +function apply(x: T | undefined, fn: (x: T) => U | undefined): U | undefined { + return x !== undefined ? fn(x) : undefined; +} + +/** + * Return the intersection of two sets + */ +function intersection(xs: Set, ys: Set): Set { + const ret = new Set(); + for (const x of xs) { + if (ys.has(x)) { + ret.add(x); + } + } + return ret; +} + /** * Whether or not the given name is conventionally an interface name * @@ -1032,4 +1109,4 @@ interface DeferredRecord { */ function isInterfaceName(name: string) { return name.length >= 2 && name.charAt(0) === 'I' && name.charAt(1).toUpperCase() === name.charAt(1); -} \ No newline at end of file +} diff --git a/packages/jsii/test/negatives/neg.mix-datatype-and-arg-name.ts b/packages/jsii/test/negatives/neg.mix-datatype-and-arg-name.ts new file mode 100644 index 0000000000..243ebd5a69 --- /dev/null +++ b/packages/jsii/test/negatives/neg.mix-datatype-and-arg-name.ts @@ -0,0 +1,15 @@ +///!MATCH_ERROR: Name occurs in both function arguments and in datatype properties, rename one: dontWorry + +export interface Lyrics { + dontWorry: string; + beHappy: string; +} + +export class MyClass { + // Can't have an argument name 'dontWorry' if the last parameter is a datatype + // which also has a field 'dontWorry'--that will give errors if the datatype + // argument fields are lifted to keyword arguments. + public dance(dontWorry: string, lyrics: Lyrics) { + return `${dontWorry}: ${lyrics.beHappy}`; + } +} \ No newline at end of file