feat(jsii-spec): Model parameter optionality #432

RomainMuller · 2019-04-04T10:45:58Z

Method Parameters, Properties and method return types
now carry an optional flag that indicates whether they are
optional or required, and the TypeReference#optional field
was removed.

BREAKING CHANGE: JSII assemblies generated by older versions of the tool
will fail loading with this new version, and vice-versa. Re-compile your
projects in order to fix this.

Fixes #296
Fixes #414

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

Method `Parameters` now carry an `optional` flag that indicates whether they are optional or required, and the `TypeReference#optional` field was renamed to `TypeReference#nullable` to better reflect its semantics. This also brings more flexibility in that it is now possible to model a method with a nullable or defaulted argument that is followed by some non-optional argument, and still obtain a reasonable type specification, where previously this was an error. Finally, in order to better reflect the type model of TypeScript and Javascript, all `any` type references are now denoted `nullable`. BREAKING CHANGE: JSII assemblies generated by older versions of the tool will fail loading with this new version, and vice-versa. Re-compile your projects in order to fix this. Fixes #296 Fixes #414

RomainMuller · 2019-04-04T10:47:01Z

Note that I decided against modeling optional properties, because it is essentially meaningless... A property can be considered optional as long as it's type is nullable.

packages/jsii-kernel/lib/kernel.ts

packages/jsii-kernel/lib/serialization.ts

packages/jsii-pacmak/lib/targets/python.ts

packages/jsii-spec/lib/spec.ts

packages/jsii/lib/assembler.ts

rix0rrr · 2019-04-04T11:34:05Z

It slightly concerns me that there are breaking changes in the assembly model. Do we know for a fact users will always use the new kernel to load the new assemblies?

Don't we have a version in the assembly metadata? Did you bump that, and does the kernel actually check the version it's expecting?

…ameter-optionality

RomainMuller · 2019-04-08T07:19:39Z

@rix0rrr new revision should have addressed all of your comments as well, and sets the Schema version of jsii/0.9.0 (which I probably should flip to 0.10.0). I wonder if I should get this to be expressive of a minimum version requirement, instead of looking like a fixed version... (can be a "for later" thing as well, but I guess if anyone has ideas...)

rix0rrr · 2019-04-08T08:13:47Z

packages/jsii-calc-base-of-base/test/assembly.jsii

@@ -16,7 +16,7 @@
    "type": "git",
    "url": "https://github.com/awslabs/jsii.git"
  },
-  "schema": "jsii/1.0",
+  "schema": "jsii/0.9.0",


Waitaminit! I don't like how this is version number is going backwards, and we already have a jsiiVersion field that encodes the JSII version. Why can't we just discretely bump the schema versions 1 -> 2 -> 3 etc? Saves us having to do the whole "what's the next release version" dance, too.

I don't really care. I kinda felt it was nice to correlate that with the jsii version so you can, from an error (like "Whoosh the version is jsii/0.9.0, but I expected to find jsii/0.42.12), determine what you need to do. I however would only update that version number if/when the spec gets a breaking change (so it acts as a "minimum required version" of sorts.

packages/jsii-spec/lib/spec.ts

rix0rrr · 2019-04-08T08:18:11Z

packages/jsii-spec/lib/spec.ts

+/**
+ * A reference to a type (primitive, collection or fqn).
+ */
+export type TypeReference = NamedTypeReference | PrimitiveTypeReference | CollectionTypeReference | UnionTypeReference;


I support this!

rix0rrr · 2019-04-08T08:19:48Z

packages/jsii-spec/lib/spec.ts

 */
-export type TypeReference = TypeReferenceBase & (NamedTypeReference | PrimitiveTypeReference | CollectionTypeReference | UnionTypeReference);
+export interface TypeInstance<T extends TypeReference> {


I think I'm going to say that this interface ony makes sense for return types, and I think I object to its use in other places.

rix0rrr · 2019-04-08T08:20:44Z

packages/jsii-spec/lib/spec.ts

@@ -390,7 +399,7 @@ export interface CollectionTypeReference extends TypeReferenceBase {
        /**
         * The type of an element (map keys are always strings).
         */
-        elementtype: TypeReference;
+        elementtype: TypeInstance<TypeReference>;


It does not make sense for either array or map elements to be of type Promise<T>, or optional.

So an array may never contain null? Or it'll contain "surprise" nulls?

But that is something else though! Feels like now we're expressing nullability, not optionality.

rix0rrr · 2019-04-08T08:23:28Z

packages/jsii-spec/lib/spec.ts

@@ -409,7 +418,7 @@ export interface UnionTypeReference extends TypeReferenceBase {
         * All the possible types (including the primary type).
         * @minItems 2
         */
-        types: TypeReference[];
+        types: Array<TypeInstance<TypeReference>>;


It does not make sense for a union type to be something like number | string? or number | Promise<string>.

And yet, this is valid TypeScript, and thus could be authored in a TypeScript signature that JSII would totally parse out in that way

Nobody says we can't reject that in jsii. Will the kernel handle it correctly?

rix0rrr · 2019-04-08T08:24:03Z

packages/jsii-spec/lib/spec.ts

     */
-    type: TypeReference;
+    value: TypeInstance<TypeReference>;


A parameter can never be a Promise<T>. It can be optional, however.

// In fact, totally possible: async function impossible(arg: Promise<Foo>): Promise<Bar>;

Sure. What I meant to say is that the kernel does not have adequate provisions to handle this, so we shouldn't allow it either.

rix0rrr · 2019-04-08T08:32:03Z

packages/jsii-spec/lib/spec.ts

@@ -508,7 +521,7 @@ export interface Method extends Documentable, Overridable, SourceLocatable {
    /**
     * The return type of the method (undefined if void or initializer)
     */
-    returns?: TypeReference;
+    returns?: TypeInstance<TypeReference>;


This is the only place where TypeInstance makes sense to me.

Although I think promise?: boolean makes more sense as an attribute of Method to me (async?: boolean) than as an attribute of the return type.

rix0rrr · 2019-04-08T08:33:28Z

packages/jsii-spec/lib/spec.ts

     */
-    type: TypeReference;
+    value: TypeInstance<TypeReference>;


Properties cannot be promise. They can be optional however.

I can totes declare a public readonly iSwearItWillHaveAValueAtSomePoint: Promise<string>. If that's an API I like? No. But it can be made (and JSII will happily generate a promise: true for that type reference.

See my previous comment, I'm not sure the kernel will handle that situation properly.

rix0rrr · 2019-04-08T08:36:50Z

I think I'd prefer a modeling like this:

interface OptionalValue {
    optional?: boolean;
    type: TypeReference;
}

interface Method {
    async?: boolean;
    returns?: OptionalValue;
    // ...
}

interface Property extends OptionalValue {
    // ...
}

interface Parameter extends OptionalValue {
    // ...
}

interface Collection {
    element: TypeReference;
    // ...
}

eladb · 2019-04-08T18:34:59Z

packages/jsii-calc-base-of-base/test/assembly.jsii

-      "initializer": {
-        "initializer": true
-      },
+      "initializer": {},


What happened here? We shouldn't use an empty object to represent something...

Empty object here is the default initializer (no parameters, public). I thought of also modeling that as "true" or the literal string "default" instead... but the empty object actually describes what I want, based on its default values... so I figured I wouldn't overthink it.

I understand where this is coming from, but still don't like this as a general modeling principle. Can we do better?

eladb · 2019-04-08T18:39:31Z

packages/jsii-calc/test/assembly.jsii

@@ -469,6 +449,7 @@
            {
              "name": "inp",
              "type": {
+                "optional": true,


I don't believe this parameter is optional

eladb · 2019-04-08T18:46:03Z

packages/jsii/lib/assembler.ts

@@ -204,22 +203,26 @@ export class Assembler implements Emitter {
   *
   * @returns the de-referenced type, if it was found, otherwise ``undefined``.
   */
-  private _dereference(ref: spec.NamedTypeReference, referencingNode: ts.Node | null): spec.Type | undefined {
-    const [assm, ] = ref.fqn.split('.');
+  private _dereference(ref: string | spec.NamedTypeReference, referencingNode: ts.Node | null): spec.Type | undefined {


For readability, I would create a type alias spec.FullyQualifiedName = string

Oh boy. That alias even exists! I can just use it :)

eladb · 2019-04-08T18:46:39Z

packages/jsii/lib/assembler.ts

-    const [assm, ] = ref.fqn.split('.');
+  private _dereference(ref: string | spec.NamedTypeReference, referencingNode: ts.Node | null): spec.Type | undefined {
+    if (typeof ref !== 'string') {
+      ref = ref.fqn;


error if ref.fqn is undefined?

eladb · 2019-04-08T18:49:32Z

packages/jsii/lib/assembler.ts

@@ -555,17 +557,17 @@ export class Assembler implements Emitter {

      // tslint:disable-next-line:no-bitwise
      if ((ts.getCombinedModifierFlags(ctorDeclaration) & ts.ModifierFlags.Private) === 0) {
-        jsiiType.initializer = { initializer: true };
+        jsiiType.initializer = {};


When I have a method, how do I know if it's an initializer?

You have an initializer, not a method. Methods have a name (guaranteed). Initializers don't (guaranteed).

eladb · 2019-04-08T18:58:46Z

packages/jsii-spec/lib/spec.ts

+/**
+ * Fully Qualified Name
+ */
+export type FQN = string;


Oh, there we go!

eladb · 2019-04-08T19:01:04Z

packages/jsii-spec/lib/spec.ts

 */
-export interface TypeReferenceBase {
+export const CANONICAL_ANY: Readonly<PrimitiveTypeReference> = { optional: true, primitive: PrimitiveType.Any };


Why does any imply optional? It implies nullable, but that's a different thing.

I guess I think of optional as allowing "no value" through, and any also allows "no value" though. But I guess I see your point still...

eladb · 2019-04-08T19:01:24Z

packages/jsii-spec/lib/spec.ts

+/**
+ * Qualifiers for a type reference.
+ */
+interface QualifiedTypeReference {


Maybe TypeReferenceQualifiers, because this is not a type reference.

eladb · 2019-04-08T19:07:00Z

packages/jsii-pacmak/test/expected.jsii-calc/python/src/jsii_calc/__init__.py

@@ -58,7 +58,7 @@ def __init__(self) -> None:
        jsii.create(AllTypes, self, [])

    @jsii.member(jsii_name="anyIn")
-    def any_in(self, inp: typing.Any) -> None:
+    def any_in(self, inp: typing.Any=None) -> None:


That's wrong... any does not imply optionality

RomainMuller · 2019-04-08T19:28:46Z

@rix0rrr well then you seem in vivid disagreement with @eladb and I am certainly not re-doing this whole thing a fourth time unless we are 100% certain the 4th is the right one and it has significant benefits over this... this is a full day of painful delving into C# failing tests each time...

eladb · 2019-04-08T19:30:48Z

@RomainMuller what disagreement?

RomainMuller · 2019-04-08T19:41:11Z

@eladb isn't what @rix0rrr "would prefer" a minor variation over the previous iteration , which I would then literally have wasted a whole day redoing all over again?

RomainMuller · 2019-04-08T19:49:24Z

I want to be able to express:

optional result
optional parameter (as in can be undefined)
optional value in collection (list/map)

rix0rrr · 2019-04-09T13:35:23Z

We all sort of agree on optionality, I feel.

What we have left to do is decide on whether nullability is a thing or not. In line with your first iteration, my intuition is yes, nullability is a thing. Elad disagreed, and now we're going back and forth.

What we couldn't express before is the guaranteed PRESENCE of a value which CAN be null/undefined. Where does this matter?

FUNCTION ARGUMENTS

For function arguments, it definitely matters.

function foo(x: string | undefined);
function bar(x?: string);

bar();  // ok
foo();  // ERROR
foo(undefined);  // ok

Is the definition of foo a case we want to allow? I would argue yes, because I don't see a good reason to exclude it. On the other hand, we've gotten quite far without needing it.

RETURN VALUES

Return values are more of a philosophical question. Does a function ever return no value? What if it returns void?

function foo(): string | undefined {
  return undefined;
}

Does foo ALWAYS return a value which can be null (are you guaranteed to get a nullable string), or does it optionally return something? (Does it sometimes return something and sometimes not return anything?)

For consistency, I would argue that it always returns a nullable. But on the other hand, what does this return:

function foo(): void {
}

That should always return nothing. Yet we have it always return a value (undefined), because that's what JavaScript does.

I could be convinced to argue that a function foo(): string | undefined half of the time behaves like void (doesn't return anything at all). It's ugly, I don't like it, but it has some sort of internal consistency.

PROPERTIES

Do properties have the same behavior as method arguments? In fact, they do.

function foo({ arg: string | undefined }) {
}
function bar({ arg?: string }) {
}

bar({});  // ok
foo({});  // ERROR
foo({ arg: undefined });  // ok

For structs I would agree with Elad that the definition of foo doesn't give any benefits, and in fact doesn't even properly serialize (the undefined would be stripped). After the initial object construction, you wouldn't even be able to tell the difference between the two invocations.

For output structs/interfaces, we kind of do an implicit mapping:

interface Foo {
  attr?: string;
}

Gets turned into:

interface Foo {
  String getAttr();
}

So we actually already turn attr?: string into attr: string | undefined for the purposes of non-JS languages, and in TS the two definitions don't have observably different behavior. Then let's settle on the easier on the easier-on-the-eyes version with the ?.

COLLECTIONS

This is an interesting one.

I will focus on arrays because maps have a trivial reduction into key optionality (just like structs do).

Is ["a", undefined, "b"] a valid inhabitant of string[]? I would argue no.

Is it valid to want to be able to represent the type of this value at all? I would say yes, but we could be free to disallow it because I don't think we use a structure like that anywhere, and its use should be rare.

If we wanted to type it, how would we type it?

Array<Nullable<string>>
or
Array<Union<string, undefined>>

The first requires us to have nullable types.

The second leads us to require being able to represent undefined as a standalone type so that we can make a union out of it (we currently don't have in the type system to represent that) and allows unions to escape into our higher level type system.

My opinion: I think we should have both optional values (for when we write value?: type) and nullable types (for positionally undefined values in an array or argument list).

eladb · 2019-04-10T07:03:04Z

Is it possible to decouple these discussions? We are in alignment on optionals, so let’s wrap up this PR, and follow up with a discussion and a separate PR on nullables. I don’t see a good reason to couple those together, right?

RomainMuller · 2019-04-10T07:31:05Z

Additional change discussions will happen in #440

rix0rrr · 2019-04-10T07:44:44Z

Is it possible to decouple these discussions?

I don't think so. We currently have one flag which we interpret as "optional" in a value context (property, argument) and as "nullable" in a type context (return values, Array<Nullable<string>>).

I would suggest we either make a decision on what properties we want from our type system and encode those, or we leave the current situation which is occasionally confusing but serviceable enough.

There is only one thing we cannot model today, which is a required, nullable parameter.

eladb · 2019-04-10T12:01:59Z

packages/jsii-spec/lib/spec.ts

     */
    returns?: string;

    /**
     * Whether the API item is beta/experimental quality
+     *
+     * @default Stability.Experimental


inherit from parent (and if no parent - stable)

RomainMuller self-assigned this Apr 4, 2019

RomainMuller requested review from costleya and a team as code owners April 4, 2019 10:45

rix0rrr reviewed Apr 4, 2019

View reviewed changes

RomainMuller added 3 commits April 5, 2019 15:52

Remove the concept of nullable and re-model optional so it's nicer

ee514aa

Merge remote-tracking branch 'origin/master' into rmuller/remodel-par…

65585a2

…ameter-optionality

Turn some 'find(...) != null' into 'some(...)'

ca26723

RomainMuller mentioned this pull request Apr 8, 2019

JSII doesnt genereate nullable enum in .Net #414

Closed

rix0rrr reviewed Apr 8, 2019

View reviewed changes

Re-model the whole beast, once more

52f04e3

eladb suggested changes Apr 8, 2019

View reviewed changes

WIP

cf59f8c

Re-re-re-re-re-model this 💩

04c3a94

eladb approved these changes Apr 10, 2019

View reviewed changes

Un-mark default stability

d9be27f

RomainMuller merged commit 21e485a into master Apr 10, 2019

RomainMuller deleted the rmuller/remodel-parameter-optionality branch April 10, 2019 12:16

RomainMuller mentioned this pull request Apr 10, 2019

chore(pkglint): Add rule to specify repository.directory aws/aws-cdk#2218

Merged

feat(jsii-spec): Model parameter optionality #432

feat(jsii-spec): Model parameter optionality #432

Conversation

RomainMuller commented Apr 4, 2019 • edited

RomainMuller commented Apr 4, 2019

rix0rrr commented Apr 4, 2019

RomainMuller commented Apr 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rix0rrr commented Apr 8, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RomainMuller commented Apr 8, 2019

eladb commented Apr 8, 2019

RomainMuller commented Apr 8, 2019 • edited

RomainMuller commented Apr 8, 2019

rix0rrr commented Apr 9, 2019 • edited

eladb commented Apr 10, 2019

RomainMuller commented Apr 10, 2019

rix0rrr commented Apr 10, 2019

Choose a reason for hiding this comment

RomainMuller commented Apr 4, 2019 •

edited

rix0rrr commented Apr 8, 2019 •

edited

RomainMuller commented Apr 8, 2019 •

edited

rix0rrr commented Apr 9, 2019 •

edited