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

fix: omitting optional property fails C# compilation #1448

Merged
merged 5 commits into from
Oct 30, 2024

Conversation

rix0rrr
Copy link
Contributor

@rix0rrr rix0rrr commented Oct 30, 2024

The following is a minimal reproducing example:

export interface ISomeInterface {
  readonly xyz?: string;
}

export abstract class SomeClass implements ISomeInterface {
}

This breaks code generation for C#, as the SomeClass._Proxy class we generate will try to generate an override for the Xyz property, which is missing from the SomeClass class itself.

This issue is exhibited by a difference in logic between how we iterate the members of a class when we generate the class itself vs when we generate its proxy.

However, after looking at the code I'm not convinced it's correct either way (nor for Java), so to be safe we picked the solution where we force the user to declare the property on the class.

The correct declaration would be:

export abstract class SomeClass implements ISomeInterface {
  public abstract readonly xyz?: string;
}

Related: https://github.com/aws/aws-cdk/pull/31946/files


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

The following is a minimal reproducing example:

```ts
export interface ISomeInterface {
  readonly xyz?: string;
}

export abstract class SomeClass implements ISomeInterface {
}
```

This breaks code generation for C#, as the `SomeClass._Proxy` class
we generate will try to generate an `override` for the `Xyz` property,
which is missing from the `SomeClass` class itself.

This issue is exhibited by a difference in logic between how we iterate
the members of a class when we generate the class itself vs when we
generate its proxy.

However, after looking at the code I'm not convinced it's correct either
way (nor for Java), so to be safe we picked the solution where we force
the user to declare the property on the class.

The correct declaration would be:

```ts
export abstract class SomeClass implements ISomeInterface {
  public abstract readonly xyz?: string;
}
```
@rix0rrr rix0rrr requested a review from a team October 30, 2024 17:39
@rix0rrr rix0rrr self-assigned this Oct 30, 2024
github-actions and others added 3 commits October 30, 2024 17:42
Signed-off-by: github-actions <github-actions@github.com>
…aws/jsii-compiler into huijbers/missing-prop-on-abstract-class
src/jsii-diagnostic.ts Outdated Show resolved Hide resolved
@aws-cdk-automation aws-cdk-automation added this pull request to the merge queue Oct 30, 2024
Merged via the queue into main with commit 3322f9e Oct 30, 2024
70 checks passed
@aws-cdk-automation aws-cdk-automation deleted the huijbers/missing-prop-on-abstract-class branch October 30, 2024 18:15
aws-cdk-automation pushed a commit that referenced this pull request Oct 30, 2024
The following is a minimal reproducing example:

```ts
export interface ISomeInterface {
  readonly xyz?: string;
}

export abstract class SomeClass implements ISomeInterface {
}
```

This breaks code generation for C#, as the `SomeClass._Proxy` class we
generate will try to generate an `override` for the `Xyz` property,
which is missing from the `SomeClass` class itself.

This issue is exhibited by a difference in logic between how we iterate
the members of a class when we generate the class itself vs when we
generate its proxy.

However, after looking at the code I'm not convinced it's correct either
way (nor for Java), so to be safe we picked the solution where we force
the user to declare the property on the class.

The correct declaration would be:

```ts
export abstract class SomeClass implements ISomeInterface {
  public abstract readonly xyz?: string;
}
```

Related: https://github.com/aws/aws-cdk/pull/31946/files

---

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

[Apache 2.0 license]: https://www.apache.org/licenses/LICENSE-2.0

---------

Signed-off-by: github-actions <github-actions@github.com>
Co-authored-by: github-actions <github-actions@github.com>
(cherry picked from commit 3322f9e)
aws-cdk-automation pushed a commit that referenced this pull request Oct 30, 2024
The following is a minimal reproducing example:

```ts
export interface ISomeInterface {
  readonly xyz?: string;
}

export abstract class SomeClass implements ISomeInterface {
}
```

This breaks code generation for C#, as the `SomeClass._Proxy` class we
generate will try to generate an `override` for the `Xyz` property,
which is missing from the `SomeClass` class itself.

This issue is exhibited by a difference in logic between how we iterate
the members of a class when we generate the class itself vs when we
generate its proxy.

However, after looking at the code I'm not convinced it's correct either
way (nor for Java), so to be safe we picked the solution where we force
the user to declare the property on the class.

The correct declaration would be:

```ts
export abstract class SomeClass implements ISomeInterface {
  public abstract readonly xyz?: string;
}
```

Related: https://github.com/aws/aws-cdk/pull/31946/files

---

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

[Apache 2.0 license]: https://www.apache.org/licenses/LICENSE-2.0

---------

Signed-off-by: github-actions <github-actions@github.com>
Co-authored-by: github-actions <github-actions@github.com>
(cherry picked from commit 3322f9e)
@aws-cdk-automation
Copy link
Collaborator

💚 All backports created successfully

Status Branch Result
maintenance/v5.3
maintenance/v5.4

Questions ?

Please refer to the Backport tool documentation and see the Github Action logs for details

github-merge-queue bot pushed a commit that referenced this pull request Oct 30, 2024
…#1449)

# Backport

This will backport the following commits from `main` to
`maintenance/v5.3`:
- [fix: omitting optional property fails C# compilation
(#1448)](#1448)

<!--- Backport version: 9.5.1 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sorenlouv/backport)

Co-authored-by: Rico Hermans <rix0rrr@gmail.com>
github-merge-queue bot pushed a commit that referenced this pull request Oct 30, 2024
…#1450)

# Backport

This will backport the following commits from `main` to
`maintenance/v5.4`:
- [fix: omitting optional property fails C# compilation
(#1448)](#1448)

<!--- Backport version: 9.5.1 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sorenlouv/backport)

Co-authored-by: Rico Hermans <rix0rrr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants