Skip to content

Commit

Permalink
fix(dontet): excessive overrides generated (#3355)
Browse files Browse the repository at this point in the history
A mis-understanding of the jsii specification early on was carried over
in the .NET code generators until now, which caused all methods and
properties that are overrides (of other JS implementations) to result in
jsii overrides being registered by the runtime.

This behavior caused unnecessary round-trips between the .NET CLR and
the node sidekick process, which affected the performance of .NET
bindings (unnecessary round-trips with JSON encoding aren't free), and
had a tendency to hit obscure edge case bugs in the jsii kernel's Ser/De
behavior.

This PR removes all `isOverride: true` declarations from generated .NET
code and neutralizes the behavior of specifying it to be true.
User-defined overrides (.NET code overriding JS code) continue to work
as they previously did (the `isOverride` attribute should simply not
have existed, ever).



---

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
  • Loading branch information
Romain Marcadier committed Jan 31, 2022
1 parent 20419b4 commit 5460d66
Show file tree
Hide file tree
Showing 7 changed files with 171 additions and 178 deletions.
Expand Up @@ -76,7 +76,7 @@ IEnumerable<Override> GetMethodOverrides()
var inheritedAttribute = method.GetAttribute<JsiiMethodAttribute>();
var uninheritedAttribute = method.GetAttribute<JsiiMethodAttribute>(false);

if ((inheritedAttribute != null && uninheritedAttribute == null) || uninheritedAttribute?.IsOverride == true)
if (inheritedAttribute != null && uninheritedAttribute == null)
{
yield return new Override(method: (inheritedAttribute ?? uninheritedAttribute)!.Name);
}
Expand All @@ -93,7 +93,7 @@ IEnumerable<Override> GetPropertyOverrides()
var inheritedAttribute = property.GetAttribute<JsiiPropertyAttribute>();
var uninheritedAttribute = property.GetAttribute<JsiiPropertyAttribute>(false);

if ((inheritedAttribute != null && uninheritedAttribute == null) || uninheritedAttribute?.IsOverride == true)
if (inheritedAttribute != null && uninheritedAttribute == null)
{
yield return new Override(property: (inheritedAttribute ?? uninheritedAttribute)!.Name);
}
Expand Down
Expand Up @@ -12,6 +12,7 @@ public sealed class JsiiMethodAttribute : Attribute
string? returnsJson = null,
string? parametersJson = null,
bool isAsync = false,
// Unused, retained for backwards-compatibility
bool isOverride = false)
{
Name = name ?? throw new ArgumentNullException(nameof(name));
Expand All @@ -23,7 +24,6 @@ public sealed class JsiiMethodAttribute : Attribute
: JsonConvert.DeserializeObject<Parameter[]>(parametersJson)
?? throw new ArgumentException("Invalid JSON descriptor", nameof(parametersJson));
IsAsync = isAsync;
IsOverride = isOverride;
}

public string Name { get; }
Expand All @@ -34,7 +34,5 @@ public sealed class JsiiMethodAttribute : Attribute


public bool IsAsync { get; }

public bool IsOverride { get; }
}
}
}
Expand Up @@ -7,22 +7,24 @@ namespace Amazon.JSII.Runtime.Deputy
[AttributeUsage(AttributeTargets.Property)]
public sealed class JsiiPropertyAttribute : Attribute, IOptionalValue
{
public JsiiPropertyAttribute(string name, string typeJson, bool isOptional = false, bool isOverride = false)
public JsiiPropertyAttribute(
string name,
string typeJson,
bool isOptional = false,
// Unused, retained for backwards-compatibility
bool isOverride = false)
{
Name = name ?? throw new ArgumentNullException(nameof(name));
Type = JsonConvert.DeserializeObject<TypeReference>(typeJson ??
throw new ArgumentNullException(nameof(typeJson)))
?? throw new ArgumentException("Invalid JSON descriptor", nameof(typeJson));
IsOptional = isOptional;
IsOverride = isOverride;
}

public string Name { get; }

public TypeReference Type { get; }

public bool IsOptional { get; }

public bool IsOverride { get; }
}
}
}
2 changes: 1 addition & 1 deletion packages/jsii-pacmak/lib/targets/dotnet/dotnetgenerator.ts
Expand Up @@ -942,7 +942,7 @@ export class DotNetGenerator extends Generator {
if (prop.optional) {
this.code.line('[JsiiOptional]');
}
this.dotnetRuntimeGenerator.emitAttributesForProperty(prop, datatype);
this.dotnetRuntimeGenerator.emitAttributesForProperty(prop);

let isOverrideKeyWord = '';
let isVirtualKeyWord = '';
Expand Down
15 changes: 4 additions & 11 deletions packages/jsii-pacmak/lib/targets/dotnet/dotnetruntimegenerator.ts
Expand Up @@ -78,8 +78,6 @@ export class DotNetRuntimeGenerator {
cls: spec.ClassType | spec.InterfaceType,
method: spec.Method /*, emitForProxyOrDatatype: boolean = false*/,
): void {
const isOverride =
spec.isClassType(cls) && method.overrides ? ', isOverride: true' : '';
const isAsync =
spec.isClassType(cls) && method.async ? ', isAsync: true' : '';
const parametersJson = method.parameters
Expand All @@ -92,28 +90,23 @@ export class DotNetRuntimeGenerator {
.replace(/"/g, '\\"')
.replace(/\\{2}"/g, 'test')}"`
: '';
const jsiiAttribute = `[JsiiMethod(name: "${method.name}"${returnsJson}${parametersJson}${isAsync}${isOverride})]`;
const jsiiAttribute = `[JsiiMethod(name: "${method.name}"${returnsJson}${parametersJson}${isAsync})]`;
this.code.line(jsiiAttribute);
this.emitDeprecatedAttributeIfNecessary(method);
}

/**
* Emits the proper jsii .NET attribute for a property
*
* Ex: [JsiiProperty(name: "foo", typeJson: "{\"fqn\":\"@scope/jsii-calc-base-of-base.Very\"}", isOptional: true, isOverride: true)]
* Ex: [JsiiProperty(name: "foo", typeJson: "{\"fqn\":\"@scope/jsii-calc-base-of-base.Very\"}", isOptional: true)]
*/
public emitAttributesForProperty(
prop: spec.Property,
datatype = false,
): void {
// If we are on a datatype then we want the property to override in Jsii
const isJsiiOverride = datatype ? ', isOverride: true' : '';
public emitAttributesForProperty(prop: spec.Property): void {
const isOptionalJsii = prop.optional ? ', isOptional: true' : '';
const jsiiAttribute =
`[JsiiProperty(name: "${prop.name}", ` +
`typeJson: "${JSON.stringify(prop.type)
.replace(/"/g, '\\"')
.replace(/\\{2}"/g, 'test')}"${isOptionalJsii}${isJsiiOverride})]`;
.replace(/\\{2}"/g, 'test')}"${isOptionalJsii})]`;
this.code.line(jsiiAttribute);
this.emitDeprecatedAttributeIfNecessary(prop);
}
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit 5460d66

Please sign in to comment.