Skip RBS rewriter when file does not contain RBS syntax

[dejmedus]

• closes Skip RBS rewriter when a file doesn't contain any RBS syntax #916
• moved from tapioca Skip RBS rewrite when no markers are present tapioca#2616

If a file does not contain typed file markers (ex # typed: true) or RBS syntax we can skip attempting to translate it. If a file has not been rewritten we can exclude it from the count of translated files

[amomchilov]

Two more cases to test:

1. Don't trigger if # typed: true exists later in the file. This is unlikely, but it's a performance improvement to ensure we're not scanning through the whole file

2. Trigger if there's multiple magic comments, but typed isn't first:

   # frozen_string_literal: true
   # typed: true

[dejmedus]

> Don't trigger if # typed: true exists later in the file

Good call! I updated to only check for the typed marker in the magic comments block

Refactored to instead use strictness_in_content and valid_strictness checks from /sorbet/sigils.rb

[dejmedus]

This will only count changed files in the Translated signatures in <x> files. message, but I'm happy to drop the commit if we want to leave as is

[amomchilov]

I like it.

[paracycle]

Shouldn't all this logic be encapsulated inside the RBSCommentsToSorbetSigs class, which already stores the ruby_contents as an ivar, and wouldn't need to pass it to a should_rewrite? method. The downside is just an extra object allocation only to return the same contents, but I think that's fine.

[amomchilov]

Few small points, but it's shaping up!

[amomchilov]

Let's mark these with private_constant where they're defined, so we don't expose them to the public.

[amomchilov]

LGTM pending the unresolved comments

[amomchilov]

Suggested change

	def test_contains_rbs_syntax_returns_true_when_typed_sigils_follow_magic_comments
	def test_contains_rbs_syntax_returns_true_when_typed_sigil_is_after_other_magic_comments

[amomchilov]

Good call

[Morriar]

Suggested change

	def rewrite(ruby_contents, file:, max_line_length: nil)
	def rewrite_if_needed(ruby_contents, file:, max_line_length: nil)

or something like this

[Morriar]

Can we instead call the underlying new.rewrite instead so we don't have to rewrite all the tests?

[dejmedus]

Can do:) Updated (I just left the few in the cli sigs_test.rb file)

[dejmedus]

Sorry scratch that, the newest commit moves the RBS check logic into new.rewrite so the tests would again need a sigil (unless we wanted to drop the sigil check and just look for RBS?) I could probably make a test helper to insert sigils to contents but I wonder if that would be more confusing. What do you think?

[paracycle]

I am not opinionated about this but I would have prefered:

Suggested change

	class << self
	#: (String source) -> bool
	def contains_rbs_syntax?(source)
	Sigils.contains_valid_sigil?(source) && source.match?(RBS_REWRITE_PATTERN)
	end
	#: (String ruby_contents, file: String, ?max_line_length: Integer?) -> String
	def rewrite_if_needed(ruby_contents, file:, max_line_length: nil)
	return ruby_contents unless contains_rbs_syntax?(ruby_contents)
	new(ruby_contents, file:, max_line_length:).rewrite
	end
	end
	#: (String source) -> bool
	def contains_rbs_syntax?
	Sigils.contains_valid_sigil?(@ruby_contents) && @ruby_contents.match?(RBS_REWRITE_PATTERN)
	end
	# @override
	#: () -> String
	def rewrite
	return @ruby_contents unless contains_rbs_syntax?
	super
	end

[paracycle]

In summary, I think we are unnecessarily pulling logic into class methods and passing values around when the value we are interested in is being passed into the constructor of our class, and no-one cares what exactly rewrite does internally (i.e. in this case it decides to return the original buffer intact).

[paracycle]

I notice that we eagerly do parsing in the initialize of Translator, but we could also short-circuit the call to super in this class's initialize method by doing the sigil and rewrite checks in the initializer.

[dejmedus]

I think I was thinking that I needed to exit early before we initialized to prevent hitting parse_ruby and that rewrite needed that path to have run already, but I think this is clicking for me now, thanks very much for the examples!

[paracycle]

@dejmedus No problem. I am glad to hear it is helpful.

As for initialize, there is nothing magic about that method, and returning early doesn't change behaviour in any way different than in other methods. Even an empty initialize method still initializes the object, so you can add any logic in initialize that you want.

[amomchilov]

Random thought I had: does factoring out the common prefixes produce a more faster regular expression? E.g. overrid(?:e|able) instead of checking for the two whole words separately.

In short, yes, but barely. Onigmo doesn't optimize out common prefixes in the NFA it creates, but it doesn't make a real world difference here.

Got Claude to benchmark it for me:

require "benchmark"

# loading corpus... 101996 files, 616MB, loaded in 15.09s
# sanity-checking match equivalence... 19104 total matches across corpus
#
#                     # user     system      total        real
# flat              0.536375   0.006953   0.543328 (  0.552176)
# factored          0.530740   0.005332   0.536072 (  0.544158)
#
# factored is 1.01x faster than flat (real time)

MARKERS = [
  "# @abstract",
  "# @interface",
  "# @sealed",
  "# @final",
  "# @requires_ancestor:",
  "# @override",
  "# @overridable",
  "# @without_runtime",
].freeze


FLAT     = Regexp.union(*MARKERS)
FACTORED = /\# @(?:abstract|interface|sealed|final|requires_ancestor:|overrid(?:e|able)|without_runtime)/

raise "regexes diverge on hits"   unless MARKERS.all? { |m| FLAT =~ m && FACTORED =~ m }
raise "regexes diverge on misses" if FLAT =~ "# @other" || FACTORED =~ "# @other"

CORPUS_GLOB = "/your/test/corpus/**/*.rb"

print "loading corpus... "
load_start = Process.clock_gettime(Process::CLOCK_MONOTONIC)
CORPUS = Dir.glob(CORPUS_GLOB).map { |path| File.read(path) rescue nil }.compact.freeze
load_elapsed = Process.clock_gettime(Process::CLOCK_MONOTONIC) - load_start
total_bytes = CORPUS.sum(&:bytesize)
puts "#{CORPUS.size} files, #{total_bytes / 1_000_000}MB, loaded in #{load_elapsed.round(2)}s"

print "sanity-checking match equivalence... "
flat_hits     = CORPUS.sum { |s| s.scan(FLAT).size }
factored_hits = CORPUS.sum { |s| s.scan(FACTORED).size }
raise "regexes diverge: flat=#{flat_hits} factored=#{factored_hits}" unless flat_hits == factored_hits
puts "#{flat_hits} total matches across corpus"

flat_time     = Benchmark.measure("flat")     { CORPUS.each { |s| s.scan(FLAT) } }
factored_time = Benchmark.measure("factored") { CORPUS.each { |s| s.scan(FACTORED) } }

slower, faster = [flat_time, factored_time].sort_by(&:real).reverse
ratio = slower.real / faster.real
faster_name = faster.equal?(flat_time) ? "flat" : "factored"
slower_name = slower.equal?(flat_time) ? "flat" : "factored"

puts <<~RESULTS

                  #{Benchmark::CAPTION}
  flat            #{flat_time.to_s.strip}
  factored        #{factored_time.to_s.strip}

  #{faster_name} is #{ratio.round(2)}x faster than #{slower_name} (real time)
RESULTS

[dejmedus]

factored is 1.01x faster than flat

Interesting!

[Morriar]

design is wrong, see comment

[Morriar]

So if contains_rbs_syntax? returns false we still initialize the object but do not call super. This means instance methods after that are responsible of knowing which state the instance is in. It's bad design.

Why do we even instantiate the Translator if we don't need to rewrite? This check should be made earlier.

Let's extract this to a maybe_rewrite singleton method.

[dejmedus]

If I'm understanding correctly, I believe this is what we originally had here but it was discussed that we could prevent needing to pass around values if the logic was moved inside

[Morriar]

The conditional call to super is a code smell, the fact that we can't test translation without the sigils is another one.

The main problems:

• Fragile: forgetting which condition triggers super leads to half-initialized objects. Future maintainers have to reason about two different initialization paths.
• Violates Liskov: subclass instances behave differently depending on whether the parent was initialized, which breaks substitutability.
• Hard to test: you need to cover both branches, and bugs in the "no super" path tend to surface late as nil errors.

Instead of short-circuiting the super call we should just short-circuit the class instantiation altogether since we don't need it.

[dejmedus]

Ah, I understand, thank you. I dropped the commit and now are exiting before initialization and calling new.rewrite in tests

[Morriar]

Thanks! Sorry for the back and forth about the design.

[amomchilov]

shipit