Skip to content

Commit

Permalink
fix: require distinct argument and property names (#272)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
rix0rrr authored Oct 23, 2018
1 parent 03103f3 commit 4d2f268
Show file tree
Hide file tree
Showing 2 changed files with 95 additions and 3 deletions.
83 changes: 80 additions & 3 deletions packages/jsii/lib/assembler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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),
Expand All @@ -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);
}
Expand Down Expand Up @@ -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<string> {
const self = this;

const ret = new Set<string>();
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);
}
}
}
}

/**
Expand Down Expand Up @@ -1024,6 +1074,33 @@ interface DeferredRecord {
cb: () => void;
}

/**
* Return the last element from a list
*/
function last<T>(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<T, U>(x: T | undefined, fn: (x: T) => U | undefined): U | undefined {
return x !== undefined ? fn(x) : undefined;
}

/**
* Return the intersection of two sets
*/
function intersection<T>(xs: Set<T>, ys: Set<T>): Set<T> {
const ret = new Set<T>();
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
*
Expand All @@ -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);
}
}
15 changes: 15 additions & 0 deletions packages/jsii/test/negatives/neg.mix-datatype-and-arg-name.ts
Original file line number Diff line number Diff line change
@@ -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}`;
}
}

0 comments on commit 4d2f268

Please sign in to comment.