Add built-in RBS parser support#9
Conversation
st0012
temporarily deployed
to
fork-preview-protection
GitHub Actions
Inactive
st0012
force-pushed
the
codex/native-rbs-parser
branch
from
095aab3 to
e78c48d
Compare
st0012
temporarily deployed
to
fork-preview-protection
GitHub Actions
Inactive
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e78c48db40
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
st0012
force-pushed
the
codex/native-rbs-parser
branch
from
e78c48d to
e143b6f
Compare
st0012
temporarily deployed
to
fork-preview-protection
GitHub Actions
Inactive
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e143b6fbce
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
st0012
force-pushed
the
codex/native-rbs-parser
branch
from
e143b6f to
6aceb1a
Compare
st0012
temporarily deployed
to
fork-preview-protection
GitHub Actions
Inactive
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6aceb1a6b4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
st0012
force-pushed
the
codex/native-rbs-parser
branch
from
6aceb1a to
ab88253
Compare
st0012
temporarily deployed
to
fork-preview-protection
GitHub Actions
Inactive
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab88253410
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
st0012
temporarily deployed
to
fork-preview-protection
GitHub Actions
Inactive
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a74a388ef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
st0012
temporarily deployed
to
fork-preview-protection
GitHub Actions
Inactive
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c855bed5b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
st0012
temporarily deployed
to
fork-preview-protection
GitHub Actions
Inactive
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b096ce950
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
st0012
temporarily deployed
to
fork-preview-protection
GitHub Actions
Inactive
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea51f183d3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| attribute.type_signature_lines = type_signature_lines | ||
| context.add_attribute attribute |
There was a problem hiding this comment.
When a Ruby class already has a normal method and an explicit non-sig/ RBS input declares it as an attribute, e.g. attr_reader name: String, this creates an RDoc::Attr with type_signature_lines but Context#add_attribute refuses to register it because @methods_hash already contains #name; it only fills an empty comment on the existing method. Since first-class RBS files are not merged through Store#rbs_signature_for, the String signature is dropped from generated docs. Merge the signature into the known method before the attribute object is discarded.
Useful? React with 👍 / 👎.
st0012
temporarily deployed
to
fork-preview-protection
GitHub Actions
Inactive
st0012
temporarily deployed
to
fork-preview-protection
GitHub Actions
Inactive
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21e9e35451
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| path.to_s.split('::').inject(self) do |owner, name| | ||
| owner.classes_hash[name] || | ||
| owner.modules_hash[name] || | ||
| owner.add_module(RDoc::NormalModule, name) |
There was a problem hiding this comment.
When the new RBS path creates an implicit owner for a qualified declaration such as class Foo::Bar, this add_module call adds Foo to the store but never records it on the current TopLevel. If that .rbs file is later deleted in the live server, Store#remove_file only sees the recorded child (Foo::Bar), leaving the implicit Foo module (and its stale child hash) in the rendered docs until a full restart/rebuild.
Useful? React with 👍 / 👎.
st0012
temporarily deployed
to
fork-preview-protection
GitHub Actions
Inactive
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 13804b7a0e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if method = context.find_method(method_name, singleton) | ||
| merge_documentation method, comment, type_signature_lines | ||
| return |
There was a problem hiding this comment.
When a class already documents a reader as an attribute (for example Ruby attr_reader :name or an earlier RBS attr_reader) and a first-class RBS file declares the same API as def name: ..., this lookup only checks method_list. The new AnyMethod then reaches Context#add_method, where the existing Attr in methods_hash causes it to be discarded, so the RBS comment and type signature never get attached to the documented attribute. Check/merge find_attribute(method_name, singleton) here the same way the attribute path handles existing methods.
Useful? React with 👍 / 👎.
st0012
temporarily deployed
to
fork-preview-protection
GitHub Actions
Inactive
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4481644e3b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| def parse_constant_decl(decl, context) | ||
| constant = RDoc::Constant.new decl.name.to_s, decl.type.to_s, | ||
| rdoc_comment_for(decl, context) | ||
| record_object_location constant, decl.location | ||
| context.add_constant constant |
There was a problem hiding this comment.
When an RBS source declares a qualified constant such as Foo::BAR: String, this constructs a single constant named Foo::BAR in the current context instead of placing BAR under Foo. The Ruby parser already splits constant paths before add_constant; without doing the same here, generated docs for Foo miss constants that are valid in RBS and the constant can show up under the wrong container.
Useful? React with 👍 / 👎.
| if merge_attribute_methods(context, name, rw, singleton, comment, type_signature_lines) | ||
| return |
There was a problem hiding this comment.
If Ruby already documents only one accessor method, for example def name= exists and a first-class RBS file declares attr_accessor name: String, this early return merges the signature into the writer and skips creating documentation for the reader that the RBS declaration also defines. RDoc::Context#add_attribute normally registers an attr_accessor when it still contributes a new method, so returning as soon as any matching method exists makes RBS accessor docs incomplete in mixed Ruby/RBS projects.
Useful? React with 👍 / 👎.
| namespace_names.length.downto(1) do |length| | ||
| qualified_name = namespace_names.take(length).join('::') | ||
| if module_name = @top_level.find_module_named("#{qualified_name}::#{name}") | ||
| return module_name.full_name |
There was a problem hiding this comment.
In the current code, if A::M exists before A::B::C says include M but A::B::M is declared later in the same RBS file, this lookup permanently rewrites the include to A::M because the inner module is not in the store yet. Fresh evidence beyond the earlier namespace concern is that this branch is still order-dependent: leaving the name unresolved would let RDoc::Mixin#module resolve M after all declarations are parsed and choose the innermost A::B::M.
Useful? React with 👍 / 👎.
st0012
temporarily deployed
to
fork-preview-protection
GitHub Actions
Inactive
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eda206e60a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| parse_files_matching RBS_FILE_EXTENSION | ||
|
|
||
| def scan | ||
| _, _, decls = ::RBS::Parser.parse_signature(@content) |
There was a problem hiding this comment.
When an .rbs file starts with a valid directive such as use A::M as M and later writes include M or class C < M, the second return value from RBS::Parser.parse_signature contains that alias mapping, but it is discarded here. As a result the new parser records the unresolved name M instead of A::M, so generated docs and links point at the wrong or missing mixin/superclass even when the target declaration is present.
Useful? React with 👍 / 👎.
st0012
temporarily deployed
to
fork-preview-protection
GitHub Actions
Inactive
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a97735b641
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| end | ||
|
|
||
| method.comment = comment if comment | ||
| context.add_method method |
There was a problem hiding this comment.
When a documented .rbs path sorts before the Ruby file that implements the same API (for example api.rbs before lib/foo.rb, since gather_files sorts filenames), this registers the RBS signature as the real method in methods_hash. The later Ruby parse then goes through Context#add_method's duplicate path and discards the implementation method, so generated docs keep the RBS file location/token stream and lose the Ruby params, source, and comments.
Useful? React with 👍 / 👎.
| constant = RDoc::Constant.new decl.name.to_s, decl.type.to_s, | ||
| rdoc_comment_for(decl) |
There was a problem hiding this comment.
When a first-class .rbs file is parsed before a Ruby file defining the same constant, this stores the RBS type string as Constant#value. A later Ruby add_constant only replaces an existing value when it is nil or empty, so X: String can permanently block the real X = 1 value and the docs show String as the constant value; RBS types need to be treated as mergeable metadata or yield to the Ruby assignment.
Useful? React with 👍 / 👎.
1 participant
Problem
RDoc discovers parser extensions by loading installed
rdoc/discoverfiles throughGem.find_latest_files. The current RBS discovery hook installs an externalRDoc::Parser::RBSthat delegates toRBS::RDocPlugin::Parser, and that parser still uses older RDoc constructor shapes such asRDoc::Attr.new(nil, name, rw, nil, ...)while current RDoc expectsRDoc::Attr.new(name, rw, comment, ...).Once
.rbsfiles become RDoc input, the installed RBS plugin can replace the parser path and fail before RDoc can document signatures.Solution
This moves
.rbsparsing into RDoc throughRDoc::Parser::RBS, registers.rbswith RDoc's parser list, and keepssig/**/*.rbssidecar files on the signature-loading path instead of recursive first-class input.During extension discovery, RDoc now skips the RBS gem's discovery hook when the built-in parser marker is active, so installed RBS versions do not overwrite the built-in parser. The discovery guard lives beside RDoc's existing extension loading in
lib/rdoc/rdoc.rb.Verification
I validated one fixture against both sides: an explicit
sample.rbswith class, attribute, and method comments. Against currentruby/rdocmaster (83bbaaff) plus latestruby/rbsmaster (d1bcab96), forcing RDoc parser dispatch to load the RBS discovery hook selected the RBS plugin and raisedArgumentError: wrong number of arguments (given 4, expected 3)atRDoc::Attr#initialize, matching the constructor mismatch above.Against this branch with the same RBS discovery hook advertised, RDoc kept the plugin unloaded, selected the built-in
RDoc::Parser::RBS, parsed all three RBS comments, and generated Aliki HTML containing the class, attribute, and method comments.