fix(pacmak): generates too many methods for Java

[rix0rrr]

When pacmak is generating Java code, it is generating Jsii$Default interfaces and Jsii$Proxy classes; these have default implementations that call out to the jsii kernel for every property and method.

Because all Jsii$Default interfaces already have default implementations for every method to call out to jsii, any methods that are inherited from these default interfaces don't need to be inherited on $Proxy and $Default subtypes.

The logic for this was incorrect, and didn't cut out as many types as it could.

Example:

interface ISuper {
  public readonly super: string;
}

interface ISub extends ISuper {
  public readonly sub: string;
}

Leads to (roughly) the following Java code:

interface ISuper {
  String getSuper();

  interface Jsii$Default extends ISuper {
      String getSuper() {
        return /* call jsii kernel here */
      }
  }
}

interface ISub extends ISuper {
  String getSub();

  interface Jsii$Default extends ISub, ISuper.Jsii$Default {
      String getSuper() {    // <--- ❌ doesn't need to be here!
        return /* call jsii kernel here */
      }

      String getSub() {
        return /* call jsii kernel here */
      }
  }
}

The logic that decided whether or not to render methods made two mistakes:

• abstract: abstract members need to be rendered, but only for classes. Interface members are always abstract, but also will have a default implementation on the $Default interface already.
• The check was using member.parentType, but should have been using member.definingType instead: parentType is always the current type we're rendering members for.

The upshot of not rendering unnecessary members is that if we happen to backwards incompatibly break some upstream interface (😇) the upstream implementation can just be inherited instead of re-rendered. This improves compatibility across versions, and makes it easier to roll out breaking changes like this.

Unfortunately there was also a bug in the original implementation which led it to generating too few members -- method overloads for optional arguments were not being generated into the Jsii$Default implementation. So we still need to generate methods for overloads in the subtypes. We can only drop the generation of these overloads when the lowest version of the jsii package that a library can be used in conjunction with definitely has them already, and I'm not sure we can know that in advance (only if users set up the version combination correctly, which we can't really control). Do the cleanup of that later.

There logic of rendering optional properties that are multiply inherited was also overzealous: it was not figuring out that a single implementation inherited via different parents didn't need to be duplicated; and in fact said it was doing only optional properties, but was actually doing all properties.

---

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

[rix0rrr]

(Sneak preview, still updating snappies)

[kumvprat]

This makes sense but for confirmation => defining type and parent type will be only same when the given property/method does not have any implementation ?

[kumvprat]

In that case won't this hasDefaultInterfaces(x.definingType.assembly) cover the check

Missing context for me but the function seems to check if any kind of implementation of the property/method is already present somewhere in the inheritance chain till now

Happy to understand this in detail if my understanding is wrong

[rix0rrr]

defining type and parent type will be only same when the given property/method does not have any implementation

An example will say it best I think:

interface IThing {
  readonly foo: string; 
}

class Thing implements IThing {
  public readonly foo: string = '3';
}

When processing IThing:

property foo    parentType = definingType = IThing

When processing Thing:

property foo    parentType = Thing             definitingType = IThing

But it's not necessarily the case that it means it doesn't have any implementation. The same parentType/definingType relationship holds here:

class Super {
  public foo() {
    return 'foo';
  }
}

class Sub extends Super {
  public foo() {                      // <-- overrides 'foo', definingType is still Super
    return 'fooooooo'; 
  }
}

[rix0rrr]

In that case won't this hasDefaultInterfaces(x.definingType.assembly) cover the check

I'll refer to the example of the PR body:

// TypeScript
interface ISub extends ISuper {
  public readonly sub: string;
}

// Java
interface ISub extends ISuper {
  String getSub();

  interface Jsii$Default extends ISub, ISuper.Jsii$Default {
      String getSub() {                             //  <-- 1️⃣ 
        return /* call jsii kernel here */
      }
  }
}

1️⃣ is not inherited from ISuper$Default, so it needs to be implemented. The way we know that is because it is defined in this interface. We test for that by checking that its defining type is equal to the current type.

[rix0rrr]

Since I made this to explain I might as well stick it here for posterity:

[kumvprat]

Any reason to make this as a constant and not make it configurable ?

If this is always true then we can simplify the makeDefaultImpls to

  private makeDefaultImpls(m: reflect.Method): spec.Method[] {
    const ret: spec.Method[] = [];
    if (needsDefaultImpl(m)) {
      ret.push(m.spec);
    }

   ret.push(...this.createOverloadsForOptionals(m.spec));

    return ret;
  }

[rix0rrr]

The reason to make this a constant is to make it somewhat configurable.

This allows me to give a name and a motivation to a condition that we can obviously decide to switch. This is less obvious if the if isn't even there in the first place.

[kumvprat]

Got it, then renaming it to something like GENERATE_OVERLOADS_FOR_UPSTREAM_PACKAGES will suggest what we are trying to do this with the constant.(or something similar)

So that even if we decide to turn it to false later on we know what we are turning off.

[mergify]

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]

Merging (with squash)...

[mergify]

Merge Queue Status

✅ The pull request has been merged at 6cdf16e

This pull request spent 33 minutes 5 seconds in the queue, including 32 minutes 56 seconds running CI.
The checks were run in-place.

Required conditions to merge

• #approved-reviews-by >= 1 [🛡 GitHub branch protection]
  • fix(pacmak): generates too many methods for Java #5007
• #changes-requested-reviews-by = 0 [🛡 GitHub branch protection]
  • fix(pacmak): generates too many methods for Java #5007
• status-success=Integration test (jsii-pacmak)
• status-success=Unit Tests
• any of [🛡 GitHub branch protection]:
  • check-success = Integration test (jsii-pacmak)
  • check-neutral = Integration test (jsii-pacmak)
  • check-skipped = Integration test (jsii-pacmak)
• any of [🛡 GitHub branch protection]:
  • check-success = Build
  • check-neutral = Build
  • check-skipped = Build
• any of [🛡 GitHub branch protection]:
  • check-success = Unit Tests
  • check-neutral = Unit Tests
  • check-skipped = Unit Tests

[mergify]

Merging (with squash)...