feat: fromXxx() methods can be called with implicit or explicit parameters #309

otaviomacedo · 2022-09-26T10:08:04Z

Static factory methods with a “from” prefix allow users to import unowned constructs. Awslint enforces the rules that the first two arguments are the scope and id, followed by the parameters specific to the method.

With this change, users can call methods of this category, by either omitting the scope and id:

"Call": {
  "aws-cdk-lib.aws_s3.Bucket.fromBucketName": "read-bucket"
}

or by passing them explicitly using a new CDK-specific intrinsic function, CDK::Args, along with a new CDK-specific pseudo-parameter, CDK::Scope:

"Call": {
  "aws-cdk-lib.aws_s3.Bucket.fromBucketName": {
    "CDK::Args": [{ "Ref": "CDK::Scope" }, "ReadBucket", "read-bucket"]
  }
}

…eters

otaviomacedo · 2022-09-26T10:11:22Z

src/type-resolution/callables.ts

 ): MethodCall {
  const candidateFQN = assertFullyQualifiedStaticMethodCall(call);
  const candidateClass = typeSystem.findClass(candidateFQN);
-  const methods = enumLikeClassMethods(candidateClass);
+  const methods = staticNonVoidMethods(candidateClass);


Because methods like Bucket.fromBucketName() return an IBucket and thus get filtered out in enumLikeClassMethods().

otaviomacedo · 2022-09-26T10:18:44Z

src/type-resolution/callables.ts

+function prepareParameters(
+  x: ArrayLiteral,
+  callable: reflect.Callable,
+  logicalId?: string


This is the place where the logical id is used. Every other function in this PR that got logicalId added to its signature only passes it down in order to reach this point. I'm not very happy with this situation, but I haven't found a good way to solve it.

One option I considered was to not receive the logical id here and, instead, inject a new pseudo-parameter (something like CDK::ImplicitId) in the second argument. Then, at the point where the lazy resource is resolved, this pseudo-parameter would be replaced with the id. But this would obscure the logic by creating a tight coupling between two functions that know nothing about each other.

I'm not sure which way is better. But I do share your feelings about it being passed down all the way.

Re: pseudo-parameter. I don't think it's too bad to do this. Also it would not have to be a pseudo-parameter, but could be an AST type lazy-logical-id. We already have lazy evaluated resources, so why not have a lazy id?

Another option would be in introduce a context to type resolution. We will likely need something like it for #246 anyway. Basically a generic means of keeping track of where in the AST we are, what logicalID, what property, what the parent type is, etc.

Thanks for the suggestion. I agree with introducing context to type resolution. We should probably address that in its own change. For now, I've created a new intrinsic that lazily resolves the id. By default, it throws the error with the information we have. When it makes sense (static method calls on lazy resources) we replace that implementation with one that actually returns the id.

otaviomacedo · 2022-09-26T10:22:23Z

src/type-resolution/resolve.ts

  if (isBehavioralInterface(typeRef.type)) {
    return ResolvableExpressionType.BEHAVIORAL_INTERFACE;
  }
+  if (isSerializableInterface(typeRef.type)) {


This is bug fix. Many behavioral interfaces are being classified as structs because of the order of evaluation.

Do we have an explicit test case for this? If not maybe a comment would be useful.

otaviomacedo · 2022-09-26T10:29:56Z

test/type-resolution/callables.test.ts

+  });
+
+  expect(() => new TypedTemplate(template, { typeSystem })).toThrow(
+    /failed because the id could not be inferred/


I suppose we could try and generate an id even for cases like this. For example, the id could be the logical id of the resource + the property name (BucketencryptionKey in this case). But this would be a complex change in itself (and I'm not even sure this can be done for all cases).

This is a one way door decision. Allowing implicit calls later would not be a breaking change.

To clarify: Does the explicit version with CDK:Args work here?

Yes. Added a new test for this scenario.

mrgrain · 2022-09-26T10:54:25Z

src/evaluate/evaluate.ts

+    if (logicalId === 'CDK::Scope') {
+      return this.context.stack;
+    }


I guess this works. The other Pseudo Parameters are added to the context as evaluation references. Did you try to do that? I'd expect that to work without adding the implemenation detail to resolveReferences.

Ah, that's better. Thanks!

mrgrain · 2022-09-26T10:55:38Z

src/parser/template/resource.ts

+  if (!/^[A-Za-z0-9]+$/.test(logicalId)) {
+    throw new Error(
+      `Resource logical IDs must be alphanumeric. Got: '${logicalId}'.`
+    );
+  }


Do we need this?

I guess not. CFN will already have done this validation.

mrgrain · 2022-09-26T11:02:57Z

src/type-resolution/callables.ts

+function prepareParameters(
+  x: ArrayLiteral,
+  callable: reflect.Callable,
+  logicalId?: string


I'm not sure which way is better. But I do share your feelings about it being passed down all the way.

Re: pseudo-parameter. I don't think it's too bad to do this. Also it would not have to be a pseudo-parameter, but could be an AST type lazy-logical-id. We already have lazy evaluated resources, so why not have a lazy id?

Another option would be in introduce a context to type resolution. We will likely need something like it for #246 anyway. Basically a generic means of keeping track of where in the AST we are, what logicalID, what property, what the parent type is, etc.

mrgrain · 2022-09-26T11:05:18Z

src/type-resolution/resolve.ts

  if (isBehavioralInterface(typeRef.type)) {
    return ResolvableExpressionType.BEHAVIORAL_INTERFACE;
  }
+  if (isSerializableInterface(typeRef.type)) {


Do we have an explicit test case for this? If not maybe a comment would be useful.

mrgrain · 2022-09-26T11:08:12Z

test/type-resolution/callables.test.ts

+  });
+
+  expect(() => new TypedTemplate(template, { typeSystem })).toThrow(
+    /failed because the id could not be inferred/


To clarify: Does the explicit version with CDK:Args work here?

mrgrain

See inline comments/questions.

mrgrain

Love it! 😍

otaviomacedo added 2 commits September 26, 2022 10:51

feat: fromXxx() methods can be called with implicit or explicit param…

d3cfc71

…eters

Updated projen

095698c

otaviomacedo commented Sep 26, 2022

View reviewed changes

otaviomacedo requested a review from mrgrain September 26, 2022 10:30

mrgrain reviewed Sep 26, 2022

View reviewed changes

mrgrain requested changes Sep 26, 2022

View reviewed changes

Addressed comments

8416498

otaviomacedo requested a review from mrgrain September 26, 2022 13:40

Made LazyLogicalId serializable

c8e97fe

mrgrain approved these changes Sep 26, 2022

View reviewed changes

Merge branch 'main' into otaviom/fromXxx

6f11951

mergify bot merged commit a1a2785 into main Sep 26, 2022

mergify bot deleted the otaviom/fromXxx branch September 26, 2022 18:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: fromXxx() methods can be called with implicit or explicit parameters #309

feat: fromXxx() methods can be called with implicit or explicit parameters #309

otaviomacedo commented Sep 26, 2022

otaviomacedo Sep 26, 2022

otaviomacedo Sep 26, 2022

mrgrain Sep 26, 2022

otaviomacedo Sep 26, 2022

otaviomacedo Sep 26, 2022

mrgrain Sep 26, 2022

otaviomacedo Sep 26, 2022

otaviomacedo Sep 26, 2022

mrgrain Sep 26, 2022

otaviomacedo Sep 26, 2022

mrgrain Sep 26, 2022

otaviomacedo Sep 26, 2022

mrgrain Sep 26, 2022

otaviomacedo Sep 26, 2022

mrgrain Sep 26, 2022

mrgrain Sep 26, 2022

mrgrain Sep 26, 2022

mrgrain left a comment

mrgrain left a comment

feat: fromXxx() methods can be called with implicit or explicit parameters #309

feat: fromXxx() methods can be called with implicit or explicit parameters #309

Conversation

otaviomacedo commented Sep 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrgrain left a comment

Choose a reason for hiding this comment

mrgrain left a comment

Choose a reason for hiding this comment