fix(jsii-diff): external structs returned from methods cannot be changed #2070

rix0rrr · 2020-10-02T15:38:36Z

We used to be checking the compatibility of structs in-line, as
a part of checking whether the call signature of a function is
compatible with its previous signature (i.e., checking that a
return type is only strengthened and an input type is only weakened).

This had a couple of downsides:

Performance: we would check the same type multiple times if it
occurred (potentially recursively) in multiple call signatures.
Locality: we would report the mismatch against the function signature
instead of the changed type.
Locality(2): if the change occurred in an external type but the
consuming function was stable, the error ought to be suppressed but
was reported instead.

The last reason recently cropped up in real life so that was a good
opportunity to rectify this long-standing architectural defect. I'm
afraid the result is a rather large refactoring; I apologize in advance,
but rest assured that everything will be better in the future, dear
reader.

New way of working:

First identify all types in the assembly
Crawl all types that have callables (methods, properties,
initializers) and identify the types they use and whether they
occur in input or output position.
Validate all types, taking extra care to structurally compare
structs based on whether they appear in input or output positions.

This only checks every struct once, report errors against the
struct itself and allows ignoring of structs that are marked external
or experimental.

While working on this change, it occurred to me that we were missing
a couple of validations that I added at the same time:

In a @subclassable type, no method or property types can be changed
at all (because C# won't allow it).
A mutable property's type cannot be changed at all (it can
both not be strengthened because it can be written, and not be weakened
because it can be read).

Fixes #2064.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

We used to be checking the compatibility of structs in-line, as a part of checking whether the call signature of a function is compatible with its previous signature (i.e., checking that a return type is only strengthened and an input type is only weakened). This had a couple of downsides: - Performance: we would check the same type multiple times if it occurred (deeply) in multiple call signatures. - Locality: we would report the mismatch against the function signature instead of the changed type. - Locality(2): if the change occurred in an `external` type but the consuming function was `stable`, the error ought to be suppressed but was reported instead. The last reason recently cropped up in real life so that was a good opportunity to rectify this long-standing architectural defect. I'm afraid the result is a rather large refactoring; I apologize in advance, but rest assured that everything will be better in the future, dear reader. New way of working: * First identify all types in the assembly * Crawl all types that have callables (methods, properties, initializers) and identify the types they use and whether they occur in input or output position. * Validate all types, taking extra care to structurally compare structs based on whether they appear in input or output positions. This only checks every struct once, report errors against the struct itself and allows ignoring of structs that are marked `external` or `experimental`. While working on this change, it occurred to me that we were missing a couple of validations that I added at the same time: - In a `@subclassable` type, no method or property types can be changed *at all* (because C# won't allow it). - A mutable property's type cannot be changed at all (it can both not be strengthened because it can be written, and not be weakened because it can be read). Fixes #2064.

rix0rrr · 2020-10-02T15:42:12Z

Implementation-wise:

old	what	new
`index.ts`	used to contain collection logic	moved to `type-comparison.ts`, is now OO
`classes-ifaces.ts`	used to contain rules logic	moved to `type-comparison.ts`, is now OO
"	and validation logic	moved to `validations.ts`
`enums.ts`	used to contain validation logic	moved to `validations.ts`
`type-analysis.ts`	used to do structural checking	structural checking moved to `type-comparison.ts` for structs

The rules inheritance looks a bit complicated still (and is made worse by the short line lengths imposed by prettier), I'm open to splitting those out more if the audience finds it bothersome in it's current form.

RomainMuller

I was about to express some concerns with how this focuses solely on the type uses within this assembly, but re-considered a couple of times and right now I'm thinking it makes sense for the purpose of this tool (@subclassable is how the external usage is gated/protected).

This looks great as always, and I only have pretty minimal comments at this stage (basically - minimal style tuning). I'll be happy to approve once you've had a chance to go through those and decide on which you want to take...

RomainMuller · 2020-10-05T09:24:19Z

packages/jsii-diff/test/classes.test.ts

+test.each([
+  [
+    'name: string',
+    'name?: string',
+    /type Optional<string> \(formerly string\): output type is now optional/,
+  ],
+  [
+    'name?: string',
+    'name: string',
+    undefined, // Strengthening is okay
+  ],
+  [
+    'name: string',
+    'name: string | number',
+    /string \| number is not assignable to string/,
+  ],
+  [
+    'name: string | number',
+    'name: string',
+    undefined, // Strengthening is okay
+  ],
+])('change class property ', (oldDecl, newDecl, error) =>


I kinda would have preferred using objects rather than tuples for this (since this is string-abundant).

Suggested change

test.each([

[

'name: string',

'name?: string',

/type Optional<string> $formerly string$: output type is now optional/,

],

[

'name?: string',

'name: string',

undefined, // Strengthening is okay

],

[

'name: string',

'name: string | number',

/string \| number is not assignable to string/,

],

[

'name: string | number',

'name: string',

undefined, // Strengthening is okay

],

])('change class property ', (oldDecl, newDecl, error) =>

test.each([

{

oldDecl: 'name: string',

newDecl: 'name?: string',

error: /type Optional<string> $formerly string$: output type is now optional/,

},

{

oldDecl: 'name?: string',

newDecl: 'name: string',

error: undefined, // Strengthening is okay

},

{

oldDecl: 'name: string',

newDecl: 'name: string | number',

error: /string \| number is not assignable to string/,

},

{

oldDecl: 'name: string | number',

newDecl: 'name: string',

error: undefined, // Strengthening is okay

},

])('change class property ', ({oldDecl, newDecl, error}) =>

The same applies to subsequent similar blocks.

Cool, never even considered to do this.

packages/jsii-diff/test/util.ts

RomainMuller · 2020-10-05T09:27:12Z

packages/jsii-diff/test/util.ts

+    await expectNoError(original, updated);
+    return;


I reckon this is equivalent (but saves 1 promise un-wrapping, then re-wrapping):

Suggested change

await expectNoError(original, updated);

return;

return expectNoError(original, updated);

I had that originally, but if I do that then eslint starts complaining about the regular return path:

{ "resource": "/Users/huijbers/Workspaces/PublicCDK/jsii/packages/jsii-diff/test/util.ts", "owner": "eslint", "code": { "value": "consistent-return", "target": { "$mid": 1, "external": "https://eslint.org/docs/rules/consistent-return", "path": "/docs/rules/consistent-return", "scheme": "https", "authority": "eslint.org" } }, "severity": 8, "message": "Expected to return a value at the end of async function 'expectError'.", "source": "eslint", "startLineNumber": 14, "startColumn": 23, "endLineNumber": 14, "endColumn": 23 }

And putting an empty return at the end of the function doesn't fix it.

Rather take the path of least resistance here and it's not like it's in a hot path or anything :).

Makes sense!

mergify · 2020-10-07T14:37:28Z

Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it!

mergify · 2020-10-07T14:38:04Z

Merging (with squash)...

aws-cdk-automation · 2020-10-07T14:54:58Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-Blkkw9bQFn8A
Commit ID: 6089cb9
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2020-10-07T15:56:55Z

Merging (with squash)...

rix0rrr requested review from RomainMuller, njlynch, shivlaks and a team October 2, 2020 15:38

rix0rrr self-assigned this Oct 2, 2020

mergify bot added the contribution/core This is a PR that came from AWS. label Oct 2, 2020

rix0rrr changed the title ~~fix(jsii-diff): allow modification of external structs~~ fix(jsii-diff): external structs returned from methods cannot be changed Oct 2, 2020

RomainMuller reviewed Oct 5, 2020

View reviewed changes

Review comments

a65a18f

rix0rrr requested a review from RomainMuller October 6, 2020 12:02

RomainMuller approved these changes Oct 7, 2020

View reviewed changes

RomainMuller added this to the October Release milestone Oct 7, 2020

rix0rrr removed request for njlynch and shivlaks October 7, 2020 14:36

mergify bot added the pr/ready-to-merge This PR is ready to be merged. label Oct 7, 2020

Merge branch 'master' into huijbers/diff-refactor

6089cb9

mergify bot merged commit 11e9389 into master Oct 7, 2020

mergify bot deleted the huijbers/diff-refactor branch October 7, 2020 15:56

mergify bot removed the pr/ready-to-merge This PR is ready to be merged. label Oct 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(jsii-diff): external structs returned from methods cannot be changed #2070

fix(jsii-diff): external structs returned from methods cannot be changed #2070

rix0rrr commented Oct 2, 2020 •

edited

Loading

rix0rrr commented Oct 2, 2020 •

edited

Loading

RomainMuller left a comment

RomainMuller Oct 5, 2020

rix0rrr Oct 6, 2020

RomainMuller Oct 5, 2020

rix0rrr Oct 6, 2020

RomainMuller Oct 7, 2020

mergify bot commented Oct 7, 2020

mergify bot commented Oct 7, 2020

aws-cdk-automation commented Oct 7, 2020

mergify bot commented Oct 7, 2020

	await expectNoError(original, updated);
	return;
	return expectNoError(original, updated);

fix(jsii-diff): external structs returned from methods cannot be changed #2070

fix(jsii-diff): external structs returned from methods cannot be changed #2070

Conversation

rix0rrr commented Oct 2, 2020 • edited Loading

rix0rrr commented Oct 2, 2020 • edited Loading

RomainMuller left a comment

Choose a reason for hiding this comment

RomainMuller Oct 5, 2020

Choose a reason for hiding this comment

rix0rrr Oct 6, 2020

Choose a reason for hiding this comment

RomainMuller Oct 5, 2020

Choose a reason for hiding this comment

rix0rrr Oct 6, 2020

Choose a reason for hiding this comment

RomainMuller Oct 7, 2020

Choose a reason for hiding this comment

mergify bot commented Oct 7, 2020

mergify bot commented Oct 7, 2020

aws-cdk-automation commented Oct 7, 2020

AWS CodeBuild CI Report

mergify bot commented Oct 7, 2020

rix0rrr commented Oct 2, 2020 •

edited

Loading

rix0rrr commented Oct 2, 2020 •

edited

Loading