8000 RBS: Check signature parameters kind match the actual Ruby definition by Morriar · Pull Request #8830 · sorbet/sorbet · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

RBS: Check signature parameters kind match the actual Ruby definition #8830

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

Merged
merged 9 commits into from
May 7, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions core/errors/rewriter.h
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ constexpr ErrorClass RBSParameterMismatch{3552, StrictLevel::False};
constexpr ErrorClass RBSAssertionError{3553, StrictLevel::False};
constexpr ErrorClass RBSUnusedComment{3554, StrictLevel::False};
constexpr ErrorClass RBSMultilineMisformatted{3555, StrictLevel::False};
constexpr ErrorClass RBSIncorrectParameterKind{3556, StrictLevel::False};

} // namespace sorbet::core::errors::Rewriter
#endif
124 changes: 102 additions & 22 deletions rbs/MethodTypeToParserNode.cc
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,18 @@ struct RBSArg {
core::LocOffsets loc;
rbs_ast_symbol_t *name;
rbs_node_t *type;

enum class Kind {
Positional,
OptionalPositional,
RestPositional,
Keyword,
OptionalKeyword,
RestKeyword,
Block,
};

Kind kind;
};

unique_ptr<parser::Node> handleAnnotations(unique_ptr<parser::Node> sigBuilder, const vector<Comment> &annotations) {
Expand Down Expand Up @@ -69,6 +81,68 @@ core::NameRef nodeName(const parser::Node *node) {
return name;
}

string argKindToString(RBSArg::Kind kind) {
switch (kind) {
case RBSArg::Kind::Positional:
return "positional";
case RBSArg::Kind::OptionalPositional:
return "optional positional";
case RBSArg::Kind::RestPositional:
return "rest positional";
case RBSArg::Kind::Keyword:
return "keyword";
case RBSArg::Kind::OptionalKeyword:
return "optional keyword";
case RBSArg::Kind::RestKeyword:
return "rest keyword";
case RBSArg::Kind::Block:
return "block";
}
}

string nodeKindToString(const parser::Node *node) {
string kind;

typecase(
node, [&](const parser::Arg *parserArg) { kind = "positional"; },
[&](const parser::Optarg *parserArg) { kind = "optional positional"; },
[&](const parser::Restarg *parserArg) { kind = "rest positional"; },
[&](const parser::Kwarg *parserArg) { kind = "keyword"; },
[&](const parser::Kwoptarg *parserArg) { kind = "optional keyword"; },
[&](const parser::Kwrestarg *parserArg) { kind = "rest keyword"; },
[&](const parser::Blockarg *parserArg) { kind = "block"; },
[&](const parser::Node *other) {
Exception::raise("Unexpected expression type: {}", ((parser::Node *)node)->nodeName());
});

return kind;
}

bool checkParameterKindMatch(core::MutableContext ctx, const RBSArg &arg, const parser::Node *methodArg) {
auto kindMatch = false;

typecase(
methodArg, [&](const parser::Arg *parserArg) { kindMatch = arg.kind == RBSArg::Kind::Positional; },
[&](const parser::Optarg *parserArg) { kindMatch = arg.kind == RBSArg::Kind::OptionalPositional; },
[&](const parser::Restarg *parserArg) { kindMatch = arg.kind == RBSArg::Kind::RestPositional; },
[&](const parser::Kwarg *parserArg) { kindMatch = arg.kind == RBSArg::Kind::Keyword; },
[&](const parser::Kwoptarg *parserArg) { kindMatch = arg.kind == RBSArg::Kind::OptionalKeyword; },
[&](const parser::Kwrestarg *parserArg) { kindMatch = arg.kind == RBSArg::Kind::RestKeyword; },
[&](const parser::Blockarg *parserArg) { kindMatch = arg.kind == RBSArg::Kind::Block; },
[&](const parser::Node *other) {
Exception::raise("Unexpected expression type: {}", ((parser::Node *)methodArg)->nodeName());
});

if (!kindMatch) {
if (auto e = ctx.beginError(arg.loc, core::errors::Rewriter::RBSIncorrectParameterKind)) {
e.setHeader("Argument kind mismatch for `{}`, method declares `{}`, but RBS signature declares `{}`",
nodeName(methodArg).show(ctx.state), nodeKindToString(methodArg), argKindToString(arg.kind));
}
}

return kindMatch;
}

parser::Args *getMethodArgs(const parser::Node *node) {
parser::Node *args;

Expand All @@ -82,7 +156,7 @@ parser::Args *getMethodArgs(const parser::Node *node) {
return parser::cast_node<parser::Args>(args);
}

void collectArgs(const RBSDeclaration &declaration, rbs_node_list_t *field, vector<RBSArg> &args) {
void collectArgs(const RBSDeclaration &declaration, rbs_node_list_t *field, vector<RBSArg> &args, RBSArg::Kind kind) {
if (field == nullptr || field->length == 0) {
return;
}
Expand All @@ -103,12 +177,12 @@ void collectArgs(const RBSDeclaration &declaration, rbs_node_list_t *field, vect

auto loc = declaration.typeLocFromRange(range);
auto node = (rbs_types_function_param_t *)list_node->node;
auto arg = RBSArg{loc, node->name, node->type};
auto arg = RBSArg{loc, node->name, node->type, kind};
args.emplace_back(arg);
}
}

void collectKeywords(const RBSDeclaration &declaration, rbs_hash_t *field, vector<RBSArg> &args) {
void collectKeywords(const RBSDeclaration &declaration, rbs_hash_t *field, vector<RBSArg> &args, RBSArg::Kind kind) {
if (field == nullptr) {
return;
}
Expand All @@ -125,7 +199,7 @@ void collectKeywords(const RBSDeclaration &declaration, rbs_hash_t *field, vecto
auto loc = declaration.typeLocFromRange(hash_node->key->location->rg);
rbs_ast_symbol_t *keyNode = (rbs_ast_symbol_t *)hash_node->key;
rbs_types_function_param_t *valueNode = (rbs_types_function_param_t *)hash_node->value;
auto arg = RBSArg{loc, keyNode, valueNode->type};
auto arg = RBSArg{loc, keyNode, valueNode->type, kind};
args.emplace_back(arg);
}
}
Expand Down Expand Up @@ -188,8 +262,8 @@ unique_ptr<parser::Node> MethodTypeToParserNode::methodSignature(const parser::N

vector<RBSArg> args;

collectArgs(declaration, functionType->required_positionals, args);
collectArgs(declaration, functionType->optional_positionals, args);
collectArgs(declaration, functionType->required_positionals, args, RBSArg::Kind::Positional);
collectArgs(declaration, functionType->optional_positionals, args, RBSArg::Kind::OptionalPositional);

rbs_node_t *restPositionals = functionType->rest_positionals;
if (restPositionals) {
Expand All @@ -199,16 +273,16 @@ unique_ptr<parser::Node> MethodTypeToParserNode::methodSignature(const parser::N

auto loc = declaration.typeLocFromRange(restPositionals->location->rg);
auto node = (rbs_types_function_param_t *)restPositionals;
auto arg = RBSArg{loc, node->name, node->type};
auto arg = RBSArg{loc, node->name, node->type, RBSArg::Kind::RestPositional};
args.emplace_back(arg);
}

collectArgs(declaration, functionType->trailing_positionals, args);
collectArgs(declaration, functionType->trailing_positionals, args, RBSArg::Kind::Positional);

// Collect keywords

collectKeywords(declaration, functionType->required_keywords, args);
collectKeywords(declaration, functionType->optional_keywords, args);
collectKeywords(declaration, functionType->required_keywords, args, RBSArg::Kind::Keyword);
collectKeywords(declaration, functionType->optional_keywords, args, RBSArg::Kind::OptionalKeyword);

rbs_node_t *restKeywords = functionType->rest_keywords;
if (restKeywords) {
Expand All @@ -218,16 +292,19 @@ unique_ptr<parser::Node> MethodTypeToParserNode::methodSignature(const parser::N

auto loc = declaration.typeLocFromRange(restKeywords->location->rg);
auto node = (rbs_types_function_param_t *)restKeywords;
auto arg = RBSArg{loc, node->name, node->type};
auto arg = RBSArg{loc, node->name, node->type, RBSArg::Kind::RestKeyword};
args.emplace_back(arg);
}

// Collect block

auto *block = node.block;
if (block) {
// TODO: RBS doesn't have location on blocks yet
auto arg = RBSArg{fullTypeLoc, nullptr, (rbs_node_t *)block};
auto loc = declaration.typeLocFromRange(block->base.location->rg);
// TODO: fix block location in RBS parser
loc.beginLoc = loc.beginLoc + 1;
loc.endLoc = loc.endLoc + 2;
Comment on lines +304 to +306
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The location given by the RBS parser is incorrectly offset, I'll fix this upstream and remove this adjustment in a follow up.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

auto arg = RBSArg{loc, nullptr, (rbs_node_t *)block, RBSArg::Kind::Block};
args.emplace_back(arg);
}

Expand All @@ -241,26 +318,29 @@ unique_ptr<parser::Node> MethodTypeToParserNode::methodSignature(const parser::N
auto &arg = args[i];
auto type = typeToParserNode.toParserNode(arg.type, declaration);

if (!methodArgs || i >= methodArgs->args.size()) {
if (auto e = ctx.beginError(fullTypeLoc, core::errors::Rewriter::RBSParameterMismatch)) {
e.setHeader("RBS signature has more parameters than in the method definition");
}

return nullptr;
}

auto methodArg = methodArgs->args[i].get();

if (auto nameSymbol = arg.name) {
// The RBS arg is named in the signature, so we use the explicit name used
auto nameStr = parser.resolveConstant(nameSymbol);
auto name = ctx.state.enterNameUTF8(nameStr);
sigParams.emplace_back(make_unique<parser::Pair>(arg.loc, parser::MK::Symbol(arg.loc, name), move(type)));
} else {
if (!methodArgs || i >= methodArgs->args.size()) {
if (auto e = ctx.beginError(fullTypeLoc, core::errors::Rewriter::RBSParameterMismatch)) {
e.setHeader("RBS signature has more parameters than in the method definition");
}

return nullptr;
}

// The RBS arg is not named in the signature, so we get it from the method definition
auto methodArg = methodArgs->args[i].get();
auto name = nodeName(methodArg);
sigParams.emplace_back(
make_unique<parser::Pair>(arg.loc, parser::MK::Symbol(methodArg->loc, name), move(type)));
}

checkParameterKindMatch(ctx, arg, methodArg);
}

// Build the sig
Expand Down
2 changes: 1 addition & 1 deletion test/testdata/lsp/hover_rbs.rb
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ def hover_named_params_1(x, y, z); T.unsafe(nil); end
# ^ hover: Symbol
# ^ hover: T.class_of(Symbol)
# ^ hover: T.class_of(Symbol)
def hover_keyword_params_1(x, y, z); T.unsafe(nil); end
def hover_keyword_params_1(x:, y:, z:); T.unsafe(nil); end

# Block params hover

Expand Down
2 changes: 1 addition & 1 deletion test/testdata/rbs/assertions_block.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
# enable-experimental-rbs-signatures: true
# enable-experimental-rbs-assertions: true

#: (untyped) {(String?) -> String} -> untyped
#: (*untyped) {(String?) -> String} -> untyped
def take_block(*args, &); end

take_block { |x|
Expand Down
2 changes: 1 addition & 1 deletion test/testdata/rbs/assertions_csend_assign.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
# let

class Let
#: (untyped) -> void
#: (*untyped) -> void
def foo=(*args); end

#: -> untyped
Expand Down
2 changes: 1 addition & 1 deletion test/testdata/rbs/assertions_send_assign.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
# let

class Let
#: (untyped) -> void
#: (*untyped) -> void
def foo=(*args); end

#: -> untyped
Expand Down
2 changes: 1 addition & 1 deletion test/testdata/rbs/signatures_attrs.rb
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ class Base
#: Integer?
attr_reader :foo

#: (Integer?) -> void
#: (?Integer?) -> void
def initialize(foo = nil)
@foo = foo
end
Expand Down
48 changes: 43 additions & 5 deletions test/testdata/rbs/signatures_defs.rb
Original file line number Diff line number Diff line change
Expand Up @@ -40,13 +40,51 @@ def parse_error4(p1, p2); end # error: The method `parse_error4` does not have a
def sig_mismatch1(p1, p2); end
# ^^ error: Malformed `sig`. Type not specified for argument `p2`

#: (foo: P1) -> void # error: Unknown argument name `foo`
#: (foo: P1) -> void
# ^^^ error: Argument kind mismatch for `p1`, method declares `positional`, but RBS signature declares `keyword`
# ^^^ error: Unknown argument name `foo`
def sig_mismatch2(p1); end
# ^^ error: Malformed `sig`. Type not specified for argument `p1`

#: (P1, P2, P3) -> void # error: RBS signature has more parameters than in the method definition
def sig_mismatch3; end # error: The method `sig_mismatch3` does not have a `sig`

#: (?Integer) -> void
# ^^^^^^^ error: Argument kind mismatch for `p`, method declares `positional`, but RBS signature declares `optional positional`
def sig_mismatch4(p); end

#: (Integer) -> void
# ^^^^^^^ error: Argument kind mismatch for `p`, method declares `optional positional`, but RBS signature declares `positional`
def sig_mismatch5(p = 42); end

#: (Integer) -> void
# ^^^^^^^ error: Argument kind mismatch for `p`, method declares `keyword`, but RBS signature declares `positional`
def sig_mismatch6(p:); end

#: (Integer) -> void
# ^^^^^^^ error: Argument kind mismatch for `p`, method declares `rest positional`, but RBS signature declares `positional`
def sig_mismatch7(*p); end

#: (?p: Integer) -> void
# ^ error: Argument kind mismatch for `p`, method declares `keyword`, but RBS signature declares `optional keyword`
def sig_mismatch8(p:); end

#: (p: Integer) -> void
# ^ error: Argument kind mismatch for `p`, method declares `optional keyword`, but RBS signature declares `keyword`
def sig_mismatch9(p: 42); end

#: (*Integer) -> void
# ^^^^^^^ error: Argument kind mismatch for `p`, method declares `rest keyword`, but RBS signature declares `rest positional`
def sig_mismatch10(**p); end

#: (^() -> void) -> void
# ^^^^^^^^^^^ error: Argument kind mismatch for `blk`, method declares `block`, but RBS signature declares `positional`
def sig_mismatch11(&blk); end

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add a test case where the mismatch is in one of the args to the block?

#: { -> void } -> void
# ^^^^^^^^^^^ error: Argument kind mismatch for `p`, method declares `positional`, but RBS signature declares `block`
def sig_mismatch12(p); end

# Sigs

# We do not create any sig if there is no RBS comment
Expand Down Expand Up @@ -136,16 +174,16 @@ def method13(x)
end

#: (?String x) -> void
def method14(x)
def method14(x = "")
T.reveal_type(x) # error: Revealed type: `String`
end

#: (String x) -> void
#: (?String x) -> void
def method15(x = nil) # error: Argument does not have asserted type `String`
T.reveal_type(x) # error: Revealed type: `T.nilable(String)`
end

#: (String ?x) -> void
#: (?String? x) -> void
def method16(x = nil)
T.reveal_type(x) # error: Revealed type: `T.nilable(String)`
end
Expand Down Expand Up @@ -224,7 +262,7 @@ def bar; end # error: The method `bar` does not have a `sig`
end

class FooProc
#: (p: ^() -> Integer ) ?{ (Integer) [self: FooProc] -> String } -> void
#: (?p: ^() -> Integer ) ?{ (Integer) [self: FooProc] -> String } -> void
def initialize(p: -> { 42 }, &block)
T.reveal_type(p) # error: Revealed type: `T.proc.returns(Integer)`
T.reveal_type(block) # error: Revealed type: `T.nilable(T.proc.params(arg0: Integer).returns(String))`
Expand Down
Loading
0