[DO NOT MERGE] `ko/rbs-over-prism` rebased by KaanOzkan · Pull Request #851 · Shopify/sorbet

KaanOzkan · 2026-03-09T21:26:59Z

Extracted into:

amomchilov · 2026-03-09T21:39:32Z

 #include "absl/container/flat_hash_map.h"
 #include "absl/container/flat_hash_set.h"
 #include "absl/container/inlined_vector.h"
+#include "absl/types/span.h"


Might be controversial, idk. It's pretty commonly used like everywhere, so it makes sense, but checkin with Stripe first

amomchilov · 2026-03-09T21:45:43Z

+        // Optimization: resolve well-known constant paths eagerly instead of leaving them unresolved.
+        // This is important for the RBS rewriter which generates references to these constants.
+        {
+            absl::InlinedVector<core::NameRef, 4> fullPath;


Would benefit from a comment:

Suggested change

absl::InlinedVector<core::NameRef, 4> fullPath;

// Max size of 4, to fit the longest path we're searching for (`Sorbet::Private::Static::Void`)

absl::InlinedVector<core::NameRef, 4> fullPath;

amomchilov · 2026-03-09T21:49:28Z

+            bool isRootAnchored = false;
+            pm_node_t *current = up_cast(const_cast<PrismLhsNode *>(node));
+
+            while (current != nullptr) {


In a follow up, can we tighten this up a bit?

Cap the iterations to 4. Any more than that, and we start heap-allocating elements of a path that we know for sure won't be matched below

Bail early if the name at each iteration doesn't match one of the patterns we're looking for. Since the PM_CONSTANT_PATH_NODE is backwards:

On the first iteration, we're looking for Static, Void, or WithoutRuntime

On the second iteration we're looking for Private, Sig, or Static,

etc.

- Remove unused `#include "absl/types/span.h"` from common.h - Remove unused Prism includes from pipeline.h - Fix `e.g.,` → `e.g.` style nit in Translator.cc comment - Optimize translateConst well-known path matching: use resolveConstant (string_view) instead of enterNameUTF8, cap walk at 4 iterations - Use prism.emptyNodeList() instead of {0, 0, NULL} in CommentsAssociatorPrism

amomchilov · 2026-03-12T18:02:27Z

+            pm_node_t *current = up_cast(const_cast<PrismLhsNode *>(node));
+            bool isRootAnchored = false;
+            // Segments collected innermost-first. Fixed-size array caps at 4 (longest target path).
+            std::string_view segments[4];


Why the switch away from InlineVector?

We only need a fixed size array and I think it should be faster than an inline vector. I can change it back.

Eh you end up needing to implement your own "append" behaviour instead of push_back(). InlineVector should be equally efficient as long as you don't spill over its inline size

amomchilov · 2026-03-12T18:08:01Z

        [&](parser::AndAsgn *andAsgn) {
            associateAssertionCommentsToNode(andAsgn->right.get(), true);
            walkNode(andAsgn->right.get());
+            walkNode(andAsgn->left.get());


I think we still need tests for these https://github.com/orgs/Shopify/projects/2215/views/83?pane=issue&itemId=145832685

Done in test/testdata/rbs/assertions_send_assign.rb

amomchilov · 2026-03-12T18:09:52Z

+            # Specific tests that are modified for Prism, contain minor differences in the exp file
+            "testdata/rbs/assertions_block_modified.rb",
+            "testdata/rbs/assertions_csend_assign_modified.rb",
+            "testdata/rbs/assertions_csend_modified.rb",
            "testdata/rbs/assertions_heredoc_modified.rb",
+            "testdata/rbs/signatures_types_modified.rb",


Can you re-confirm these still fail, and explain which kind of differences are causing the issues?

Done as a comment on each file. I opted to add it to the test files because I didn't want to pollute the more general test/BUILD with details.

amomchilov · 2026-03-12T20:31:50Z

                auto prismDirectDesugarAST = ast::prismDesugar::node2Tree(ctx, move(translatedTree));

-                if (!disableParserComparison) {
+                if (!disableParserComparison && !usePrismRBS) {


Why don't we want to run the comparison if RBS is on?

If the two desugar trees were equal before (without RBS rewriting for the Prism one), they should still be equal now, right?

This was always skipped. I just tried turning it on, had to trigger RBS rewriting for the legacy case. But looking at the trees they are different after an RBS rewrite:

def foo #: self as Foo T.reveal_type(self) end

Legacy: InsSeq{ loc = 3:3-3:22 } starts at first real statement
Prism: InsSeq{ loc = 2:14-3:22 } starts at synthetic T.bind from RBS comment

Should I investigate prismDesugarEqual and see if we can support this?

Ahhhh, these locations would be pretty different all over the place.

I guess this is already tested with the RBS expectation files, so there's no need to compare the desugar trees.

I think a better way to express this is to roll it into the definition on disableParserComparison, rather than a new usePrismRBS variable (whose value is actually more past tense, like didUsePrismRBS, because by the time we check it, we've already rewritten the RBS)

Can you hoist the declaration of disableParserComparison up to where bool usePrismRBS = false; is, and set it to false where usePrismRBS is set to true?

amomchilov · 2026-03-13T01:25:58Z

[DO NOT MERGE] ko/rbs-over-prism rebased #851 👈 (View in Graphite)
master

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

- Fix start > end invariant violation in desugarStatements when RBS rewriter inserts synthetic nodes with earlier locations. Falls back to the statements node's own location. - Eagerly resolve well-known root-anchored constant paths (::Sorbet::Private::Static, ::T::Sig::WithoutRuntime, etc.) that the RBS rewriter generates, avoiding unresolved constant paths. Gated on rbsEnabled so it only fires for RBS-rewritten ASTs. - Add ENFORCE check in resolveConstant() for PM_CONSTANT_ID_UNSET.

- Walk LHS of AndAsgn, OpAsgn, OrAsgn in CommentsAssociator so RBS comments on the left-hand side are not missed. - Add typeAliasAllowedInContext() helper in CommentsAssociatorPrism. - Fix type alias handling in empty class/module bodies and improve error messages when type aliases appear in disallowed contexts.

- Remove RBS_REWRITER stop-after-phase (subsumed by desugarer phase). - Update pipeline_test_runner to run Prism+RBS desugaring path: parse with Prism, run RBS rewrite on the Prism AST, then desugar. Skip legacy parser comparison when using Prism RBS path. - Unexclude RBS tests from PrismPosTests (most now pass with Prism). - Add new RBS test data: csend_assign_modified, csend_modified, block_modified, signatures_types_modified, and others. - Update cli-ref.md to remove 'rbs' from stop-after phases.

KaanOzkan changed the title ~~Ko/rbs over prism rebased~~ [DO NOT MERGE] ko/rbs-over-prism rebased Mar 9, 2026

amomchilov reviewed Mar 9, 2026

View reviewed changes

amomchilov reviewed Mar 12, 2026

View reviewed changes

KaanOzkan force-pushed the ko/rbs-over-prism-rebased branch from e120ef5 to 51be6b9 Compare March 12, 2026 18:41

amomchilov reviewed Mar 12, 2026

View reviewed changes

Comment thread main/options/options.cc

Comment thread test/pipeline_test_runner.cc

KaanOzkan force-pushed the ko/rbs-over-prism-rebased branch 3 times, most recently from 61f9415 to daa0bc5 Compare March 12, 2026 19:35

amomchilov reviewed Mar 12, 2026

View reviewed changes

amomchilov force-pushed the ko/rbs-over-prism-rebased branch from daa0bc5 to 7e6fc73 Compare March 13, 2026 01:25

KaanOzkan force-pushed the ko/rbs-over-prism-rebased branch from 7e6fc73 to c084553 Compare March 13, 2026 13:48

amomchilov force-pushed the ko/rbs-over-prism-rebased branch 3 times, most recently from c084553 to 33c4ad5 Compare March 13, 2026 14:44

KaanOzkan force-pushed the ko/rbs-over-prism-rebased branch 3 times, most recently from 5258486 to e91aa5a Compare March 13, 2026 16:58

KaanOzkan added 3 commits March 13, 2026 14:30

KaanOzkan force-pushed the ko/rbs-over-prism-rebased branch from e91aa5a to f123bbe Compare March 13, 2026 18:50

	absl::InlinedVector<core::NameRef, 4> fullPath;
	// Max size of 4, to fit the longest path we're searching for (`Sorbet::Private::Static::Void`)
	absl::InlinedVector<core::NameRef, 4> fullPath;

Conversation

KaanOzkan commented Mar 9, 2026 • 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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amomchilov commented Mar 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

KaanOzkan commented Mar 9, 2026 •

edited

Loading