RBS: Check signature parameters kind match the actual Ruby definition #8830
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
neilparikh
reviewed
May 5, 2025
rbs/MethodTypeToParserNode.cc
Outdated
| return kind; | ||
| } | ||
|
|
||
| bool checkParameterKind(core::MutableContext ctx, const RBSDeclaration &declaration, const RBSArg &arg, |
rbs/MethodTypeToParserNode.cc
Outdated
|
|
||
| bool checkParameterKind(core::MutableContext ctx, const RBSDeclaration &declaration, const RBSArg &arg, | ||
| const parser::Node *methodArg) { | ||
| auto error = false; |
There was a problem hiding this comment.
Can we rename this to something like kindMatches? When I saw error, I expected it hold some sort of error object, not a boolean.
rbs/MethodTypeToParserNode.cc
Outdated
|
|
||
| if (error) { | ||
| if (auto e = ctx.beginError(arg.loc, core::errors::Rewriter::RBSIncorrectParameterKind)) { | ||
| e.setHeader("Argument kind mismatch for `{}`, expected `{}`, got `{}`", nodeName(methodArg).show(ctx.state), |
There was a problem hiding this comment.
The error message isn't super clear to me; it's not obvious that expected is the arg in the def, and got is the one in the sig. Maybe something like:
error: Argument kind mismatch for
p, method declarespositional, but RBS sig declaresoptional positional
| #: (^() -> void) -> void | ||
| # ^^^^^^^^^^^ error: Argument kind mismatch for `blk`, expected `block`, got `positiona 8000 l` | ||
| def sig_mismatch11(&blk); end | ||
|
|
There was a problem hiding this comment.
Could you add a test case where the mismatch is in one of the args to the block?
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Morriar
commented
May 6, 2025
Comment on lines
+304
to
+306
| // TODO: fix block location in RBS parser | ||
| loc.beginLoc = loc.beginLoc + 1; | ||
| loc.endLoc = loc.endLoc + 2; |
There was a problem hiding this comment.
The location given by the RBS parser is incorrectly offset, I'll fix this upstream and remove this adjustment in a follow up.
TODO: fix the location in the RBS parser Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
673db09 to
a2fff8f
Compare
neilparikh
approved these changes
May 7, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
I found a few cases where the kind of parameter used in the RBS signature was not matching the actual kind expected.
For example, all these cases should error :
but they do not
Test plan
See included automated tests.