From 8d0a24826052600c3ab51cac7e31a2666930ef39 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Mon, 8 Nov 2021 15:48:39 +0100 Subject: [PATCH] fix(pacmak): API locations for inherited members are incorrect (#3130) 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 --- packages/jsii-pacmak/lib/targets/_utils.ts | 10 +++ .../lib/targets/dotnet/dotnetgenerator.ts | 46 +++++++++----- packages/jsii-pacmak/lib/targets/java.ts | 34 ++++++---- packages/jsii-pacmak/lib/targets/python.ts | 62 ++++++++++++++----- 4 files changed, 109 insertions(+), 43 deletions(-) diff --git a/packages/jsii-pacmak/lib/targets/_utils.ts b/packages/jsii-pacmak/lib/targets/_utils.ts index c1dbd596e6..1699454c95 100644 --- a/packages/jsii-pacmak/lib/targets/_utils.ts +++ b/packages/jsii-pacmak/lib/targets/_utils.ts @@ -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; +} diff --git a/packages/jsii-pacmak/lib/targets/dotnet/dotnetgenerator.ts b/packages/jsii-pacmak/lib/targets/dotnet/dotnetgenerator.ts index fe41b807a1..b6dd9bacce 100644 --- a/packages/jsii-pacmak/lib/targets/dotnet/dotnetgenerator.ts +++ b/packages/jsii-pacmak/lib/targets/dotnet/dotnetgenerator.ts @@ -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'; @@ -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( @@ -431,11 +432,11 @@ 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( @@ -443,14 +444,14 @@ export class DotNetGenerator extends Generator { 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); } } @@ -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) { @@ -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(); @@ -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( @@ -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 = new Map(); + const methods = new Map(); /* 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 @@ -822,7 +824,7 @@ 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, ) => { @@ -830,7 +832,7 @@ export class DotNetGenerator extends Generator { 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); } @@ -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}`); } @@ -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, ); } @@ -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 { @@ -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) { diff --git a/packages/jsii-pacmak/lib/targets/java.ts b/packages/jsii-pacmak/lib/targets/java.ts index 93a63c3ea6..f2bac01704 100644 --- a/packages/jsii-pacmak/lib/targets/java.ts +++ b/packages/jsii-pacmak/lib/targets/java.ts @@ -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; @@ -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); } } @@ -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) { @@ -1364,6 +1367,7 @@ class JavaGenerator extends Generator { private emitProperty( cls: spec.Type, prop: spec.Property, + definingType: spec.Type, { defaultImpl = false, final = false, @@ -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) { @@ -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 @@ -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(), }); @@ -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, @@ -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( @@ -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(); @@ -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; } diff --git a/packages/jsii-pacmak/lib/targets/python.ts b/packages/jsii-pacmak/lib/targets/python.ts index fe695983c3..077f8cec90 100644 --- a/packages/jsii-pacmak/lib/targets/python.ts +++ b/packages/jsii-pacmak/lib/targets/python.ts @@ -18,7 +18,7 @@ import { warn } from '../logging'; import { md2rst } from '../markdown'; import { Target, TargetOptions } from '../target'; import { shell } from '../util'; -import { renderSummary } from './_utils'; +import { renderSummary, PropertyDefinition } from './_utils'; import { NamingContext, toTypeName, @@ -329,6 +329,7 @@ abstract class BasePythonClassType implements PythonType, ISortableType { public constructor( protected readonly generator: PythonGenerator, public readonly pythonName: string, + public readonly spec: spec.Type, public readonly fqn: string | undefined, opts: PythonTypeOpts, public readonly docs: spec.Docs | undefined, @@ -474,7 +475,11 @@ abstract class BaseMethod implements PythonBase { } public get apiLocation(): ApiLocation { - return { api: 'member', fqn: this.parent.fqn, memberName: this.jsiiMethod }; + return { + api: 'member', + fqn: this.parent.fqn, + memberName: this.jsName ?? '', + }; } public requiredImports(context: EmitContext): PythonImports { @@ -549,6 +554,14 @@ abstract class BaseMethod implements PythonBase { } const documentableArgs: DocumentableArgument[] = this.parameters + .map( + (p) => + ({ + name: p.name, + docs: p.docs, + definingType: this.parent, + } as DocumentableArgument), + ) // If there's liftedProps, the last argument is the struct and it won't be _actually_ emitted. .filter((_, index) => this.liftedProp != null ? index < this.parameters.length - 1 : true, @@ -573,19 +586,28 @@ abstract class BaseMethod implements PythonBase { // Iterate over all of our props, and reflect them into our params. for (const prop of liftedProperties) { - const paramName = toPythonParameterName(prop.name); - const paramType = toTypeName(prop).pythonType({ + const paramName = toPythonParameterName(prop.prop.name); + const paramType = toTypeName(prop.prop).pythonType({ ...context, parameterType: true, }); - const paramDefault = prop.optional ? ' = None' : ''; + const paramDefault = prop.prop.optional ? ' = None' : ''; pythonParams.push(`${paramName}: ${paramType}${paramDefault}`); } } // Document them as keyword arguments - documentableArgs.push(...liftedProperties); + documentableArgs.push( + ...liftedProperties.map( + (p) => + ({ + name: p.prop.name, + docs: p.prop.docs, + definingType: p.definingType, + } as DocumentableArgument), + ), + ); } else if ( this.parameters.length >= 1 && this.parameters[this.parameters.length - 1].variadic @@ -701,7 +723,7 @@ abstract class BaseMethod implements PythonBase { // We need to build up a list of properties, which are mandatory, these are the // ones we will specifiy to start with in our dictionary literal. const liftedProps = this.getLiftedProperties(context.resolver).map( - (p) => new StructField(this.generator, p, this.parent), + (p) => new StructField(this.generator, p.prop, p.definingType), ); const assignments = liftedProps .map((p) => p.pythonName) @@ -764,8 +786,8 @@ abstract class BaseMethod implements PythonBase { ); } - private getLiftedProperties(resolver: TypeResolver): spec.Property[] { - const liftedProperties: spec.Property[] = []; + private getLiftedProperties(resolver: TypeResolver): PropertyDefinition[] { + const liftedProperties: PropertyDefinition[] = []; const stack = [this.liftedProp]; const knownIfaces = new Set(); @@ -794,7 +816,7 @@ abstract class BaseMethod implements PythonBase { if (knownProps.has(prop.name)) { continue; } - liftedProperties.push(prop); + liftedProperties.push({ prop, definingType: current }); knownProps.add(prop.name); } } @@ -1058,7 +1080,7 @@ class Struct extends BasePythonClassType { */ private get allMembers(): StructField[] { return this.thisInterface.allProperties.map( - (x) => new StructField(this.generator, x.spec, this.thisInterface), + (x) => new StructField(this.generator, x.spec, x.definingType.spec), ); } @@ -1126,6 +1148,7 @@ class Struct extends BasePythonClassType { const args: DocumentableArgument[] = this.allMembers.map((m) => ({ name: m.pythonName, docs: m.docs, + definingType: this.spec, })); this.generator.emitDocString(code, this.apiLocation, this.docs, { arguments: args, @@ -1199,7 +1222,7 @@ class StructField implements PythonBase { public constructor( private readonly generator: PythonGenerator, public readonly prop: spec.Property, - private readonly parent: spec.NamedTypeReference, + private readonly definingType: spec.Type, ) { this.pythonName = toPythonPropertyName(prop.name); this.jsiiName = prop.name; @@ -1208,7 +1231,11 @@ class StructField implements PythonBase { } public get apiLocation(): ApiLocation { - return { api: 'member', fqn: this.parent.fqn, memberName: this.jsiiName }; + return { + api: 'member', + fqn: this.definingType.fqn, + memberName: this.jsiiName, + }; } public get optional(): boolean { @@ -1265,11 +1292,12 @@ class Class extends BasePythonClassType implements ISortableType { public constructor( generator: PythonGenerator, name: string, + spec: spec.Type, fqn: string, opts: ClassOpts, docs: spec.Docs | undefined, ) { - super(generator, name, fqn, opts, docs); + super(generator, name, spec, fqn, opts, docs); const { abstract = false, interfaces = [], abstractBases = [] } = opts; @@ -2483,6 +2511,7 @@ class PythonGenerator extends Generator { const klass = new Class( this, toPythonIdentifier(cls.name), + cls, cls.fqn, { abstract, @@ -2625,6 +2654,7 @@ class PythonGenerator extends Generator { iface = new Struct( this, toPythonIdentifier(ifc.name), + ifc, ifc.fqn, { bases: ifc.interfaces?.map((base) => this.findType(base)) }, ifc.docs, @@ -2633,6 +2663,7 @@ class PythonGenerator extends Generator { iface = new Interface( this, toPythonIdentifier(ifc.name), + ifc, ifc.fqn, { bases: ifc.interfaces?.map((base) => this.findType(base)) }, ifc.docs, @@ -2684,7 +2715,7 @@ class PythonGenerator extends Generator { protected onBeginEnum(enm: spec.EnumType) { this.addPythonType( - new Enum(this, toPythonIdentifier(enm.name), enm.fqn, {}, enm.docs), + new Enum(this, toPythonIdentifier(enm.name), enm, enm.fqn, {}, enm.docs), ); } @@ -2800,6 +2831,7 @@ class PythonGenerator extends Generator { */ interface DocumentableArgument { name: string; + definingType: spec.Type; docs?: spec.Docs; }