From f9b45f071c9eef62c396a0e59f319e5bbd055aa1 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Mon, 22 Oct 2018 15:50:25 +0200 Subject: [PATCH] fix: require distinct argument and property names 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 | 81 ++++++++++++++++++- .../neg.mix-datatype-and-arg-name.ts | 15 ++++ 2 files changed, 94 insertions(+), 2 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 c289900ca9..8714131e75 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) { @@ -601,11 +601,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), @@ -614,6 +616,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); } @@ -853,6 +878,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); + } + } + } } /** @@ -1008,4 +1058,31 @@ interface DeferredRecord { * Callback representing the action to run. */ 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; } \ 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