feat(transformer): Support circular interface extensions #389

martinjlowm · 2020-06-20T11:04:40Z

Utilize constructors to support circular generics for instantiable types.

If the transformer experiences circular generics, the scope descriptor parameter
is now used to preserve a nested state to avoid looping descriptors forever. The
scope enables the generic descriptor to determine whether to emit a new instance
of the extension or to reuse the parent's constructor (which would emit an
instance of the same prototype), i.e.:

getFactory("@factory")([{
  i: ["@parameterId"],
  w: function () { return new function () { Object.assign(this, ɵRepository.ɵRepository.instance.getFactory("@factory")([{
    i: ["@parameterId"],
    w: function () { return new this.constructor; }
  }])); }; }
}])

┌────────────────────────┬────────┬──────────┬──────────┬─────────────┬──────────┬─────────┬─────────────┬──────────┬───────────┬────────────┬───────────┬────────────┬───────────┬────────────┐
│        (index)         │ Files  │  Lines   │  Nodes   │ Identifiers │ Symbols  │  Types  │ Memory used │ I/O read │ I/O write │ Parse time │ Bind time │ Check time │ Emit time │ Total time │
├────────────────────────┼────────┼──────────┼──────────┼─────────────┼──────────┼─────────┼─────────────┼──────────┼───────────┼────────────┼───────────┼────────────┼───────────┼────────────┤
│     no transformer     │ '5093' │ '112415' │ '409280' │  '127866'   │ '103042' │ '37318' │  '455358K'  │  '0.35'  │  '0.87'   │   '1.45'   │  '0.50'   │   '2.94'   │  '2.82'   │   '7.71'   │
│     no createMock      │ '5093' │ '112415' │ '409280' │  '127866'   │ '103042' │ '37318' │  '486916K'  │  '0.30'  │  '0.86'   │   '1.37'   │  '0.50'   │   '2.89'   │  '2.98'   │   '7.75'   │
│ One interface per file │ '5098' │ '112431' │ '439436' │  '137914'   │ '103077' │ '32346' │  '508633K'  │  '0.33'  │  '0.84'   │   '1.52'   │  '0.50'   │   '3.12'   │  '3.53'   │   '8.67'   │
│    Interface reuse     │ '5098' │ '97431'  │ '444436' │  '132914'   │ '93078'  │ '27346' │  '499714K'  │  '0.32'  │  '0.95'   │   '1.47'   │  '0.55'   │   '3.20'   │  '3.67'   │   '8.90'   │
│ One Interface / Reuse  │ '5098' │ '104931' │ '441936' │  '135414'   │ '98078'  │ '29846' │  '504858K'  │  '0.30'  │  '0.90'   │   '1.49'   │  '0.50'   │   '3.29'   │  '3.71'   │   '8.99'   │
└────────────────────────┴────────┴──────────┴──────────┴─────────────┴──────────┴─────────┴─────────────┴──────────┴───────────┴────────────┴───────────┴────────────┴───────────┴────────────┘

I just went ahead and opened this here (cc: @uittorio) - it's easier than having a commit hash lying around in some random conversation.

uittorio · 2020-06-25T07:28:09Z

Hi @martinjlowm. @Pmyl and I had a look at your changes. This is great work.

Pmyl found a scenario where this implementation will not work. Could you add a test ?


interface B<T> {
    prop: T;
  }
  interface A extends B<A> {
    a: boolean;
    test: A;
  }

  const type: A = createMock<A>();
  expect(type.test.prop.a).toBe(false); // undefined

the reason this is not working its because generic are cached and re used. I think in your implementation you have to make sure the value w will be cached correctly. I can have a look later but I'm sure you'll find the solution :P

w: function () {
                             return new this.constructor;
}

martinjlowm · 2020-06-26T16:26:44Z

Hi @martinjlowm. @Pmyl and I had a look at your changes. This is great work.

Pmyl found a scenario where this implementation will not work. Could you add a test ?
interface B<T> {
    prop: T;
  }
  interface A extends B<A> {
    a: boolean;
    test: A;
  }

  const type: A = createMock<A>();
  expect(type.test.prop.a).toBe(false); // undefined
the reason this is not working its because generic are cached and re used. I think in your implementation you have to make sure the value w will be cached correctly. I can have a look later but I'm sure you'll find the solution :P
w: function () {
                             return new this.constructor;
}

I see!

So, the fix is easy, but I'd like some thoughts on some naming.

The problem lies here:

ts-auto-mock/src/transformer/genericDeclaration/genericDeclaration.ts

Lines 160 to 162 in 57727e6

    
           if (!typeParameterDeclaration || scope.currentMockKey !== declarationKey) { 
        
             genericValueDescriptor = GetDescriptor(genericNode, new Scope(declarationKey)); 
        
           }

The mock key for the new scope may conflict with other parallel scopes and the fix is as simple as:

        const constructedDeclarationKey = `${declarationKey}_C`;
        if (!typeParameterDeclaration || scope.currentMockKey !== constructedDeclarationKey) {
          genericValueDescriptor = GetDescriptor(genericNode, new Scope(constructedDeclarationKey));
        }

Which will uniquely identify the scope with the appended _C (c, for constructed and _ to keep it consistent with generated keys) and the constructor will always be emitted on the first occurrence.

Alternatively, we may also extend Scope with a "bind"-state and toggle it on here (to know that a constructor has been emitted, thus binding this). I don't know, maybe, it'll become useful in other scenarios.

        if (!typeParameterDeclaration || !scope.isBound()) {
          genericValueDescriptor = GetDescriptor(genericNode, (new Scope(declarationKey)).bind());
        }

What do You think?

Pmyl · 2020-06-26T18:13:49Z

I've found another problem with a much simpler example.
It seems like the this.constructor code doesn't work as expected, in your tests you're not hitting that code so they don't fail.

export class ClassWithGenerics<T> {
  public a: T;
}

interface A extends ClassWithGenerics<A> {
  b: number;
}

createMock<A>();

This is the generated code:

ɵRepository.ɵRepository.instance.getFactory("@A_1")([{
    i: ["@ClassWithGenerics_1T"],
    w: function () {
        return new function () {
            Object.assign(this, ɵRepository.ɵRepository.instance.getFactory("@A_1")([{
                i: ["@ClassWithGenerics_1T"],
                w: function () {
                    return new this.constructor;
                }
            }]));
        };
    }
}]);

This is your test:
99cb2de#diff-a93f7cd70330ab19d1c4df5c1b3bd0e7R258

expect(propertiesA.a.b).toBe(0);

That hits only the code with Object.assign, to hit the return new this.constructor we need to go deeper and use again the a property:

expect(propertiesA.a.a.b).toBe(0);

That will fail because b is undefined. It seems like the last a is an empty object

martinjlowm · 2020-06-26T19:48:56Z

Right, that makes sense - I didn't realize class properties were emitted differently to interface properties. this is not bound correctly for class properties. I'll investigate :)

Pmyl · 2020-06-26T19:51:32Z

I’m looking forward the new solution, I was positively surprised by how smart this solution is, too bad it doesn’t work for every case

Utilize constructors to support circular generics for instantiable types. If the transformer experiences circular generics, the scope descriptor parameter is now used to preserve a nested state to avoid looping descriptors forever. The scope enables the generic descriptor to determine whether to emit a new instance of the extension or to reuse the parent's constructor (which would emit an instance of the same prototype), i.e.: ``` getFactory("@factory")([{ i: ["@parameterId"], w: function () { return new function () { Object.assign(this, ɵRepository.ɵRepository.instance.getFactory("@factory")([{ i: ["@parameterId"], w: function () { return new this.constructor; } }])); }; } }]) ```

…d this

…since such keys may be cached and reused in parallel A bound state is an indicator whether a new function instance is emitted, binding this in the current scope. The scope's constructor can then be referenced with `this.constructor` enabling a reference to the parent constructor.

…measure

…etween two interfaces

martinjlowm · 2020-06-27T08:40:33Z

It appears there was a couple of issues:

The cached declaration key issue, I fixed by implementing the proposed Scope change. And actually, this led to the following issue.
Object.assign invokes the getter's when copying, leading to an inifinite recursion in runtime. This is fixed by using a combination of Object.defineProperties and Object.getOwnPropertyDescriptors. That also implies, that if you were to log the reference to stdout, the toString function will result in the same infinite recursion.
this is now temporarily stored as that from the MockIdentifierGenericCircularReference identifier to preserve the reference.

I went ahead and bumped the test to cover the actual nesting. I didn't realize that I had previously tested by referencing the non-generic properties which is completely irrelevant to this PR 🤦

uittorio · 2020-06-27T15:56:57Z

src/transformer/genericDeclaration/genericDeclaration.ts

@@ -110,10 +167,15 @@ export function GenericDeclaration(scope: Scope): IGenericDeclaration {
          }
        }

+        if (!typeParameterDeclaration || !scope.isBound()) {


It may be a dumb question but I still don't understand why the previous if statement is not good enough to make sure is a circular generic dependency?

I mean this const isExtendingItself: boolean = MockDefiner.instance.getDeclarationKeyMap(typeParameterDeclaration) === declarationKey;

The old condition was fine if you were to completely ignore circular generics and terminate early.

However, this time around, we want to process the declaration exactly twice, where the second iteration avoids calling GetDescriptor (which is what's causing the infinite recursion) and instead utilizes this.constructor for a back-reference.

isExtendingItself does not help us in that case if we were to use it to our advantage, since the type checker would bring us to the same declaration on every GetDescriptor call and result in the same scenario, yes, it is extending itself, over and over.

Pmyl · 2020-06-29T13:30:54Z

I have few comments on it but before leaving them I'm spending a bit of time understanding the reason behind every change. At the moment the behaviour seems fine but I'll let you know when I read and ran everything locally, sorry for the delay

martinjlowm · 2020-06-29T13:52:49Z

Sure, take your time :)

Pmyl · 2020-07-08T14:34:04Z

I had some time to test some edge cases and I noticed something interesting in the generated code:

In case of a double circular dependency

interface GenC<T> {
  c: T;
}

interface GenB<T> extends GenC<GenB<T>> {
  b: T;
}

interface A extends GenB<A> {
  a: A;
  value: string;
}

there is a piece of generated code that goes like this

[...]
Object.defineProperties(m, {
    "a": {
        get: function () {
            return d.hasOwnProperty("a") ? d["a"] : d["a"] = ɵRepository.ɵRepository.instance.getFactory("@A_1")([{
                i: ["@GenB_1T"],
                w: function () {
                    return new function () {
                        var that = this;
                        Object.defineProperties(this, Object.getOwnPropertyDescriptors(ɵRepository.ɵRepository.instance.getFactory("@A_1")([{
                            i: ["@GenB_1T"],
                            w: function () {
                                return new that.constructor;
                            }
                        }, {
                            i: ["@GenC_1T"],
                            w: function () {
                                return new that.constructor;
                            }
                        }])));
                    };
                }
            }, {
[...]

In these generics @GenB_1T makes sense to be "new that.constructor" because that generic has to generate another instance of @A_1.
The problem is in @GenC_1T, that shouldn't be A but GenB<A>.
I don't think this can cause particular problems because A extends GenB<A> so for the rules of polymorphism those are actually matching!

But in general I would like to make sure that these (currently failing) tests pass correctly

it('should work', () => {
  interface GenC<T> {
    c: T;
  }

  interface GenB<T> extends GenC<GenB<T>> {
    b: T;
  }

  interface A extends GenB<A> {
    a: A;
    value: string;
  }

  const type: A = createMock<A>();
  expect((type.a.c.b as unknown as A).value).not.toEqual('');
  expect((type.a.b as unknown as A).value).not.toEqual('');
  expect((type.a.b.c as unknown as A).value).not.toEqual('');
  expect((type.b.c as unknown as A).value).not.toEqual('');
  expect((type.b as unknown as A).value).not.toEqual('');
});

As I said this can be a non-issue if nobody casts anything to unknown/any but I didn't spent much time to think of worse outcomes.
If someone want to ignore this small issue please let me know, we can take a decision

Pmyl · 2020-07-08T15:03:05Z

I just found a more problematic case:

  interface GenC<T> {
    c: T;
  }

  interface GenB<T> {
    b: T;
  }

  interface B {
    valueB: string;
  }

  interface A extends GenB<A>, GenC<B> {
    a: A;
    valueA: string;
  }

  const type: A = createMock<A>();
  expect((type.a.b.c as unknown as A).valueA).not.toEqual(''); //fail
  expect(type.a.b.c.valueB).toEqual(''); //fail
  expect((type.b.c as unknown as A).valueA).not.toEqual(''); //fail
  expect(type.b.c.valueB).toEqual(''); //fail

Generated code:

var type = ɵRepository.ɵRepository.instance.getFactory("@A_1")([{
    i: ["@GenB_1T"],
    w: function () {
        return new function () {
            var that = this;
            Object.defineProperties(this, Object.getOwnPropertyDescriptors(ɵRepository.ɵRepository.instance.getFactory("@A_1")([{
                i: ["@GenB_1T"],
                w: function () {
                    return new that.constructor;
                }
            }, {
                i: ["@GenC_1T"],
                w: function () {
                    return new that.constructor;
                }
            }])));
        };
    }
}, {
    i: ["@GenC_1T"],
    w: function () {
        return new function () {
            var that = this;
            Object.defineProperties(this, Object.getOwnPropertyDescriptors(ɵRepository.ɵRepository.instance.getFactory("@B_1")([])));
        };
    }
}]);

As you can see the generic @GenC_1T is going to be A (new that.constructor) but it should be B. This breaks cases where no cast to any/unknown happened.

I'm not sure why this happens really, I think it's something about the scope... unfortunately this feature is more complex than expected

martinjlowm · 2020-07-09T14:51:31Z

I see! Nice that you found some complex test cases for this. In general, in both examples, it seems we need to apply some extra logic to this "bind"-scope behavior - a simple scope check is not enough since the generic variable may change in that same iteration, in the case where there are more than a single extension applied.

I hope to find some time over the weekend to test against those two examples and find a solution. I will keep you posted :)

martinjlowm · 2020-07-20T07:09:52Z

Just an update - I had a look at these extra cases yesterday and I'm close to a solution - what's left, is being able to distinguish the that assignment and a reference to it, since reassignments may break previous references (through nesting). I updated the "bind"-scope change to bind for generic keys to avoid the conflicts when multiple extensions are present, i.e. a Set for bind() -> bindFor(key: string).

… forked extensions

… context

martinjlowm · 2020-07-27T18:19:17Z

454713f solves the issue you discovered @Pmyl - I rewrote the extends test to cover those cases and a range of parallel circular extensions. MockIdentifierGenericCircularReference is replaced with a variable named as the uniqueName in createGenericParameter, with @ replaced by an _. We may have to name it differently to avoid variable clashing - perhaps by using MockPrivatePrefix instead? What do You guys think?

edit: That was perhaps a bit too quick I commented - discovered some cases that weren't covered.

… singly linked list

martinjlowm · 2020-07-31T20:10:36Z

A quick update.

With the two most recent commits, the emitted code for a fairly complex extension configuration is as follows:

ɵRepository.ɵRepository.instance.registerFactory("@B_1", function (t) { return (function () { var d = {}, m = { "b": "" }; Object.defineProperties(m, {
    "A": { get: function () { return d.hasOwnProperty("A") ? d["A"] : d["A"] = ɵRepository.ɵRepository.instance.getFactory("@A_1")([{
                i: ["@GenericC_1T"],
                w: function () { return new function () { var ɵA_1_GenericC_1T = this; Object.defineProperties(this, Object.getOwnPropertyDescriptors(ɵRepository.ɵRepository.instance.getFactory("@A_1")([{
                        i: ["@GenericC_1T"],
                        w: function () { return new ɵA_1_GenericC_1T.constructor; }
                    }, {
                        i: ["@GenericD_1T"],
                        w: function () { return new function () { var ɵA_1_GenericD_1T = this; Object.defineProperties(this, Object.getOwnPropertyDescriptors(ɵRepository.ɵRepository.instance.getFactory("@A_1")([{
                                i: ["@GenericC_1T"],
                                w: function () { return new ɵA_1_GenericC_1T.constructor; }
                            }, {
                                i: ["@GenericD_1T"],
                                w: function () { return new ɵA_1_GenericD_1T.constructor; }
                            }, {
                                i: ["@GenericE_1T"],
                                w: function () { return new function () { var ɵA_1_GenericE_1T = this; Object.defineProperties(this, Object.getOwnPropertyDescriptors(ɵRepository.ɵRepository.instance.getFactory("@B_1")([{
                                        i: ["@GenericC_1T"],
                                        w: function () { return new function () { var ɵB_1_GenericC_1T = this; Object.defineProperties(this, Object.getOwnPropertyDescriptors(ɵRepository.ɵRepository.instance.getFactory("@B_1")([{
                                                i: ["@GenericC_1T"],
                                                w: function () { return new ɵB_1_GenericC_1T.constructor; }
                                            }, {
                                                i: ["@GenericD_1T"],
                                                w: function () { return new function () { var ɵB_1_GenericD_1T = this; Object.defineProperties(this, Object.getOwnPropertyDescriptors(ɵRepository.ɵRepository.instance.getFactory("@A_1")([{
                                                        i: ["@GenericC_1T"],
                                                        w: function () { return new ɵA_1_GenericC_1T.constructor; }
                                                    }, {
                                                        i: ["@GenericD_1T"],
                                                        w: function () { return new ɵA_1_GenericD_1T.constructor; }
                                                    }, {
                                                        i: ["@GenericE_1T"],
                                                        w: function () { return new ɵA_1_GenericE_1T.constructor; }
                                                    }]))); }; }
                                            }, {
                                                i: ["@GenericE_1T"],
                                                w: function () { return new function () { var ɵB_1_GenericE_1T = this; Object.defineProperties(this, Object.getOwnPropertyDescriptors(ɵRepository.ɵRepository.instance.getFactory("@A_1")([{
                                                        i: ["@GenericC_1T"],
                                                        w: function () { return new ɵA_1_GenericC_1T.constructor; }
                                                    }, {
                                                        i: ["@GenericD_1T"],
                                                        w: function () { return new ɵA_1_GenericD_1T.constructor; }
                                                    }, {
                                                        i: ["@GenericE_1T"],
                                                        w: function () { return new ɵA_1_GenericE_1T.constructor; }
                                                    }]))); }; }
                                            }]))); }; }
                                    }, {
...

The naming of A_1_GenericC_1T relates to the GenericC<T> implementation of A.

Since these constructor references are only valid for the current scope, this quickly builds up a lot of extra code, that I think can be reused to some degree. At least the scope functionality is there, so registration of cached functions should be fairly straightforward to implement.

Currently, There's still at least one uncovered case I know of:

interface A extends GenericC<A>, GenericD<A>, GenericE<B>, GenericFG<A, B> {
  a: number;
  B: B;
}

Here, GenericFG does not bind A and B properly. The bind functionality, does not consider more than one type argument. I hope the fix for that is easy :)

martinjlowm changed the title ~~feature(transformer): Support circular interface extensions~~ feat(transformer): Support circular interface extensions Jun 20, 2020

martinjlowm force-pushed the feature/recursive-generics branch from 0c5b829 to 99cb2de Compare June 20, 2020 11:54

martinjlowm added 5 commits June 27, 2020 10:16

chore(docs): Remove circular generics from types-not-supported.mdx

c35347b

fix(transformer): Store reference to parent generic to avoid a reboun…

3f0bd8c

…d this

chore(test): Bump the nesting of the generic recursion test for good …

aa5285f

…measure

martinjlowm force-pushed the feature/recursive-generics branch from 57727e6 to aa5285f Compare June 27, 2020 08:23

chore(test): Extend the circular generic test with cross references b…

0b80962

…etween two interfaces

uittorio reviewed Jun 27, 2020

View reviewed changes

uittorio and others added 2 commits June 27, 2020 17:43

Merge branch 'master' into feature/recursive-generics

b1cab69

Merge branch 'master' into feature/recursive-generics

079c3f0

martinjlowm added 3 commits July 27, 2020 19:58

fix(transformer): Adjust the Scope bind interface to make it work for…

84762b8

… forked extensions

fix(transformer): Properly distinguish scoped references in a generic…

454713f

… context

fix(transformer): Remove constructor marker

b8fa5ce

martinjlowm force-pushed the feature/recursive-generics branch from 51a73c1 to b8fa5ce Compare July 27, 2020 18:15

Merge branch 'master' into feature/recursive-generics

e000ef9

martinjlowm added 2 commits July 31, 2020 21:50

chore(transformer): Use MockPrivatePrefix for generic owner references

b66779a

fix(transformer): Correct scope nesting and owner reference through a…

a9b5013

… singly linked list

Merge branch 'master' into feature/recursive-generics

8d6341f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(transformer): Support circular interface extensions #389

feat(transformer): Support circular interface extensions #389

martinjlowm commented Jun 20, 2020 •

edited

Loading

uittorio commented Jun 25, 2020

martinjlowm commented Jun 26, 2020

Pmyl commented Jun 26, 2020

martinjlowm commented Jun 26, 2020

Pmyl commented Jun 26, 2020

martinjlowm commented Jun 27, 2020

uittorio Jun 27, 2020

martinjlowm Jun 28, 2020

Pmyl commented Jun 29, 2020

martinjlowm commented Jun 29, 2020

Pmyl commented Jul 8, 2020 •

edited

Loading

Pmyl commented Jul 8, 2020

martinjlowm commented Jul 9, 2020

martinjlowm commented Jul 20, 2020

martinjlowm commented Jul 27, 2020 •

edited

Loading

martinjlowm commented Jul 31, 2020

feat(transformer): Support circular interface extensions #389

Are you sure you want to change the base?

feat(transformer): Support circular interface extensions #389

Conversation

martinjlowm commented Jun 20, 2020 • edited Loading

uittorio commented Jun 25, 2020

martinjlowm commented Jun 26, 2020

Pmyl commented Jun 26, 2020

martinjlowm commented Jun 26, 2020

Pmyl commented Jun 26, 2020

martinjlowm commented Jun 27, 2020

uittorio Jun 27, 2020

Choose a reason for hiding this comment

martinjlowm Jun 28, 2020

Choose a reason for hiding this comment

Pmyl commented Jun 29, 2020

martinjlowm commented Jun 29, 2020

Pmyl commented Jul 8, 2020 • edited Loading

Pmyl commented Jul 8, 2020

martinjlowm commented Jul 9, 2020

martinjlowm commented Jul 20, 2020

martinjlowm commented Jul 27, 2020 • edited Loading

martinjlowm commented Jul 31, 2020

martinjlowm commented Jun 20, 2020 •

edited

Loading

Pmyl commented Jul 8, 2020 •

edited

Loading

martinjlowm commented Jul 27, 2020 •

edited

Loading