Skip to content

Commit

Permalink
feat(jsii-diff): also check stability transitions (#592)
Browse files Browse the repository at this point in the history
Once an API element is marked @stable, it is not allowed to
retract that stability guarantee anymore by changing it back to
@experimental. This is now verified by `jsii-diff`.

This was always intended to be checked, but was missed as an oversight
in the initial implementation.
  • Loading branch information
rix0rrr authored Jul 12, 2019
1 parent 17338fd commit 15f77b5
Show file tree
Hide file tree
Showing 4 changed files with 79 additions and 0 deletions.
9 changes: 9 additions & 0 deletions packages/jsii-diff/lib/classes-ifaces.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import reflect = require('jsii-reflect');
import log4js = require('log4js');
import { compareStabilities } from './stability';
import { Analysis, FailedAnalysis, isSuperType } from './type-analysis';
import { ComparisonContext } from './types';

Expand All @@ -12,6 +13,8 @@ const LOG = log4js.getLogger('jsii-diff');
* present on the new type, and that they match in turn.
*/
export function compareReferenceType<T extends reflect.ReferenceType>(original: T, updated: T, context: ComparisonContext) {
compareStabilities(original, updated, context);

if (original.isClassType() && updated.isClassType()) {
if (updated.abstract && !original.abstract) {
context.mismatches.report({
Expand Down Expand Up @@ -57,6 +60,8 @@ export function compareReferenceType<T extends reflect.ReferenceType>(original:
}

export function compareStruct(original: reflect.InterfaceType, updated: reflect.InterfaceType, context: ComparisonContext) {
compareStabilities(original, updated, context);

// We don't compare structs here; they will be evaluated for compatibility
// based on input and output positions.
//
Expand Down Expand Up @@ -98,6 +103,8 @@ function compareMethod<T extends (reflect.Method | reflect.Initializer)>(
original: T,
updated: T,
context: ComparisonContext) {
compareStabilities(original, updated, context);

// Type guards on original are duplicated on updated to help tsc... They are required to be the same type by the declaration.
if (reflect.isMethod(original) && reflect.isMethod(updated)) {
if (original.static !== updated.static) {
Expand Down Expand Up @@ -201,6 +208,8 @@ function findParam(parameters: reflect.Parameter[], i: number): reflect.Paramete
}

function compareProperty(original: reflect.Property, updated: reflect.Property, context: ComparisonContext) {
compareStabilities(original, updated, context);

if (original.static !== updated.static) {
// tslint:disable-next-line:max-line-length
context.mismatches.report({
Expand Down
5 changes: 5 additions & 0 deletions packages/jsii-diff/lib/enums.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
import reflect = require('jsii-reflect');
import { compareStabilities } from './stability';
import { ComparisonContext } from './types';

export function compareEnum(original: reflect.EnumType, updated: reflect.EnumType, context: ComparisonContext) {
compareStabilities(original, updated, context);

for (const origMember of original.members) {
const updatedMember = updated.members.find(m => m.name === origMember.name);
if (!updatedMember) {
Expand All @@ -12,5 +15,7 @@ export function compareEnum(original: reflect.EnumType, updated: reflect.EnumTyp
});
continue;
}

compareStabilities(origMember, updatedMember, context);
}
}
45 changes: 45 additions & 0 deletions packages/jsii-diff/lib/stability.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
import reflect = require('jsii-reflect');
import spec = require('jsii-spec');
import { ApiElement, ComparisonContext } from './types';

export function compareStabilities(original: reflect.Documentable & ApiElement, updated: reflect.Documentable, context: ComparisonContext) {
// Nothing to do in these cases
if (original.docs.stability === undefined || original.docs.stability === updated.docs.stability) { return; }

// Not allowed to disavow stability
if (updated.docs.stability === undefined) {
context.mismatches.report({
ruleKey: 'removed-stability',
message: `stability was '${original.docs.stability}', has been removed`,
violator: original,
});
return;
}

const allowed = allowedTransitions(original.docs.stability);
if (!allowed.includes(updated.docs.stability)) {
context.mismatches.report({
ruleKey: 'changed-stability',
message: `stability not allowed to go from '${original.docs.stability}' to '${updated.docs.stability}'`,
violator: original,
});
}
}

function allowedTransitions(start: spec.Stability): spec.Stability[] {
switch (start) {
// Experimental can go to stable or be deprecated
case spec.Stability.Experimental:
return [spec.Stability.Stable, spec.Stability.Deprecated];

// Stable can be deprecated
case spec.Stability.Stable:
return [spec.Stability.Deprecated];

// Deprecated can be reinstated
case spec.Stability.Deprecated:
return [spec.Stability.Stable];
}

throw new Error(`Unrecognized stability: ${start}`);
}
20 changes: 20 additions & 0 deletions packages/jsii-diff/test/test.diagnostics.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,4 +56,24 @@ export = {
test.done();
},

// ----------------------------------------------------------------------
async 'changing stable to experimental is breaking'(test: Test) {
const mms = await compare(`
/** @stable */
export class Foo1 { }
`, `
/** @experimental */
export class Foo1 { }
`);

const experimentalErrors = false;
const diags = classifyDiagnostics(mms, experimentalErrors, new Set());

test.ok(diags.length > 0);
test.ok(diags.some(d => d.message.match(/stability not allowed to go from 'stable' to 'experimental'/)));
test.equals(true, hasErrors(diags));

test.done();
},

};

0 comments on commit 15f77b5

Please sign in to comment.