Add MethodTypeToParserNodePrism for RBS method signature translation #814

allcre · 2025-12-16T21:50:55Z

Motivation

Test plan

See included automated tests.

allcre · 2025-12-16T21:51:12Z

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

Co-authored-by: Kaan Ozkan <[email protected]>

amomchilov · 2025-12-18T19:50:19Z

rbs/prism/MethodTypeToParserNodePrism.cc

+}
+
+bool isRaise(pm_node_t *node, const parser::Prism::Parser *prismParser) {
+    if (!node) {


Kinda sus, why/where do callers call with null?

We do check in callsites that the body isn't null but I kept this check from the Whitequark version. It may be helpful in case there are new callers being added.

I suggest ENFORCE(!node);, and reconsider if/when we hit it.

amomchilov · 2025-12-18T19:57:28Z

rbs/prism/MethodTypeToParserNodePrism.cc

+    // In Prism, method bodies are always wrapped in PM_STATEMENTS_NODE.
+    // Unwrap if it contains exactly one statement (just the raise call).
+    // Reject if multiple statements (e.g., puts + raise).
+    if (PM_NODE_TYPE_P(node, PM_STATEMENTS_NODE)) {


Huh? This isRaise() method detects def x = raise?

Yes, it's used to ensure that the only thing an abstract method does is call raise. I simplified and moved the comment to make it clearer.

rbs/prism/MethodTypeToParserNodePrism.cc

amomchilov · 2025-12-18T20:03:59Z

rbs/prism/MethodTypeToParserNodePrism.cc

+
+void ensureAbstractMethodRaises(core::MutableContext ctx, const pm_node_t *node,
+                                const parser::Prism::Parser *prismParser) {
+    if (PM_NODE_TYPE_P(node, PM_DEF_NODE)) {


Suggested change

if (PM_NODE_TYPE_P(node, PM_DEF_NODE)) {

if (!PM_NODE_TYPE_P(node, PM_DEF_NODE)) return;

Still unresolved

amomchilov · 2025-12-18T20:25:42Z

rbs/prism/MethodTypeToParserNodePrism.cc

+    if (PM_NODE_TYPE_P(node, PM_DEF_NODE)) {
+        auto *def = down_cast<pm_def_node_t>(const_cast<pm_node_t *>(node));
+        if (def->body && isRaise(def->body, prismParser)) {
+            def->body = nullptr;


This leaks

Suggested change

def->body = nullptr;

pm_node_destroy(prismParser, def->body);

def->body = nullptr;

Still unresolved

Wouldn't the arena free it at the end so that it won't leak?

Also pm_node_destroy takes a pm_parser_t so I'd need to create a new method to access the internal pm_parser_t. Do you still prefer it?

Prism isn't using an arena (yet). Needs manual cleanup like this.

I'd need to create a new method to access the internal pm_parser_t

Make sense to me

rbs/prism/MethodTypeToParserNodePrism.cc

Co-authored-by: Alexander Momchilov <[email protected]>

rbs/prism/MethodTypeToParserNodePrism.cc

amomchilov · 2026-01-05T20:45:33Z

rbs/prism/MethodTypeToParserNodePrism.cc

+}
+
+bool isRaise(pm_node_t *node, const parser::Prism::Parser *prismParser) {
+    if (!node) {


I suggest ENFORCE(!node);, and reconsider if/when we hit it.

amomchilov · 2026-01-05T20:45:57Z

rbs/prism/MethodTypeToParserNodePrism.cc

+    return false;
+}
+
+// Check if the method body is only a single `raise` call


Suggested change

// Check if the method body is only a single `raise` call

// Detects a single `raise` with no arguments:

// - raise

// - self.raise

// - Kernel.raise

// - ::Kernel.raise

I modified this code and updated the comment slightly but we do accept raise with other arguments.

If raise with args is allowed, please add tests to cover it

There's good coverage right now so I left it as is https://github.com/sorbet/sorbet/blob/0a871d2922d7f6a8cba6e3512b51e76d149b613a/test/testdata/rbs/signatures_defs.rb#L341-L413

amomchilov · 2026-01-05T20:47:53Z

rbs/prism/MethodTypeToParserNodePrism.cc

+    if (PM_NODE_TYPE_P(node, PM_DEF_NODE)) {
+        auto *def = down_cast<pm_def_node_t>(const_cast<pm_node_t *>(node));
+        if (def->body && isRaise(def->body, prismParser)) {
+            def->body = nullptr;


Still unresolved

amomchilov · 2026-01-05T20:48:03Z

rbs/prism/MethodTypeToParserNodePrism.cc

+
+void ensureAbstractMethodRaises(core::MutableContext ctx, const pm_node_t *node,
+                                const parser::Prism::Parser *prismParser) {
+    if (PM_NODE_TYPE_P(node, PM_DEF_NODE)) {


Still unresolved

amomchilov · 2026-01-05T20:48:38Z

rbs/prism/MethodTypeToParserNodePrism.cc

+                                       {core::AutocorrectSuggestion::Edit{ctx.locAt(editLoc), corrected}}};
+}
+
+void ensureAbstractMethodRaises(core::MutableContext ctx, pm_node_t *node, const parser::Prism::Parser *prismParser) {


Suggested change

void ensureAbstractMethodRaises(core::MutableContext ctx, pm_node_t *node, const parser::Prism::Parser *prismParser) {

// Abstract methods must always have a single `raise` statement:

//

// def my_abstract_method = raise

void ensureAbstractMethodRaises(core::MutableContext ctx, pm_node_t *node, const parser::Prism::Parser *prismParser) {

I moved this comment to isValidAbstractMethod instead. I think the function name ensureAbstractMethodRaises is self documenting.

amomchilov · 2026-01-05T20:52:37Z

rbs/prism/MethodTypeToParserNodePrism.cc

+            sigBuilder = prism.Call0(annotation.typeLoc, sigBuilder, core::Names::overridable().show(ctx.state));
+        } else if (annotation.string == "override") {
+            sigBuilder = prism.Call0(annotation.typeLoc, sigBuilder, core::Names::override_().show(ctx.state));
+        } else if (annotation.string == "override(allow_incompatible: true)" ||


What about override(allow_incompatible: false)

Maybe just forward the args "as is"?

We can extract the relevant part out of the RBS, and call Prism.parse on it directly, which we can just attach that into the generated sig { override(...)... } call. (low priority)

I did a quick prototype. We can parse the override args with a new Prism parser instance (AFAIK we can't reuse the existing Prism parser). Then we use the Prism factory to add specific kinds of args to the existing parser. So we still need a case statement that supports specific Prism node types to have good coverage. Not great.

pm_node_t *copyLiteralNode(pm_node_t *node, parser::Prism::Parser &sourceParser, const parser::Prism::Factory &factory, core::LocOffsets loc) { switch (PM_NODE_TYPE(node)) { case PM_TRUE_NODE: return factory.True(loc); case PM_FALSE_NODE: return factory.False(loc); case PM_SYMBOL_NODE: { auto *sym = down_cast<pm_symbol_node_t>(node); return factory.Symbol(loc, string(sourceParser.extractString(&sym->unescaped))); } case PM_INTEGER_NODE: { auto *num = down_cast<pm_integer_node_t>(node); return factory.Integer(loc, num->value.value); } case PM_STRING_NODE: { auto *str = down_cast<pm_string_node_t>(node); return factory.String(loc, string(sourceParser.extractString(&str->unescaped))); } } }

Or we could go the string manipulation route to extract the arg and call factory methods to insert to the Prism tree, similar to what we do now.

if (annotation.string.compare(0, 28, "override(allow_incompatible:") == 0) { auto start = annotation.string.find(": ") + 2; auto valueStr = annotation.string.substr(start, annotation.string.find_last_of(')') - start); // Trim whitespace while (!valueStr.empty() && (valueStr[0] == ' ' || valueStr[0] == '\t')) valueStr.remove_prefix(1); while (!valueStr.empty() && (valueStr.back() == ' ' || valueStr.back() == '\t')) valueStr.remove_suffix(1); pm_node_t *value = nullptr; if (valueStr == "true") { value = prism.True(annotation.typeLoc); } else if (valueStr == "false") { value = prism.False(annotation.typeLoc); } else if (!valueStr.empty() && valueStr[0] == ':') { value = prism.Symbol(annotation.typeLoc, string(valueStr.substr(1))); } // Add more types: integers, strings with parsing, etc. if (value) { ... } }

I kept it as is right now and added support for false, let me know how you want to proceed.

I like this approach for now. I'll play around with using the parser

amomchilov · 2026-01-05T20:55:18Z

rbs/prism/MethodTypeToParserNodePrism.cc

+        return;
+    }
+
+    for (rbs_hash_node_t *hash_node = field->head; hash_node != nullptr; hash_node = hash_node->next) {


Suggested change

for (rbs_hash_node_t *hash_node = field->head; hash_node != nullptr; hash_node = hash_node->next) {

for (auto *hash_node = field->head; hash_node != nullptr; hash_node = hash_node->next) {

rbs/prism/MethodTypeToParserNodePrism.cc

amomchilov · 2026-01-05T21:07:34Z

rbs/prism/MethodTypeToParserNodePrism.cc

+    sigParams.reserve(args.size());
+
+    // Check if methodParams is empty once before the loop (loop invariant)
+    if (methodParams.empty() && !args.empty()) {


This is the wrong kind of check for the kind of message that's emitted

Suggested change

if (methodParams.empty() && !args.empty()) {

if (methodParams.size() > args.size()) {

rbs/prism/MethodTypeToParserNodePrism.cc

amomchilov · 2026-01-05T21:23:59Z

rbs/prism/MethodTypeToParserNodePrism.cc

+            sigBuilder = prism.Call0(annotation.typeLoc, sigBuilder, core::Names::overridable().show(ctx.state));
+        } else if (annotation.string == "override") {
+            sigBuilder = prism.Call0(annotation.typeLoc, sigBuilder, core::Names::override_().show(ctx.state));
+        } else if (annotation.string == "override(allow_incompatible: true)" ||


Maybe just forward the args "as is"?

We can extract the relevant part out of the RBS, and call Prism.parse on it directly, which we can just attach that into the generated sig { override(...)... } call. (low priority)

amomchilov

One last suggestion, but looks good!

amomchilov · 2026-01-06T16:39:21Z

rbs/prism/MethodTypeToParserNodePrism.cc

+bool isValidAbstractMethod(pm_node_t *node, const parser::Prism::Parser *prismParser) {
+    if (!PM_NODE_TYPE_P(node, PM_DEF_NODE)) {
+        return false;
+    }
+
+    auto *def = down_cast<pm_def_node_t>(node);
+
+    // Check if body is a single raise call
+    if (def->body) {
+        pm_node_t *bodyNode = def->body;
+
+        // Unwrap statements node if it contains exactly one statement
+        if (PM_NODE_TYPE_P(bodyNode, PM_STATEMENTS_NODE)) {
+            auto *stmts = down_cast<pm_statements_node_t>(bodyNode);
+            if (stmts->body.size == 1) {
+                bodyNode = stmts->body.nodes[0];
+            } else {
+                return false; // Multiple statements, not just a raise
+            }
+        }
+
+        // Check if it's a raise call
+        if (bodyNode && PM_NODE_TYPE_P(bodyNode, PM_CALL_NODE)) {
+            auto *call = down_cast<pm_call_node_t>(bodyNode);
+            auto methodName = prismParser->resolveConstant(call->name);
+
+            if (methodName == "raise" && (call->receiver == nullptr || isSelfOrKernel(call->receiver, prismParser))) {
+                return true;
+            }
+        }
+    }
+
+    return false;
+}


Re-use the same early return style you used in isSelfOrKernel()

Suggested change

bool isValidAbstractMethod(pm_node_t *node, const parser::Prism::Parser *prismParser) {

if (!PM_NODE_TYPE_P(node, PM_DEF_NODE)) {

return false;

}

auto *def = down_cast<pm_def_node_t>(node);

// Check if body is a single raise call

if (def->body) {

pm_node_t *bodyNode = def->body;

// Unwrap statements node if it contains exactly one statement

if (PM_NODE_TYPE_P(bodyNode, PM_STATEMENTS_NODE)) {

auto *stmts = down_cast<pm_statements_node_t>(bodyNode);

if (stmts->body.size == 1) {

bodyNode = stmts->body.nodes[0];

} else {

return false; // Multiple statements, not just a raise

}

}

// Check if it's a raise call

if (bodyNode && PM_NODE_TYPE_P(bodyNode, PM_CALL_NODE)) {

auto *call = down_cast<pm_call_node_t>(bodyNode);

auto methodName = prismParser->resolveConstant(call->name);

if (methodName == "raise" && (call->receiver == nullptr || isSelfOrKernel(call->receiver, prismParser))) {

return true;

}

}

}

return false;

}

bool isValidAbstractMethod(pm_node_t *node, const parser::Prism::Parser *prismParser) {

if (!PM_NODE_TYPE_P(node, PM_DEF_NODE)) {

return false;

}

auto *def = down_cast<pm_def_node_t>(node);

// Check if body is a single raise call

if (def->body == nullptr) {

return false;

}

pm_node_t *bodyNode = def->body;

ENFORCE(bodyNode != nullptr);

// Unwrap statements node if it contains exactly one statement

if (PM_NODE_TYPE_P(bodyNode, PM_STATEMENTS_NODE)) {

auto *stmts = down_cast<pm_statements_node_t>(bodyNode);

if (stmts->body.size > 1) { // Multiple statements, not just a raise

return false;

}

bodyNode = stmts->body.nodes[0];

}

if (!PM_NODE_TYPE_P(bodyNode, PM_CALL_NODE)) {

return false;

}

auto *call = down_cast<pm_call_node_t>(bodyNode);

auto methodName = prismParser->resolveConstant(call->name);

return methodName == "raise" && (call->receiver == nullptr || isSelfOrKernel(call->receiver, prismParser))

}

amomchilov · 2026-01-06T16:39:52Z

rbs/prism/MethodTypeToParserNodePrism.cc

+        }
+
+        // Check if it's a raise call
+        if (bodyNode && PM_NODE_TYPE_P(bodyNode, PM_CALL_NODE)) {


I think this bodyNode check can't fail. Statements' body nodes can't contain nulls.

amomchilov · 2026-01-06T16:40:29Z

rbs/prism/MethodTypeToParserNodePrism.cc

+        pm_node_t *bodyNode = def->body;
+
+        // Unwrap statements node if it contains exactly one statement
+        if (PM_NODE_TYPE_P(bodyNode, PM_STATEMENTS_NODE)) {


You're relying on bodyNode not being null here, I'd document that with a ENFORCE(bodyNode != nullptr); (which I added in my suggestion in the prev comment)

Updated the function. We now check for null and then assign so ENFORCE shouldn't be necessary.

This was referenced Dec 16, 2025

Add RBS helper utilities and extract RBSArg to shared header #812

Closed

Add TypeToParserNodePrism for RBS type translation #813

Closed

Add SignatureTranslatorPrism and TypeParamsToParserNodesPrism #815

Closed

allcre force-pushed the ac-add_typetoparsernodeprism_for_rbs_type_translation branch from 2c0e20a to 5a67fec Compare December 16, 2025 22:09

allcre force-pushed the ac-add_methodtypetoparsernodeprism_for_rbs_method_signature_translation branch from 8914ab9 to 77219b7 Compare December 16, 2025 22:09

allcre marked this pull request as ready for review December 16, 2025 22:18

allcre force-pushed the ac-add_methodtypetoparsernodeprism_for_rbs_method_signature_translation branch from 77219b7 to 005a338 Compare December 16, 2025 22:22

allcre mentioned this pull request Dec 18, 2025

Add RBS helper utilities and extract RBSArg to shared header sorbet/sorbet#9770

Merged

allcre force-pushed the ac-add_typetoparsernodeprism_for_rbs_type_translation branch from 5a67fec to 613acea Compare December 19, 2025 19:35

allcre force-pushed the ac-add_methodtypetoparsernodeprism_for_rbs_method_signature_translation branch from 005a338 to 4536485 Compare December 19, 2025 19:35

Add MethodTypeToParserNodePrism for RBS method signature translation

388f9ab

Co-authored-by: Kaan Ozkan <[email protected]>

allcre force-pushed the ac-add_methodtypetoparsernodeprism_for_rbs_method_signature_translation branch from 4536485 to 388f9ab Compare December 23, 2025 22:04

KaanOzkan requested a review from amomchilov January 2, 2026 15:35

Remove const parameters, add designated initializers and small fixes

9aec767

amomchilov reviewed Jan 2, 2026

View reviewed changes

Apply suggestions from code review

e759508

Co-authored-by: Alexander Momchilov <[email protected]>

amomchilov reviewed Jan 2, 2026

View reviewed changes

rbs/prism/MethodTypeToParserNodePrism.cc Outdated Show resolved Hide resolved

amomchilov reviewed Jan 2, 2026

View reviewed changes

rbs/prism/MethodTypeToParserNodePrism.cc Outdated Show resolved Hide resolved

More code refactors

2db0cdc

KaanOzkan force-pushed the ac-add_methodtypetoparsernodeprism_for_rbs_method_signature_translation branch from 9f090e4 to 2db0cdc Compare January 5, 2026 20:18

Remove unnecessary comments and format

38fe73e

amomchilov reviewed Jan 5, 2026

View reviewed changes

Address review comments

129e5c9

amomchilov approved these changes Jan 6, 2026

View reviewed changes

More refactors

97dfce9

KaanOzkan force-pushed the ac-add_methodtypetoparsernodeprism_for_rbs_method_signature_translation branch from a8558f5 to 97dfce9 Compare January 6, 2026 20:49

KaanOzkan closed this Jan 7, 2026

	if (PM_NODE_TYPE_P(node, PM_DEF_NODE)) {
	if (!PM_NODE_TYPE_P(node, PM_DEF_NODE)) return;

	def->body = nullptr;
	pm_node_destroy(prismParser, def->body);
	def->body = nullptr;

-// Check if the method body is only a single `raise` call
+// Detects a single `raise` with no arguments:
+//   - raise
+//   - self.raise
+//   - Kernel.raise
+//   - ::Kernel.raise

	for (rbs_hash_node_t *hash_node = field->head; hash_node != nullptr; hash_node = hash_node->next) {
	for (auto *hash_node = field->head; hash_node != nullptr; hash_node = hash_node->next) {

	if (methodParams.empty() && !args.empty()) {
	if (methodParams.size() > args.size()) {

Add MethodTypeToParserNodePrism for RBS method signature translation #814

Add MethodTypeToParserNodePrism for RBS method signature translation #814

Conversation

allcre commented Dec 16, 2025

Motivation

Test plan

Uh oh!

allcre commented Dec 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

amomchilov Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amomchilov Jan 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

allcre commented Dec 16, 2025 •

edited

Loading

amomchilov Dec 18, 2025 •

edited

Loading

amomchilov Jan 6, 2026 •

edited

Loading