Skip RBS rewrite when no markers are present - #2616
Conversation
c06244a to
3ad4466
Compare
3ad4466 to
0422f6d
Compare
| def possible_rbs_runtime_rewrite_syntax?(source) | ||
| return true if source.include?("#:") || source.include?("#|") | ||
| return false unless source.include?("# @") | ||
|
|
||
| RBS_ANNOTATION_MARKERS.any? { |marker| source.include?(marker) } | ||
| end |
There was a problem hiding this comment.
This seems to be scanning the source string multiple times, can we turn the search into a Regexp.union and do a single scan through the source?
There was a problem hiding this comment.
I think something like:
SEARCH_REGEXP = Regexp.union(TYPED_FILE_PATTERN, Regexp.escape("#:"), Regexp.escape("#|"), *RBS_ANNOTATION_MARKERS.map { Regexp.escape(it) })
def should_rewrite?
source.index(SEARCH_REGEXP)
endshould give you the decision to rewrite or not with a single pass.
There was a problem hiding this comment.
Good call on the redundant scans. I just pushed an amend.
However, I kept typed_file? as a separate stage rather than folding into the union. A single union would introduce OR semantics, so a typed file with no markers would match and I think we'd want to avoid the expensive Spoom translator execution in this case.
There was a problem hiding this comment.
Ah, right, I forgot that the typed check wasn't being OR'ed. Good call.
There was a problem hiding this comment.
heh I had the same suggestion here: https://github.com/shop/world/pull/675510/changes#r3220064805
Is that other pull request just a PoC that's replaced by this one?
0422f6d to
3b07e94
Compare
3b07e94 to
d0dd91c
Compare
paracycle
left a comment
paracycle
left a comment
There was a problem hiding this comment.
Looks good to me, but have one question
| return unless typed_file?(source) | ||
| return source unless possible_rbs_runtime_rewrite_syntax?(source) | ||
|
|
||
| Spoom::Sorbet::Translate.rbs_comments_to_sorbet_sigs(source, file: path) |
There was a problem hiding this comment.
I don't see any reason why somebody might want Spoom::Sorbet::Translate.rbs_comments_to_sorbet_sigs without your early-return optimization. Would it be better to move this optimization into Spoom itself?
|
Thank you! We'll move this logic right into Spoom Shopify/spoom#916 |
4 participants
Summary
Avoid running Spoom's RBS-to-Sorbet-sig translator for typed Ruby files that cannot contain runtime-rewriteable RBS syntax.
Tapioca currently calls
Spoom::Sorbet::Translate.rbs_comments_to_sorbet_sigsfor every typed Ruby file loaded through the RBS rewriter. A large portion of typed files in Core do not contain RBS signatures or supported RBS annotations, so the translator often parses source only to return it unchanged.This keeps the existing
typed:gate and adds a conservative marker gate. Translation now runs only when source contains one of:#:#|# @abstract# @interface# @sealed# @final# @requires_ancestor:# @override# @overridable# @without_runtimeFalse positives are safe because they still use the existing translator. False negatives would be unsafe, so the annotation list is intentionally aligned with Spoom's currently supported runtime-rewrite annotations.