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

Generate RBS #2950

Closed
2 tasks done
ksss opened this issue Nov 20, 2023 · 15 comments
Closed
2 tasks done

Generate RBS #2950

ksss opened this issue Nov 20, 2023 · 15 comments
Labels
feature-request A feature should be added or improved.

Comments

@ksss
Copy link
Contributor

ksss commented Nov 20, 2023

Describe the feature

Currently, Ruby code generation is done by build_tools, but I propose that RBS files also be generated during this Ruby code generation. RBS refers to Ruby's standard type files and tools, which can be bundled with the gem itself.
I have already been conducting experimental operations for over a year.
In a repository similar to DefinitelyTyped for RBS, I have been generating RBS from apis/**/*.json, and it has been used within the RBS community.

https://github.com/ruby/gem_rbs_collection/tree/main/generators/aws-sdk-rbs-generator

Use Case

Using LSP (Language Server Protocol) in recent years, we can receive coding assistance as follows:

  • Automatic suggestion of methods
  • Type checking of argument names and method names
  • Usage similar to API documentation

Moreover, by being incorporated into the upstream, it becomes possible to accurately manage the Ruby code and corresponding RBS for each version.

Proposed Solution

I will integrate the existing aws-sdk-code-generator and the aws-sdk-rbs-generator that I implemented, and organize the workflow so that both Ruby code and RBS code are generated and released simultaneously.
Prepare a sig directory in each gem and generate RBS files within this directory. This will not affect the Ruby code at all.

Other Information

Since I have made numerous commits to RBS, I can provide detailed support regarding RBS.

https://github.com/ruby/rbs/graphs/contributors
https://github.com/ruby/gem_rbs_collection/tree/main/generators/aws-sdk-rbs-generator

Acknowledgements

  • I may be able to implement this feature request
  • This feature might incur a breaking change

SDK version used

3

Environment details (OS name and version, etc.)

any

@ksss ksss added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Nov 20, 2023
@mullermp
Copy link
Contributor

Thanks for reaching out! We met with Soutaro at RubyConf and he told us about your project. We would love to get RBS typing into v3 and would appreciate any help doing so. This would certainly be a large project and may take some time. As far as generation goes, v3 code generation uses Mustache templates. Looking at aws-sdk-rbs-generator, I think most of the logic goes away when using the existing aws-sdk-code-generator. I think we should get a simple gem working, like lambda, as a proof of concept. There's no need to be backwards compatible inside aws-sdk-code-generator within reason. You can test builds with rake build:aws-sdk-lambda for example. If you start a feature branch, we would be happy to contribute and review when able.

@ksss
Copy link
Contributor Author

ksss commented Nov 26, 2023

@mullermp
I apologize for the late reply; I was down with a cold 🤧
As you said, I also heard from @soutaro that there are plans to add RBS to aws-sdk-ruby.
Let's start with lambda and gradually expand.
Thanks for the follow-up.

@ksss
Copy link
Contributor Author

ksss commented Nov 29, 2023

Issue1 We need RBS for aws-sdk-core

There are RBS that should be defined in aws-sdk-core, such as Aws::Errors::ServiceError, Aws::EmptyStructure, and Seahorse::Client::Base.
Firstly, we need to introduce RBS in the aws-sdk-core gem, and gems that want to incorporate RBS will need to change their dependencies to use this core version or later. Is this acceptable?

@mullermp
Copy link
Contributor

Yes. That's fine! A minimal set would be appreciated, as we will have to maintain this going forward as a small team. When we are done, we can setup the minimum version constraints between gems and core.

We should also setup steep on CI to check new features are added.

@ksss
Copy link
Contributor Author

ksss commented Nov 30, 2023

OK. Since aws-sdk-core is outside the scope of automatic generation and has a broad impact, I intend to limit it to only the minimum necessary RBS.

@mullermp mullermp removed the needs-triage This issue or PR still needs to be triaged. label Dec 4, 2023
@ksss
Copy link
Contributor Author

ksss commented Dec 19, 2023

Issue2 CI for RBS files

We should be able to automatically verify the correctness of the output RBS for maintainability.
There are several things we can do to achieve this.

Summary

I recommend implementing the following

  • Run $ rbs validate
  • Run rspec with RBS::Test and modify spec file a little.

Details

There are 4 possible methods for testing RBS.

static dynamic
light $ rbs validate RBS::Test
heavy $ steep check RBS::UnitTest

$ rbs validate

$ rbs validate cli command can statically validate RBS files.
It checks for various RBS inconsistencies as well as syntax.
This can already be implemented as a rake task.

RBS::Test

RBS::Test is a tool included with the rbs gem.
It allows for dynamic checking to see if the types of arguments and return values for a specific class match the actual behavior by running existing rspec tests.

You can find type errors without additional code.
I have already been able to find several type errors by having them working at hand.

However, there are several problems with integrating this method into CI.

Need rbs v3.4.0

I have fixed the problems I found throughout this project with the rbs gem. This code will be released in rbs v3.4.0.
rbs v3.4.0 has been released pre1, but the official version has not yet been released.

Need to give up or small fix

We need to give up a little type correctness or small modify spec file.
Which would you prefer for the aws-sdk-ruby team?
My personal opinion is that I prefer the convenience method and use annotations in exceptional cases.

give up a little type correctness

I got 1155 examples, 2 failures for the rspec of aws-sdk-s3 in my test using RBS::Test.

it 'can be used with a Resource client' do
resource = S3::Resource.new(client: client)
expect(resource.client.config).to eq(api_client.config)
end

it 'can be used with a Resource client' do
resource = S3::Resource.new(client: client)
expect(resource.client.config).to eq(api_client.config)
end

That is, when the Encryption client is used as the Resource class.

If we try to express this behavior precisely with types, we would lose the convenience of types in many cases or have to generate distorted RBS.

small modify spec file

Another idea is to tag test files to skip test cases only when using this RBS::Test.

it '...', rbs_skip: true do
  # ...
end
$ rspec --tag '~rbs_skip'

In total there are 90 spec files that are not auto-generated, and 43 aws-sdk-s3 spec files.
Since we found 2 cases in aws-sdk-s3, we can simply calculate that there are about 5 such cases in total.

$ steep check

There are methods to perform this on the implementation and on the sample code.
However, I do not recommend either for this project.

On the implementation

Applying steep check to the implementation can assist in development and check for correctness in types.
However, there are several issues with this project, which is why I personally do not recommend using it within this project.

The Project is Too Large

Using steep in CI implies the need for high RBS coverage and knowledge of RBS.
This project is large, so it takes time to prepare all types without any omissions.

Auto-Generated Code

Most of the code is auto-generated, so using it for development support offers little benefit.

Keyword Arguments

In many cases, this project uses Hash options instead of keyword arguments.
Therefore, even if RBS uses keyword arguments, the implementation does not, so steep will report an error.
If you stop using keyword arguments in RBS, it will then lose convenience on the user side.

RBS::UnitTest

This is how to write type-specific test code as described below.
https://github.com/ruby/rbs/blob/master/docs/gem.md#testing-your-type-definition

We can ensure the accuracy of RBS against the actual behavior.
However, additional test code needs to be written. Knowledge about types and maintenance costs are required.
Therefore, I don't think it's a suitable option for this project.

@alextwoods
Copy link
Contributor

My guess is that the S3 tests are likely the most complex/likely to have tests that need to be skipped.

I would lean towards adding an rbs_skip tag and just skipping those tests. Though to be honest looking at those two specific test cases - I don't see significant benefit in them (they're only testing that you can pass in the client - not that it is actually api equivalent, since the Resource client does no validation on what is passed in) - I actually think for these two specific cases, we should just remove those test cases. But I think a general strategy of an rbs_skip tag makes sense.

@ksss
Copy link
Contributor Author

ksss commented Dec 21, 2023

@alextwoods
Thank you for your response.
Finally, I plan to run $ rbs validate and RBS::Test with rspec on aws-sdk-core and aws-sdk-s3.
I've realized that cases to skip should include exceptional cases in the client method (such as when data becomes nil), so it looks like the number will increase to about 13 cases.

@ksss
Copy link
Contributor Author

ksss commented Dec 21, 2023

Issue3 Handwritten code type

Some services have handwritten custom code.
Should we add RBS for all of these before release?
Or should we first release RBS for the auto-generated code and gradually add types for the handwritten code? (I think this will take time).

@ksss
Copy link
Contributor Author

ksss commented Dec 21, 2023

I have created a branch here with the generated results for all services at this time.
Please check it out.

https://github.com/ksss/aws-sdk-ruby/tree/rbs-full-generated

@mullermp
Copy link
Contributor

Thank you! We will check this out this or next week, please allow time for vacation.

@mullermp
Copy link
Contributor

I think code generation cases are fine for now.

@ksss
Copy link
Contributor Author

ksss commented Dec 27, 2023

No problem at all. Have a happy New Year 🐉

@mullermp
Copy link
Contributor

Merged with #2961

Copy link

This issue is now closed. Comments on closed issues are hard for our team to see.
If you need more assistance, please open a new issue that references this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request A feature should be added or improved.
Projects
None yet
Development

No branches or pull requests

3 participants