Skip to content

Conversation

st0012
Copy link
Member

@st0012 st0012 commented Apr 25, 2025

ruby/rbs recently received a few enhancements for new keywords and inline parsing:

So the major change of this PR is to reimplement them under the c-api structure.

soutaro and others added 30 commits April 9, 2025 16:13
A sub buffer is a buffer that contains slices of a buffer.
* Position APIs returns the absolute position of the buffer
* Use `#local_location` to access the local positions of a sub-buffer
Bumps [csv](https://github.com/ruby/csv) from 3.3.3 to 3.3.4.
- [Release notes](https://github.com/ruby/csv/releases)
- [Changelog](https://github.com/ruby/csv/blob/main/NEWS.md)
- [Commits](ruby/csv@v3.3.3...v3.3.4)

---
updated-dependencies:
- dependency-name: csv
  dependency-version: 3.3.4
  dependency-type: indirect
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [strscan](https://github.com/ruby/strscan) from 3.1.2 to 3.1.3.
- [Release notes](https://github.com/ruby/strscan/releases)
- [Changelog](https://github.com/ruby/strscan/blob/master/NEWS.md)
- [Commits](ruby/strscan@v3.1.2...v3.1.3)

---
updated-dependencies:
- dependency-name: strscan
  dependency-version: 3.1.3
  dependency-type: indirect
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
….3.4

Bump csv from 3.3.3 to 3.3.4 in /steep
…an-3.1.3

Bump strscan from 3.1.2 to 3.1.3 in /steep
Bumps [rubocop-ast](https://github.com/rubocop/rubocop-ast) from 1.44.0 to 1.44.1.
- [Release notes](https://github.com/rubocop/rubocop-ast/releases)
- [Changelog](https://github.com/rubocop/rubocop-ast/blob/master/CHANGELOG.md)
- [Commits](rubocop/rubocop-ast@v1.44.0...v1.44.1)

---
updated-dependencies:
- dependency-name: rubocop-ast
  dependency-version: 1.44.1
  dependency-type: indirect
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [parser](https://github.com/whitequark/parser) from 3.3.7.4 to 3.3.8.0.
- [Changelog](https://github.com/whitequark/parser/blob/master/CHANGELOG.md)
- [Commits](whitequark/parser@v3.3.7.4...v3.3.8.0)

---
updated-dependencies:
- dependency-name: parser
  dependency-version: 3.3.8.0
  dependency-type: indirect
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
…-1.44.1

Bump rubocop-ast from 1.44.0 to 1.44.1
soutaro and others added 13 commits April 18, 2025 06:44
…t-runtime-0.5.12024

Bump sorbet-runtime from 0.5.12020 to 0.5.12024 in /steep
Bumps [sorbet-runtime](https://github.com/sorbet/sorbet) from 0.5.12024 to 0.5.12026.
- [Release notes](https://github.com/sorbet/sorbet/releases)
- [Commits](https://github.com/sorbet/sorbet/commits)

---
updated-dependencies:
- dependency-name: sorbet-runtime
  dependency-version: 0.5.12026
  dependency-type: indirect
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
…t-runtime-0.5.12026

Bump sorbet-runtime from 0.5.12024 to 0.5.12026 in /steep
Bumps [stringio](https://github.com/ruby/stringio) from 3.1.6 to 3.1.7.
- [Release notes](https://github.com/ruby/stringio/releases)
- [Changelog](https://github.com/ruby/stringio/blob/master/NEWS.md)
- [Commits](ruby/stringio@v3.1.6...v3.1.7)

---
updated-dependencies:
- dependency-name: stringio
  dependency-version: 3.1.7
  dependency-type: indirect
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [nokogiri](https://github.com/sparklemotion/nokogiri) from 1.18.7 to 1.18.8.
- [Release notes](https://github.com/sparklemotion/nokogiri/releases)
- [Changelog](https://github.com/sparklemotion/nokogiri/blob/main/CHANGELOG.md)
- [Commits](sparklemotion/nokogiri@v1.18.7...v1.18.8)

---
updated-dependencies:
- dependency-name: nokogiri
  dependency-version: 1.18.8
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
…18.8

Bump nokogiri from 1.18.7 to 1.18.8
Bumps [sorbet-runtime](https://github.com/sorbet/sorbet) from 0.5.12026 to 0.5.12028.
- [Release notes](https://github.com/sorbet/sorbet/releases)
- [Commits](https://github.com/sorbet/sorbet/commits)

---
updated-dependencies:
- dependency-name: sorbet-runtime
  dependency-version: 0.5.12028
  dependency-type: indirect
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
…t-runtime-0.5.12028

Bump sorbet-runtime from 0.5.12026 to 0.5.12028 in /steep
Bumps [sorbet-runtime](https://github.com/sorbet/sorbet) from 0.5.12028 to 0.5.12032.
- [Release notes](https://github.com/sorbet/sorbet/releases)
- [Commits](https://github.com/sorbet/sorbet/commits)

---
updated-dependencies:
- dependency-name: sorbet-runtime
  dependency-version: 0.5.12032
  dependency-type: indirect
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
…t-runtime-0.5.12032

Bump sorbet-runtime from 0.5.12028 to 0.5.12032 in /steep
Bumps [rubocop](https://github.com/rubocop/rubocop) from 1.75.2 to 1.75.3.
- [Release notes](https://github.com/rubocop/rubocop/releases)
- [Changelog](https://github.com/rubocop/rubocop/blob/master/CHANGELOG.md)
- [Commits](rubocop/rubocop@v1.75.2...v1.75.3)

---
updated-dependencies:
- dependency-name: rubocop
  dependency-version: 1.75.3
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
rbs_location_list_node_t *head;
rbs_location_list_node_t *tail;
size_t length;
} rbs_location_list_t;
Copy link
Member Author

Choose a reason for hiding this comment

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

The new MethodTypesAnnotation type node has an attribute called vertical_bar_locations that holds a list of locations. Since we currently don't have a data structure for that, I added this by mimicking the rbs_node_list struct.

rbs_parser_set_error(parser, parser->current_token, false, "Unexpected error");
return false;
}
switch (parser->current_token.type) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Following upstream to use switch

}

VALUE rbs_loc_to_ruby_location(rbs_translation_context_t ctx, rbs_location_t *source_loc) {
if (source_loc == NULL) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is for the new ReturnTypeAnnotation as its comment_location attribute is a nilable Location.
The alternatives would be either:

  • Add a new nilable_rbs_location C type to the template and config.yml
  • Or, have a case in templates specifically for comment_location

@st0012 st0012 requested review from Morriar and amomchilov April 25, 2025 12:21
st0012 added a commit that referenced this pull request Apr 25, 2025
`rbs_node_destroy`, `rbs_hash_free`, `rbs_node_list_free` are only
calling each other recursively without any real freeing logic.

This is the result of previous efforts to allocate all nodes on the
arena. So we don't need these functions anymore.

Discovered while working on #41
self.class.new(
location:, prefix_location:,
type: type.map_type_name { yield _1 }
) #: self
Copy link

Choose a reason for hiding this comment

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

❤️

Copy link

@amomchilov amomchilov left a comment

Choose a reason for hiding this comment

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

Looks good overall! I suggest some small changes, but none are blockers.

Great work, Stan!


rbs_parser_advance(parser);

rbs_location_list_append(bar_locations, bar_location);

Choose a reason for hiding this comment

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

From what I can tell, this is the only place where rbs_location_list_append is called. We have access to the allocator here.

Would it be preferable to pass is as the first param, so that the rbs_location_list_t doesn't have to store it? I'm not sure either way, cc @Morriar

Copy link

Choose a reason for hiding this comment

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

Let's focus on getting this merged to avoid more rebasing. The change can be done later.

Ideally we should refactor this so we don't need a location list.

Choose a reason for hiding this comment

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

Agree

I suggest some small changes, but none are blockers.

st0012 and others added 2 commits April 30, 2025 21:23
Co-authored-by: Alexander Momchilov <[email protected]>
Co-authored-by: Alexander Momchilov <[email protected]>
@st0012 st0012 merged commit 21752fb into c-api Apr 30, 2025
19 of 20 checks passed
@st0012 st0012 deleted the merge-master branch April 30, 2025 13:37
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.

4 participants