Skip to content

Commit

Permalink
fix(pacmak): API locations for inherited members are incorrect (#3130)
Browse files Browse the repository at this point in the history 
The code generators in pacmak are now passing API locations to Rosetta,
to indicate the source of the snippets they're finding. This is necessary
to distinguish the same code snippet found in a different submodule, because
it might be using a different fixture.

They all had the same mistake though, in that `rosetta extract` was identifying
snippets by "defining API element", whereas pacmak was identifying snippets by
what class it was generating code for... and in the case of inheritance, those
two locations would be different.

Put in extra effort in the pacmak target generators to keep track of where
we found a method or property, so we can use the same location translating
the examples.

This was validated by running pacmak over the entire CDK repository, with the
option `--rosetta-unknown-snippets=fail`, and making sure it completed
successfully. We prevent regressions by doing the same in the jsii integ tests:
cdklabs/cdk-ops#1777. I hope that will do, I'm not sure how to turn this into a proper unit test.

---

By submitting this pull request, I confirm that my contribution is made under the terms of the [Apache 2.0 license].

[Apache 2.0 license]: https://www.apache.org/licenses/LICENSE-2.0
  • Loading branch information
@rix0rrr
rix0rrr authored Nov 8, 2021
1 parent 236dd88 commit 8d0a248
Show file tree
Hide file tree
Showing 4 changed files with 109 additions and 43 deletions.
10 changes: 10 additions & 0 deletions packages/jsii-pacmak/lib/targets/_utils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,13 @@ export function stabilityPrefixFor(element: spec.Documentable): string {
export function renderSummary(docs?: spec.Docs): string {
return docs?.summary ? stabilityPrefixFor({ docs }) + docs.summary : '';
}

export interface PropertyDefinition {
readonly prop: spec.Property;
readonly definingType: spec.Type;
}

export interface MethodDefinition {
readonly method: spec.Method;
readonly definingType: spec.Type;
}
46 changes: 29 additions & 17 deletions packages/jsii-pacmak/lib/targets/dotnet/dotnetgenerator.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { Rosetta } from 'jsii-rosetta';
import * as path from 'path';

import { Generator, Legalese } from '../../generator';
import { MethodDefinition, PropertyDefinition } from '../_utils';
import { DotNetDocGenerator } from './dotnetdocgenerator';
import { DotNetRuntimeGenerator } from './dotnetruntimegenerator';
import { DotNetTypeResolver } from './dotnettyperesolver';
Expand Down Expand Up @@ -419,7 +420,7 @@ export class DotNetGenerator extends Generator {
}

protected onMethod(cls: spec.ClassType, method: spec.Method) {
this.emitMethod(cls, method);
this.emitMethod(cls, method, cls);
}

protected onMethodOverload(
Expand All @@ -431,26 +432,26 @@ export class DotNetGenerator extends Generator {
}

protected onProperty(cls: spec.ClassType, prop: spec.Property) {
this.emitProperty(cls, prop);
this.emitProperty(cls, prop, cls);
}

protected onStaticMethod(cls: spec.ClassType, method: spec.Method) {
this.emitMethod(cls, method);
this.emitMethod(cls, method, cls);
}

protected onStaticMethodOverload(
cls: spec.ClassType,
overload: spec.Method,
_originalMethod: spec.Method,
) {
this.emitMethod(cls, overload);
this.emitMethod(cls, overload, cls);
}

protected onStaticProperty(cls: spec.ClassType, prop: spec.Property) {
if (prop.const) {
this.emitConstProperty(cls, prop);
} else {
this.emitProperty(cls, prop);
this.emitProperty(cls, prop, cls);
}
}

Expand All @@ -459,7 +460,7 @@ export class DotNetGenerator extends Generator {
prop: spec.Property,
_union: spec.UnionTypeReference,
) {
this.emitProperty(cls, prop);
this.emitProperty(cls, prop, cls);
}

protected onBeginEnum(enm: spec.EnumType) {
Expand Down Expand Up @@ -516,6 +517,7 @@ export class DotNetGenerator extends Generator {
private emitMethod(
cls: spec.ClassType | spec.InterfaceType,
method: spec.Method,
definingType: spec.Type,
emitForProxyOrDatatype = false,
): void {
this.emitNewLineIfNecessary();
Expand Down Expand Up @@ -568,7 +570,7 @@ export class DotNetGenerator extends Generator {

this.dotnetDocGenerator.emitDocs(method, {
api: 'member',
fqn: cls.fqn,
fqn: definingType.fqn,
memberName: method.name,
});
this.dotnetRuntimeGenerator.emitAttributesForMethod(
Expand Down Expand Up @@ -813,7 +815,7 @@ export class DotNetGenerator extends Generator {
proxy: boolean,
): void {
// The key is in the form 'method.name;parameter1;parameter2;' etc
const methods: Map<string, spec.Method> = new Map<string, spec.Method>();
const methods = new Map<string, MethodDefinition>();
/*
Only get the first declaration encountered, and keep it if it is abstract. The list contains ALL
methods and properties encountered, in the order encountered. An abstract class can have concrete
Expand All @@ -822,15 +824,15 @@ export class DotNetGenerator extends Generator {
*/
const excludedMethod: string[] = []; // Keeps track of the methods we already ran into and don't want to emit
const excludedProperties: string[] = []; // Keeps track of the properties we already ran into and don't want to emit
const properties: { [name: string]: spec.Property } = {};
const properties: { [name: string]: PropertyDefinition } = {};
const collectAbstractMembers = (
currentType: spec.InterfaceType | spec.ClassType,
) => {
for (const prop of currentType.properties ?? []) {
if (!excludedProperties.includes(prop.name)) {
// If we have never run into this property before and it is abstract, we keep it
if (prop.abstract) {
properties[prop.name] = prop;
properties[prop.name] = { prop, definingType: currentType };
}
excludedProperties.push(prop.name);
}
Expand All @@ -848,7 +850,10 @@ export class DotNetGenerator extends Generator {
if (!excludedMethod.includes(`${method.name}${methodParameters}`)) {
// If we have never run into this method before and it is abstract, we keep it
if (method.abstract) {
methods.set(`${method.name}${methodParameters}`, method);
methods.set(`${method.name}${methodParameters}`, {
method,
definingType: currentType,
});
}
excludedMethod.push(`${method.name}${methodParameters}`);
}
Expand Down Expand Up @@ -879,24 +884,30 @@ export class DotNetGenerator extends Generator {
// emit all properties
for (const propName of Object.keys(properties)) {
const prop = clone(properties[propName]);
prop.abstract = false;
this.emitProperty(ifc, prop, datatype, proxy);
prop.prop.abstract = false;
this.emitProperty(ifc, prop.prop, prop.definingType, datatype, proxy);
}
// emit all the methods
for (const methodNameAndParameters of methods.keys()) {
const originalMethod = methods.get(methodNameAndParameters);
if (originalMethod) {
const method = clone(originalMethod);
method.abstract = false;
this.emitMethod(ifc, method, /* emitForProxyOrDatatype */ true);
method.method.abstract = false;
this.emitMethod(
ifc,
method.method,
method.definingType,
/* emitForProxyOrDatatype */ true,
);

for (const overloadedMethod of this.createOverloadsForOptionals(
method,
method.method,
)) {
overloadedMethod.abstract = false;
this.emitMethod(
ifc,
overloadedMethod,
method.definingType,
/* emitForProxyOrDatatype */ true,
);
}
Expand All @@ -910,6 +921,7 @@ export class DotNetGenerator extends Generator {
private emitProperty(
cls: spec.Type,
prop: spec.Property,
definingType: spec.Type,
datatype = false,
proxy = false,
): void {
Expand All @@ -922,7 +934,7 @@ export class DotNetGenerator extends Generator {

this.dotnetDocGenerator.emitDocs(prop, {
api: 'member',
fqn: cls.fqn,
fqn: definingType.fqn,
memberName: prop.name,
});
if (prop.optional) {
Expand Down
34 changes: 23 additions & 11 deletions packages/jsii-pacmak/lib/targets/java.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -479,6 +479,9 @@ interface JavaProp {
// The original JSII property spec this struct was derived from
spec: spec.Property;

// The original JSII type this property was defined on
definingType: spec.Type;

// Canonical name of the Java property (eg: 'MyProperty')
propName: string;

Expand Down Expand Up @@ -729,14 +732,14 @@ class JavaGenerator extends Generator {
}

protected onProperty(cls: spec.ClassType, prop: spec.Property) {
this.emitProperty(cls, prop);
this.emitProperty(cls, prop, cls);
}

protected onStaticProperty(cls: spec.ClassType, prop: spec.Property) {
if (prop.const) {
this.emitConstProperty(cls, prop);
} else {
this.emitProperty(cls, prop);
this.emitProperty(cls, prop, cls);
}
}

Expand All @@ -748,7 +751,7 @@ class JavaGenerator extends Generator {
prop: spec.Property,
_union: spec.UnionTypeReference,
) {
this.emitProperty(cls, prop);
this.emitProperty(cls, prop, cls);
}

protected onMethod(cls: spec.ClassType, method: spec.Method) {
Expand Down Expand Up @@ -1364,6 +1367,7 @@ class JavaGenerator extends Generator {
private emitProperty(
cls: spec.Type,
prop: spec.Property,
definingType: spec.Type,
{
defaultImpl = false,
final = false,
Expand Down Expand Up @@ -1396,7 +1400,7 @@ class JavaGenerator extends Generator {
this.code.line();
this.addJavaDocs(prop, {
api: 'member',
fqn: cls.fqn,
fqn: definingType.fqn,
memberName: prop.name,
});
if (overrides && !prop.static) {
Expand Down Expand Up @@ -1568,7 +1572,10 @@ class JavaGenerator extends Generator {
const prop = clone(reflectProp.spec);
prop.abstract = false;
// Emitting "final" since this is a proxy and nothing will/should override this
this.emitProperty(type.spec, prop, { final: true, overrides: true });
this.emitProperty(type.spec, prop, reflectProp.definingType.spec, {
final: true,
overrides: true,
});
}

// emit all the methods
Expand Down Expand Up @@ -1620,7 +1627,7 @@ class JavaGenerator extends Generator {
(prop.parentType.fqn === type.fqn ||
!hasDefaultInterfaces(prop.assembly)),
)) {
this.emitProperty(type.spec, property.spec, {
this.emitProperty(type.spec, property.spec, property.definingType.spec, {
defaultImpl: true,
overrides: type.isInterfaceType(),
});
Expand Down Expand Up @@ -1676,13 +1683,18 @@ class JavaGenerator extends Generator {
}
}

private toJavaProp(property: spec.Property, inherited: boolean): JavaProp {
private toJavaProp(
property: spec.Property,
definingType: spec.Type,
inherited: boolean,
): JavaProp {
const safeName = JavaGenerator.safeJavaPropertyName(property.name);
const propName = jsiiToPascalCase(safeName);

return {
docs: property.docs,
spec: property,
definingType,
propName,
jsiiName: property.name,
nullable: !!property.optional,
Expand Down Expand Up @@ -1861,8 +1873,8 @@ class JavaGenerator extends Generator {
})) {
this.addJavaDocs(setter, {
api: 'member',
fqn: cls.fqn,
memberName: setter.name,
fqn: prop.definingType.fqn, // Could be inherited
memberName: prop.name,
});
this.emitStabilityAnnotations(prop.spec);
this.code.openBlock(
Expand Down Expand Up @@ -1940,7 +1952,7 @@ class JavaGenerator extends Generator {
const remarks = markDownToJavaDoc(
this.convertSamplesInMarkdown(prop.docs.remarks, {
api: 'member',
fqn: parentType.fqn,
fqn: prop.definingType.fqn,
memberName: prop.jsiiName,
}),
).trimRight();
Expand Down Expand Up @@ -2050,7 +2062,7 @@ class JavaGenerator extends Generator {
isBaseClass = false,
) {
for (const property of currentIfc.properties ?? []) {
const javaProp = this.toJavaProp(property, isBaseClass);
const javaProp = this.toJavaProp(property, currentIfc, isBaseClass);
propsByName[javaProp.propName] = javaProp;
}

Expand Down
Loading

0 comments on commit 8d0a248

Please sign in to comment.