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

Refactor runtimeType usage in codegen #1933

Closed
wants to merge 25 commits into from

Conversation

Velfi
Copy link
Contributor

@Velfi Velfi commented Nov 1, 2022

Description

This primarily does three things:

  • (huge change) unify the instantiation of runtime types during code generation
  • (small change) update AWS codegen to support customizing specific sections. This works just like the existing customization hook for operations and configs.
  • (small change) fix some gradle stuff that had to do with trying to use delegation from within a plugin block + issues with the cargo commands

Info for smithy contribs:

  • awsRuntimeDependency has been renamed to awsRuntimeCrate in order to match smithyRuntimeCrate. Additionally, declaration of the runtime crates has been centralized in software.amazon.smithy.rustsdk.AwsRuntimeDependency for AWS types and software.amazon.smithy.rust.codegen.core.rustlang.CargoDependency for smithy types.

  • The RuntimeConfig runtime crate functions now return RuntimeTypes instead of CargoDependencies. This better matches their usage in codegen. If you want to access the cargo dependency, use the awsRuntimeCrate/smithyRuntimeCrate functions directly.

    before: rust("#T", CargoDependency.SmithyClient(runtimeConfig).asType())
    after: rust("#T", runtimeConfig.smithyClient())

  • Service-specific decorators in SDK codegen have been moved into software.amazon.smithy.rustsdk.servicedecorators. All other decorators have been moved to software.amazon.smithy.rustsdk.decorators.
    -RustCodegenDecorator's name field has been removed. If you need to log the name of a decorator, use Kotlin's builtin class field:

    val someDecorator = SomeDecorator()
    print(someDecorator::class)
    // prints "class SomeDecorator"
  • The class AwsCodegenDecorator.kt has been renamed to AwsCombinedCodegenDecorator.kt. A new class, now named AwsCodegenDecorator is an AWS-specific subclass of RustCodegenDecorator. AwsCodegenDecorator supports the customization of the FromSdkConfigForBuilder section.

    For example, adding this customization to codegen as part of a decorator:

    class SleepImplFromSdkConfig : AwsCustomization() {
        override fun section(section: AwsSection): Writable = writable {
            when (section) {
                is AwsSection.FromSdkConfigForBuilder -> rust("builder.set_sleep_impl(input.sleep_impl());")
            }
        }
    }

    would result in this Rust code being generated:

    // In aws-sdk-<service>/src/config.rs
    impl From<&SdkConfig> for Builder {
        fn from(input: &#{SdkConfig}) -> Self {
            let mut builder = Builder::default();
            builder.set_sleep_impl(input.sleep_impl());
            builder
        }
    }
  • CombinedCodegenDecorator's orderedDecorators field is now protected instead of private so that the new AwsCodegenDecorator subclass can use it.

  • runtimeCrate has been renamed to smithyRuntimeCrate and now requires users to pass the "smithy-" prefix as part of the crate name:

    before: val smithyHttp = runtimeCrate("http").asType()
    after: val smithyHttp = smithyRuntimeCrate("smithy-http").asType()

Testing

ran existing tests

Checklist

  • I have updated CHANGELOG.next.toml if I made changes to the smithy-rs codegen or runtime crates
  • I have updated CHANGELOG.next.toml if I made changes to the AWS SDK, generated SDK code, or SDK runtime crates

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Velfi added 4 commits November 1, 2022 16:01
add: AwsCustomization NamedSectionGenerator
add: AwsSection for `From<SdkConfig> for service::config::Builder` codegen
rename: AwsCodegenDecorator to AwsCombinedCodegenDecorator
add: new interface AwsCodegenDecorator that respects AwsCustomizations
format: unify casing of runtime type function names
format: unify casing of CargoDependency function names
rename: args of FluentClientGenerics.sendBounds method to be more explicit
update: make orderedDecorators field of CombinedCodegenDecorator accessible to inheritors
fix: spelling issues highlighted by IntelliJ
update: make protected fields private where possible
@Velfi Velfi requested review from a team as code owners November 1, 2022 21:15
@Velfi Velfi marked this pull request as draft November 1, 2022 21:57
@Velfi
Copy link
Contributor Author

Velfi commented Nov 1, 2022

Keeping this in draft until I can get around to writing the changelog

@github-actions
Copy link

github-actions bot commented Nov 2, 2022

A new generated diff is ready to view.

A new doc preview is ready to view.

add: missing copyright headers
fix: endpoints test by differentiating between usages of aws-endpoints
@github-actions
Copy link

github-actions bot commented Nov 3, 2022

A new generated diff is ready to view.

A new doc preview is ready to view.

Velfi added 2 commits November 4, 2022 10:47
update: rename servicecustomizations to servicedecorators
remove: decorator name field
add: CHANGELOG.next.toml entries
@Velfi Velfi marked this pull request as ready for review November 4, 2022 15:50
@github-actions
Copy link

github-actions bot commented Nov 4, 2022

A new generated diff is ready to view.

A new doc preview is ready to view.

Info for smithy contribs:

- `awsRuntimeDependency` has been renamed to `awsRuntimeCrate` in order to match `smithyRuntimeCrate`. Additionally, declaration of the runtime crates has been centralized in `software.amazon.smithy.rustsdk.AwsRuntimeDependency` for AWS types and `software.amazon.smithy.rust.codegen.core.rustlang.CargoDependency` for smithy types.
- The `RuntimeConfig` runtime crate functions now return `RuntimeTypes` instead of `CargoDependencies`. This better matches their usage in codegen. If you want to access the cargo dependency, use the `awsRuntimeCrate`/`smithyRuntimeCrate` functions directly.

  before: `rust("#T", CargoDependency.SmithyClient(runtimeConfig).asType())`
  after: `rust("#T", runtimeConfig.smithyClient())`
- Service-specific decorators in SDK codegen have been moved into `software.amazon.smithy.rustsdk.servicedecorators`. All other decorators have been moved to `software.amazon.smithy.rustsdk.decorators`.
-`RustCodegenDecorator`'s name field has been removed. If you need to log the name of a decorator, use Kotlin's builtin `class` field:

  ```kotlin
  val someDecorator = SomeDecorator()
  print(someDecorator::class)
  // prints "class SomeDecorator"
  ```
- The class `AwsCodegenDecorator.kt` has been renamed to `AwsCombinedCodegenDecorator.kt`. A new class, now named `AwsCodegenDecorator` is an AWS-specific subclass of `RustCodegenDecorator`. `AwsCodegenDecorator` supports the customization of the `FromSdkConfigForBuilder` section.

  For example, adding this customization to codegen as part of a decorator:

  ```kotlin
  class SleepImplFromSdkConfig : AwsCustomization() {
      override fun section(section: AwsSection): Writable = writable {
          when (section) {
              is AwsSection.FromSdkConfigForBuilder -> rust("builder.set_sleep_impl(input.sleep_impl());")
          }
      }
  }
  ```

  would result in this Rust code being generated:

  ```rust
  // In aws-sdk-<service>/src/config.rs
  impl From<&SdkConfig> for Builder {
      fn from(input: &#{SdkConfig}) -> Self {
          let mut builder = Builder::default();
          builder.set_sleep_impl(input.sleep_impl());
          builder
      }
  }
  ```

- `CombinedCodegenDecorator`'s `orderedDecorators` field is now protected instead of private so that the new `AwsCodegenDecorator` subclass can use it.
- `runtimeCrate` has been renamed to `smithyRuntimeCrate` and now requires users to pass the "smithy-" prefix as part of the crate name:

  before: `val smithyHttp = runtimeCrate("http").asType()`
  after: `val smithyHttp = smithyRuntimeCrate("smithy-http").asType()`
@Velfi
Copy link
Contributor Author

Velfi commented Nov 4, 2022

[this comment was moved into the PR description]

@github-actions
Copy link

github-actions bot commented Nov 4, 2022

A new generated diff is ready to view.

A new doc preview is ready to view.

Copy link
Collaborator

@jdisanti jdisanti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome! Lots of nice changes in here, and I love the push for more consistency on the runtime types.

}
}

fun generateImplFromRefSdkConfigForConfigBuilder(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be in SdkConfigDecorator?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think no because I feel that all the section writing code should be defined in one place. The combined decorator is also the place where we have access to the full list of decorators that modify an AwsSection.

Copy link
Collaborator

@jdisanti jdisanti Nov 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Organizationally, this isn't going to work out well since people will think to look in SdkConfigDecorator for this code rather than in AwsCombinedCodegenDecorator. The full list of decorators can be passed into the customizations if it's required (and I think we do that in codegen-core/codegen-client).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Russell is adding the ability to do this as part of #1953, I can take the change from that or wait for the merge.

settings.gradle.kts Show resolved Hide resolved
tools/crate-hasher/src/file_list.rs Show resolved Hide resolved
Velfi added 3 commits November 8, 2022 15:05
remove: need to specify `CargoDependency`
refactor: get runtime types from `RuntimeType` instead of `RuntimeConfig`
@github-actions
Copy link

github-actions bot commented Nov 9, 2022

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions
Copy link

github-actions bot commented Nov 9, 2022

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@@ -11,8 +11,8 @@ import software.amazon.smithy.model.traits.AnnotationTrait

/** Synthetic trait that indicates an operation is presignable. */
// TODO(https://github.com/awslabs/smithy/pull/897) this can be replaced when the trait is added to Smithy.
class PresignableTrait(val syntheticOperationId: ShapeId) : AnnotationTrait(ID, Node.objectNode()) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this was never used?

"HeaderMap" to RuntimeType.http.member("HeaderMap"),
"JsonError" to CargoDependency.smithyJson(runtimeConfig).asType().member("deserialize::Error"),
"Response" to RuntimeType.http.member("Response"),
"Bytes" to Bytes.toType().resolve("Bytes"),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

personally, I do actually appreciate the prefix so I know this is a rust library but not a super strong preference

"HeaderMap" to RuntimeType.http.member("HeaderMap"),
"Response" to RuntimeType.http.member("Response"),
"XmlError" to CargoDependency.smithyXml(runtimeConfig).asType().member("decode::XmlError"),
"Bytes" to Bytes.toType().resolve("Bytes"),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if replacing the single-source-of-truth Bytes with a resolved package everywhere is useful? Maybe a CommonTypes module?

@rcoh
Copy link
Collaborator

rcoh commented Nov 10, 2022

can you move the "changelog" comment into the PR description so it's easier to find?

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@Velfi
Copy link
Contributor Author

Velfi commented Nov 16, 2022

I'm going to close this and re-open separate PRs, each with a subset of the changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants