From 15f77b50b646c924582c691137921ecd5cb741bc Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Fri, 12 Jul 2019 10:53:21 +0200 Subject: [PATCH] feat(jsii-diff): also check stability transitions (#592) 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. --- packages/jsii-diff/lib/classes-ifaces.ts | 9 +++++ packages/jsii-diff/lib/enums.ts | 5 +++ packages/jsii-diff/lib/stability.ts | 45 +++++++++++++++++++++ packages/jsii-diff/test/test.diagnostics.ts | 20 +++++++++ 4 files changed, 79 insertions(+) create mode 100644 packages/jsii-diff/lib/stability.ts diff --git a/packages/jsii-diff/lib/classes-ifaces.ts b/packages/jsii-diff/lib/classes-ifaces.ts index ef843dd3f8..5cc02148c7 100644 --- a/packages/jsii-diff/lib/classes-ifaces.ts +++ b/packages/jsii-diff/lib/classes-ifaces.ts @@ -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'; @@ -12,6 +13,8 @@ const LOG = log4js.getLogger('jsii-diff'); * present on the new type, and that they match in turn. */ export function compareReferenceType(original: T, updated: T, context: ComparisonContext) { + compareStabilities(original, updated, context); + if (original.isClassType() && updated.isClassType()) { if (updated.abstract && !original.abstract) { context.mismatches.report({ @@ -57,6 +60,8 @@ export function compareReferenceType(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. // @@ -98,6 +103,8 @@ function compareMethod( 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) { @@ -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({ diff --git a/packages/jsii-diff/lib/enums.ts b/packages/jsii-diff/lib/enums.ts index 2d6ee7a3b0..80c58b6014 100644 --- a/packages/jsii-diff/lib/enums.ts +++ b/packages/jsii-diff/lib/enums.ts @@ -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) { @@ -12,5 +15,7 @@ export function compareEnum(original: reflect.EnumType, updated: reflect.EnumTyp }); continue; } + + compareStabilities(origMember, updatedMember, context); } } \ No newline at end of file diff --git a/packages/jsii-diff/lib/stability.ts b/packages/jsii-diff/lib/stability.ts new file mode 100644 index 0000000000..65e089bd07 --- /dev/null +++ b/packages/jsii-diff/lib/stability.ts @@ -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}`); +} \ No newline at end of file diff --git a/packages/jsii-diff/test/test.diagnostics.ts b/packages/jsii-diff/test/test.diagnostics.ts index 5baf82128d..7e4c213039 100644 --- a/packages/jsii-diff/test/test.diagnostics.ts +++ b/packages/jsii-diff/test/test.diagnostics.ts @@ -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(); + }, + };