Skip to content

Commit

Permalink
fix: deprecation warnings are generated even when one property is not…
Browse files Browse the repository at this point in the history 
… deprecated (#3157)

This happens when:
1. An interface inherits from at least two other interfaces.
2. Both super-interfaces have a property with the same name.
3. The property is deprecated in one, but not the other.

The solution here is to inline the `print` calls for properties coming from the super types, instead of calling the functions that handle those super types. If there is a conflict (i.e. item 3 above), then the warning is suppressed for that property.

In terms of implementation, this is achieved by recursively calling the function that processes the warnings for interface types. Every property that is found is added to a map and non-deprecated properties are added to an exclusion list. At the end, properties that are in the exclusion list are removed from the final statements list.

---

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
@otaviomacedo
otaviomacedo authored Nov 11, 2021
1 parent ca04dad commit e566f37
Show file tree
Hide file tree
Showing 2 changed files with 172 additions and 101 deletions.
196 changes: 106 additions & 90 deletions packages/jsii/lib/transforms/deprecation-warnings.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,97 +74,17 @@ export class DeprecationWarningsInjector {
}
}
} else if (spec.isInterfaceType(type) && type.datatype) {
for (const prop of Object.values(type.properties ?? {})) {
if (spec.isDeprecated(prop) || spec.isDeprecated(type)) {
// If the property individually is deprecated, or the entire type is deprecated
const deprecatedDocs =
prop.docs?.deprecated ?? type.docs?.deprecated;
statements.push(
createWarningFunctionCall(
`${type.fqn}#${prop.name}`,
deprecatedDocs,
ts.createIdentifier(`"${prop.name}" in ${PARAMETER_NAME}`),
),
);
isEmpty = false;
}

if (
spec.isNamedTypeReference(prop.type) &&
Object.keys(types).includes(prop.type.fqn)
) {
const functionName = importedFunctionName(
prop.type.fqn,
assembly,
projectInfo,
);
if (functionName) {
statements.push(
createTypeHandlerCall(
functionName,
`${PARAMETER_NAME}.${prop.name}`,
),
);
isEmpty = false;
}
} else if (
spec.isCollectionTypeReference(prop.type) &&
spec.isNamedTypeReference(prop.type.collection.elementtype)
) {
const functionName = importedFunctionName(
prop.type.collection.elementtype.fqn,
assembly,
projectInfo,
);
if (functionName) {
statements.push(
createTypeHandlerCall(
functionName,
`${PARAMETER_NAME}.${prop.name}`,
),
);
isEmpty = false;
}
} else if (
spec.isUnionTypeReference(prop.type) &&
spec.isNamedTypeReference(prop.type.union.types[0]) &&
Object.keys(types).includes(prop.type.union.types[0].fqn)
) {
const functionName = importedFunctionName(
prop.type.union.types[0].fqn,
assembly,
projectInfo,
);
if (functionName) {
statements.push(
createTypeHandlerCall(
functionName,
`${PARAMETER_NAME}.${prop.name}`,
),
);
isEmpty = false;
}
}
const { statementsByProp, excludedProps } = processInterfaceType(
type,
types,
assembly,
projectInfo,
);

// We also generate calls to all the supertypes
for (const iface of type.interfaces ?? []) {
const functionName = importedFunctionName(
iface,
assembly,
projectInfo,
);
if (functionName) {
statements.push(
ts.createExpressionStatement(
ts.createCall(
ts.createIdentifier(functionName),
[],
[ts.createIdentifier(PARAMETER_NAME)],
),
),
);
isEmpty = false;
}
for (const [name, statement] of statementsByProp.entries()) {
if (!excludedProps.has(name)) {
statements.push(statement);
isEmpty = false;
}
}
}
Expand Down Expand Up @@ -232,6 +152,102 @@ export class DeprecationWarningsInjector {
}
}

function processInterfaceType(
type: spec.InterfaceType,
types: { [p: string]: spec.Type },
assembly: Assembly,
projectInfo: ProjectInfo,
statementsByProp: Map<string, Statement> = new Map<string, ts.Statement>(),
excludedProps: Set<string> = new Set<string>(),
) {
for (const prop of Object.values(type.properties ?? {})) {
const fqn = `${type.fqn}#${prop.name}`;
if (spec.isDeprecated(prop) || spec.isDeprecated(type)) {
// If the property individually is deprecated, or the entire type is deprecated
const deprecatedDocs = prop.docs?.deprecated ?? type.docs?.deprecated;
const statement = createWarningFunctionCall(
fqn,
deprecatedDocs,
ts.createIdentifier(`"${prop.name}" in ${PARAMETER_NAME}`),
);
statementsByProp.set(prop.name, statement);
} else {
/* If a prop is not deprecated, we don't want to generate a warning for it,
even if another property with the same name is deprecated in another
super-interface. */
excludedProps.add(prop.name);
}

if (
spec.isNamedTypeReference(prop.type) &&
Object.keys(types).includes(prop.type.fqn)
) {
const functionName = importedFunctionName(
prop.type.fqn,
assembly,
projectInfo,
);
if (functionName) {
const statement = createTypeHandlerCall(
functionName,
`${PARAMETER_NAME}.${prop.name}`,
);
statementsByProp.set(`${prop.name}_`, statement);
}
} else if (
spec.isCollectionTypeReference(prop.type) &&
spec.isNamedTypeReference(prop.type.collection.elementtype)
) {
const functionName = importedFunctionName(
prop.type.collection.elementtype.fqn,
assembly,
projectInfo,
);
if (functionName) {
const statement = createTypeHandlerCall(
functionName,
`${PARAMETER_NAME}.${prop.name}`,
);
statementsByProp.set(`${prop.name}_`, statement);
}
} else if (
spec.isUnionTypeReference(prop.type) &&
spec.isNamedTypeReference(prop.type.union.types[0]) &&
Object.keys(types).includes(prop.type.union.types[0].fqn)
) {
const functionName = importedFunctionName(
prop.type.union.types[0].fqn,
assembly,
projectInfo,
);
if (functionName) {
const statement = createTypeHandlerCall(
functionName,
`${PARAMETER_NAME}.${prop.name}`,
);
statementsByProp.set(`${prop.name}_`, statement);
}
}
}

// We also generate calls to all the supertypes
for (const interfaceName of type.interfaces ?? []) {
const assemblies = projectInfo.dependencyClosure.concat(assembly);
const superType = findType(interfaceName, assemblies);
if (superType.type) {
processInterfaceType(
superType.type as spec.InterfaceType,
types,
assembly,
projectInfo,
statementsByProp,
excludedProps,
);
}
}
return { statementsByProp, excludedProps };
}

function fnName(fqn: string): string {
return fqn.replace(/[^\w\d]/g, '_');
}
Expand Down
77 changes: 66 additions & 11 deletions packages/jsii/test/deprecation-warnings.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -195,28 +195,79 @@ function testpkg_Baz(p) {
`);
});

test('generates calls for supertypes', async () => {
test('generates calls for deprecated inherited properties', async () => {
const result = await compileJsiiForTest(
`
export interface Foo {}
export interface Bar {readonly foo: Foo;}
export interface Baz extends Bar {readonly x: string;}
export interface Baz {
/** @deprecated message from Baz */
readonly x: string;
}
export interface Bar {
/** @deprecated message from Bar */
readonly x: string;
}
export interface Foo extends Bar, Baz {
}
`,
undefined /* callback */,
{ addDeprecationWarnings: true },
);

expect(jsFile(result, '.warnings.jsii')).toMatch(`function testpkg_Baz(p) {
const warningsFileContent = jsFile(result, '.warnings.jsii');

// For each supertype, its corresponding function should be generated, as usual
expect(warningsFileContent).toMatch(`function testpkg_Baz(p) {
if (p == null)
return;
visitedObjects.add(p);
testpkg_Bar(p);
if ("x" in p)
print("testpkg.Baz#x", "message from Baz");
visitedObjects.delete(p);
}
`);
}`);
expect(warningsFileContent).toMatch(`function testpkg_Bar(p) {
if (p == null)
return;
visitedObjects.add(p);
if ("x" in p)
print("testpkg.Bar#x", "message from Bar");
visitedObjects.delete(p);
}`);

// But a call for one of the instances of the property should also be generated in the base function
expect(warningsFileContent).toMatch(`function testpkg_Foo(p) {
if (p == null)
return;
visitedObjects.add(p);
if ("x" in p)
print("testpkg.Baz#x", "message from Baz");
visitedObjects.delete(p);
}`);
});

test('skips properties that are deprecated in one supertype but not the other', async () => {
const result = await compileJsiiForTest(
`
export interface Baz {
readonly x: string;
}
export interface Bar {
/** @deprecated message from Bar */
readonly x: string;
}
export interface Foo extends Bar, Baz {
}
`,
undefined /* callback */,
{ addDeprecationWarnings: true },
);

const warningsFileContent = jsFile(result, '.warnings.jsii');

expect(warningsFileContent).toMatch(`function testpkg_Foo(p) {
}`);
});

test('generates calls for tyes with deprecated properties', async () => {
test('generates calls for types with deprecated properties', async () => {
const result = await compileJsiiForTest(
`
export interface Bar {
Expand Down Expand Up @@ -274,9 +325,13 @@ function testpkg_Baz(p) {
});

test('generates calls for types in other assemblies', async () => {
const calcBaseOfBaseRoot = resolveModuleDir(
'@scope/jsii-calc-base-of-base',
);
const calcBaseRoot = resolveModuleDir('@scope/jsii-calc-base');
const calcLibRoot = resolveModuleDir('@scope/jsii-calc-lib');

await compile(calcBaseOfBaseRoot, false);
await compile(calcBaseRoot, true);
await compile(calcLibRoot, true);
const warningsFile = loadWarningsFile(calcBaseRoot);
Expand All @@ -290,11 +345,11 @@ function testpkg_Baz(p) {
// Recompiling without deprecation warning to leave the packages in a clean state
await compile(calcBaseRoot, false);
await compile(calcLibRoot, false);
}, 25000);
}, 30000);
});

describe('Call injections', () => {
test('does not add warnings, by default', async () => {
test('does not add warnings by default', async () => {
const result = await compileJsiiForTest(
`
export class Foo {
Expand Down

0 comments on commit e566f37

Please sign in to comment.