Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(jsii-diff): also check stability transitions #592

Merged
merged 1 commit into from
Jul 12, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that the case where stability is inherited? What happens if you change the stability of the oRent module/type, is it inherited by the Jsii compiler! Can we evened even have an undefined stability for an api?

Also, wasn’t this what we wanted to do for the CFN layer?

Copy link
Contributor Author

@rix0rrr rix0rrr Jul 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inheritance has already been calculated at this level. I thought for the CFN layer we introduce a new explicit stability level (imported), not (nothing).

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();
},

};