Skip to content

Commit

Permalink
feat(jsii-diff): allow external stability to be treated as error (#4076)
Browse files Browse the repository at this point in the history
In certain situations one might want to treat changes to `external` APIs as errors.
This could be to check the external APIs for any breaking changes and have an opportunity to selectively reject, heal or soften an upstream change.

This change introduces a new cli option `--error-on` which can be one of three classes:

| `--error-on`       | Stabilities that cause an ERROR                    |
| ------------------ | -------------------------------------------------- |
| `prod` (default)   | `stable`, `deprecated`                             |
| `non-experimental` | `stable`, `deprecated`, `external`                 |
| `all`              | `stable`, `deprecated`, `experimental`, `external` |

**Fixes `deprecated` APIs not being treated as `stable`.**
In jsii, deprecations are treated as a stability.
However for the purpose of jsii-diff they should be treated as stable.
Otherwise one could do `stable -> deprecated, make breaking change, deprecated -> stable`, which should not be allowed. 
We also can't prohibit the transition from deprecated back to stable, as it's perfectly okay to un-deprecate an API.

---

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
mrgrain committed May 10, 2023
1 parent 4fd596a commit 95689ea
Show file tree
Hide file tree
Showing 4 changed files with 185 additions and 61 deletions.
67 changes: 45 additions & 22 deletions packages/jsii-diff/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,74 +2,97 @@

__jsii-diff__ compares two jsii assemblies for compatibility.

In the future, it will be able to do generic comparisons, but for
In the future, it will be able to do generic comparisons.
But for
now it will compare assemblies for API compatibility, and exit
with a non-zero exit code if any **stable** APIs have had incompatible
changes.
with a non-zero exit code if any __stable__ or __deprecated__ APIs have had incompatible changes.

API items that have no stability are treated as **stable**. To treat
unmarked API items as experimental, pass the `--default-experimental` flag.
API items that have no stability are treated as __stable__.
To treat unmarked API items as experimental, pass the `--default-experimental` flag.

## Usage

To compare two JSII packages:

jsii-diff <old> [new]
```console
jsii-diff <old> [new]
```

Packages can be identified by either:

* **A path**, in which case it should be the path to a JSII package directory,
* __A path__, in which case it should be the path to a JSII package directory,
or to a `.jsii` file.
* **An NPM package specifier** of the form `npm:[<package>[@version]]`, in
* __An NPM package specifier__ of the form `npm:[<package>[@version]]`, in
which case the indicated version is downloaded and used. If `@version` is
left out, the latest version will be used. If `package` is left out,
the assembly name of `.jsii` in the current directory will be used.

To compare current package against latest published NPM release:

jsii-diff npm:
```console
jsii-diff npm:<package>
```

### Stability Error Classes

By default only incompatible changes to `stable` or `deprecated` APIs are treated as errors and will fail the command.
Changes to `experimental` or `external` APIs emit a warning.

Change this behavior with the `--error-on` flag:

```console
jsii-diff npm:<package> --error-on=all
```

The following `--error-on` groups are available:

| `--error-on` | Stabilities that cause an ERROR |
| ------------------ | -------------------------------------------------- |
| `prod` (default) | `stable`, `deprecated` |
| `non-experimental` | `stable`, `deprecated`, `external` |
| `all` | `stable`, `deprecated`, `experimental`, `external` |

## Details

__jsii-diff__ will assert that code written against version **A** of a library
will still typecheck when compiled against version **B** of that library. It
__jsii-diff__ will assert that code written against version __A__ of a library
will still typecheck when compiled against version __B__ of that library. It
does this by verifying the following properties:

- Any type (class/interface/enum) in **A** must also exist in **B**.
- Enums have only added members.
- Classes and interfaces have only added members, or modified existing
* Any type (class/interface/enum) in __A__ must also exist in __B__.
* Enums have only added members.
* Classes and interfaces have only added members, or modified existing
members in an allowed way.
- Property types are the same or have been strengthened (see below).
- Methods have only added optional arguments, existing argument types have
* Property types are the same or have been strengthened (see below).
* Methods have only added optional arguments, existing argument types have
only been weakened, and the return type has only been strengthened (see below).

### Strengthening and weakening

- *Strengthening* a type refers to *excluding* more possible values. Changing
* *Strengthening* a type refers to *excluding* more possible values. Changing
a field from `optional` to `required`, or changing a type from `any` to
`string` are examples of strengthening.

- As the opposite of strengthening, *weakening* refers to *allowing* more
* As the opposite of strengthening, *weakening* refers to *allowing* more
possible values. Changing a field from `required` to `optional`, or
changing a type to a superclass or interface are examples of weakening.

An API can change in the following way without breaking its consumer:

- It can *weaken* its input (require *less* from the caller); and
- It can *strengthen* its output (guarantee *more* to the caller).
* It can *weaken* its input (require *less* from the caller); and
* It can *strengthen* its output (guarantee *more* to the caller).

### Struct types

Structs (interfaces consisting completely of `readonly` properties) are
treated as bags of data. Their API compatibility will be evaluated depending
on whether they appear in input or output position of operations.

- Structs are *weakened* if all types of all of its properties are weakened.
* Structs are *weakened* if all types of all of its properties are weakened.
Normally removing properties would also be considered weakening, but
because that may cause references to the fields in existing code bases to
become undefined (which is not allowed in most programming languages) we
disallow removing properties.
- Structs are *strengthened* if all types of all of its properties are
* Structs are *strengthened* if all types of all of its properties are
strengthened, or if fields are added.

__jsii-diff__ will check the evolution of structs against their position
Expand Down
14 changes: 12 additions & 2 deletions packages/jsii-diff/bin/jsii-diff.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,11 @@ import * as yargs from 'yargs';
import { compareAssemblies } from '../lib';
import {
classifyDiagnostics,
treatAsError,
formatDiagnostic,
hasErrors,
ErrorClass,
ERROR_CLASSES,
} from '../lib/diagnostics';
import {
DownloadFailure,
Expand Down Expand Up @@ -42,6 +45,13 @@ async function main(): Promise<number> {
type: 'boolean',
default: false,
desc: 'Error on experimental API changes',
deprecate: 'Use `--error-on` instead',
})
.option('error-on', {
type: 'string',
default: 'prod',
choices: ERROR_CLASSES,
desc: 'Which type of API changes should be treated as an error',
})
.option('ignore-file', {
alias: 'i',
Expand Down Expand Up @@ -119,15 +129,15 @@ async function main(): Promise<number> {
if (mismatches.count > 0) {
const diags = classifyDiagnostics(
mismatches,
argv['experimental-errors'],
treatAsError(argv['error-on'] as ErrorClass, argv['experimental-errors']),
await loadFilter(argv['ignore-file']),
);

process.stderr.write(
`Original assembly: ${original.name}@${original.version}\n`,
);
process.stderr.write(
`Updated assembly: ${updated.name}@${updated.version}\n`,
`Updated assembly: ${updated.name}@${updated.version}\n`,
);
process.stderr.write('API elements with incompatible changes:\n');
for (const diag of diags) {
Expand Down
57 changes: 48 additions & 9 deletions packages/jsii-diff/lib/diagnostics.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,57 @@ export function hasErrors(diags: Diagnostic[]) {
return diags.some((diag) => diag.level === DiagLevel.Error);
}

export function onlyErrors(diags: Diagnostic[]) {
return diags.filter((diag) => diag.level === DiagLevel.Error);
}

export function onlyWarnings(diags: Diagnostic[]) {
return diags.filter((diag) => diag.level === DiagLevel.Warning);
}

export const ERROR_CLASSES = ['prod', 'non-experimental', 'all'] as const;

export type ErrorClass = (typeof ERROR_CLASSES)[number];

export const ERROR_CLASSES_TO_STABILITIES: Record<ErrorClass, Stability[]> = {
prod: [Stability.Stable, Stability.Deprecated],
'non-experimental': [
Stability.Stable,
Stability.Deprecated,
Stability.External,
],
all: [
Stability.Stable,
Stability.Experimental,
Stability.External,
Stability.Deprecated,
],
};

export function treatAsError(
errorClass: ErrorClass,
deprecatedExperimentalErrors = false,
): Set<Stability> {
const shouldError = new Set<Stability>();

for (const stability of ERROR_CLASSES_TO_STABILITIES[errorClass]) {
shouldError.add(stability);
}

if (deprecatedExperimentalErrors) {
shouldError.add(Stability.Experimental);
}

return shouldError;
}

/**
* Classify API mismatches into a set of warnings and errors
*/
export function classifyDiagnostics(
mismatches: Mismatches,
experimentalErrors: boolean,
skipFilter: Set<string>,
shouldError: Set<Stability>,
skipFilter: Set<string> = new Set(),
): Diagnostic[] {
const ret = mismatches.mismatches.map((mis) => ({
level: level(mis),
Expand All @@ -56,12 +100,7 @@ export function classifyDiagnostics(
if (skipFilter.has(mis.violationKey)) {
return DiagLevel.Skipped;
}
if (
mis.stability === Stability.Stable ||
(mis.stability === Stability.Experimental && experimentalErrors)
) {
return DiagLevel.Error;
}
return DiagLevel.Warning;

return shouldError.has(mis.stability) ? DiagLevel.Error : DiagLevel.Warning;
}
}

0 comments on commit 95689ea

Please sign in to comment.