Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

can a diamond dependency be used #2256

Closed
1 of 4 tasks
shivlaks opened this issue Nov 17, 2020 · 3 comments
Closed
1 of 4 tasks

can a diamond dependency be used #2256

shivlaks opened this issue Nov 17, 2020 · 3 comments
Labels
closed-for-staleness language/java Related to Java bindings management/tracking Issues that track a subject or multiple issues needs-discussion This issue/PR requires more discussion with community.

Comments

@shivlaks
Copy link
Contributor

shivlaks commented Nov 17, 2020

❓ Guidance

Affected Languages

  • TypeScript or Javascript
  • Python
  • Java
  • .NET (C#, F#, ...)

General Information

  • JSII Version: 1.14.1
  • Platform: macOS Catalina (10.15.7)

The Question

Is it possible to use a diamond dependency if there are no conflicting (potentially duplicated) properties?
A recent CDK PR motivated this question where a construct was updated to declare interfaces in the following manner:
image

a simplified view of the interface declarations here looks like the following:

interface CommonStartBuildOptions {
  readonly buildSpec?: BuildSpec;
  readonly encryptionKey?: kms.IKey;
  readonly cache?: Cache;
  readonly environment?: BuildEnvironment;
  readonly role?: iam.IRole;
  /**
   * The number of minutes after which AWS CodeBuild stops the build if it's
   * not complete. For valid values, see the timeoutInMinutes field in the AWS
   * CodeBuild User Guide.
   *
   * @default Duration.hours(1)
   */
  readonly timeout?: Duration;
  readonly environmentVariables?: { [name: string]: BuildEnvironmentVariable };
}

export interface StartBuildOptions extends CommonStartBuildOptions {
  readonly source?: ISource;
  readonly artifacts?: IArtifacts;
  readonly secondarySources?: ISource[];
  readonly secondaryArtifacts?: IArtifacts[];
}

export interface CommonProjectProps extends CommonStartBuildOptions {
  readonly description?: string;
  readonly badge?: boolean;
  readonly projectName?: string;
  readonly vpc?: ec2.IVpc;
  readonly subnetSelection?: ec2.SubnetSelection;
  readonly securityGroups?: ec2.ISecurityGroup[];
  readonly allowAllOutbound?: boolean;
  readonly fileSystemLocations?: IFileSystemLocation[];
  readonly grantReportGroupPermissions?: boolean;
}

export interface ProjectProps extends StartBuildOptions, CommonProjectProps {
}

However there was a failure with the java code generation:

[ERROR] /tmp/npm-packf10vHL/monocdk/src/main/java/software/amazon/awscdk/services/codebuild/ProjectProps.java:[10,8] interface software.amazon.awscdk.services.codebuild.ProjectProps inherits unrelated defaults for getTimeout() from types software.amazon.awscdk.services.codebuild.StartBuildOptions and software.amazon.awscdk.services.codebuild.CommonProjectProps

Is this something that we could support? If not, perhaps it's an opportunity to validate

@shivlaks shivlaks added guidance Question that needs advice or information. needs-triage This issue or PR still needs to be triaged. labels Nov 17, 2020
@SomayaB SomayaB added the language/java Related to Java bindings label Nov 18, 2020
@NGL321 NGL321 added management/tracking Issues that track a subject or multiple issues needs-discussion This issue/PR requires more discussion with community. and removed guidance Question that needs advice or information. needs-triage This issue or PR still needs to be triaged. labels Dec 7, 2020
@MrArnoldPalmer
Copy link
Contributor

Related to this is when a TS interface or class implements two interfaces that share a property/method with the same name and type. This will generate Java interfaces with default implementations for both and the compiler doesn't know which one to use. Re-declaring the implementation at the implementer level when there is a conflict will fix this. We saw this in aws/aws-cdk#12700.

mergify bot pushed a commit to aws/aws-cdk that referenced this issue Feb 2, 2021
Needs reverting because of aws/jsii#2256 .

This reverts commit 1a9f2a8.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@RomainMuller
Copy link
Contributor

Turns out the code is generated correctly for diamond dependencies, however what happens and probably cannot be supported in a safe way is when two upstream interfaces declare optional properties by the same name (and type, but this is already checked for).

This can appear to happen with a diamond graph, if the interface that declares some optional property (and is inherited by a single child through multiple parents) is not exported... in such cases declarations are "promoted" to the extending classes as the internal interface is erased from the exported model.

In any case - even though we could generate code that compiles, given a snapshot of a dependency graph, this cannot be made to be forward-compatible with addition of new a conflicting declaration in an extended dependency: the solution would be to "override" the default implementation if there are > 1 parents declaring it; but we cannot know what will happen in the future. Additionally, this scenario would likely result in usability issues.

NovakGu pushed a commit to NovakGu/aws-cdk that referenced this issue Feb 18, 2021
…s#12832)

Needs reverting because of aws/jsii#2256 .

This reverts commit 1a9f2a8.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
skinny85 added a commit to skinny85/aws-cdk that referenced this issue Mar 3, 2021
This is a re-submit of the PR aws#12700,
which had to be reverted because of JSII issue aws/jsii#2256.
Since that issue has been fixed in JSII version `1.23.0`,
which is what we currently use,
re-introduce the changes from that PR.
mergify bot pushed a commit to aws/aws-cdk that referenced this issue Mar 4, 2021
This is a re-submit of the PR #12700,
which had to be reverted because of JSII issue aws/jsii#2256.
Since that issue has been fixed in JSII version `1.23.0`,
which is what we currently use,
re-introduce the changes from that PR.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
nija-at pushed a commit to aws/aws-cdk that referenced this issue Mar 5, 2021
This is a re-submit of the PR #12700,
which had to be reverted because of JSII issue aws/jsii#2256.
Since that issue has been fixed in JSII version `1.23.0`,
which is what we currently use,
re-introduce the changes from that PR.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
nija-at pushed a commit to aws/aws-cdk that referenced this issue Mar 8, 2021
This is a re-submit of the PR #12700,
which had to be reverted because of JSII issue aws/jsii#2256.
Since that issue has been fixed in JSII version `1.23.0`,
which is what we currently use,
re-introduce the changes from that PR.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
cornerwings pushed a commit to cornerwings/aws-cdk that referenced this issue Mar 8, 2021
This is a re-submit of the PR aws#12700,
which had to be reverted because of JSII issue aws/jsii#2256.
Since that issue has been fixed in JSII version `1.23.0`,
which is what we currently use,
re-introduce the changes from that PR.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
nija-at pushed a commit to aws/aws-cdk that referenced this issue Mar 9, 2021
This is a re-submit of the PR #12700,
which had to be reverted because of JSII issue aws/jsii#2256.
Since that issue has been fixed in JSII version `1.23.0`,
which is what we currently use,
re-introduce the changes from that PR.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@RomainMuller RomainMuller removed their assignment Jun 24, 2021
@github-actions
Copy link
Contributor

This issue has not received any attention in 2 years. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

@github-actions github-actions bot added closing-soon This issue will automatically close in 4 days unless further comments are made. closed-for-staleness and removed closing-soon This issue will automatically close in 4 days unless further comments are made. labels Jun 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-for-staleness language/java Related to Java bindings management/tracking Issues that track a subject or multiple issues needs-discussion This issue/PR requires more discussion with community.
Projects
None yet
Development

No branches or pull requests

5 participants