Display line diffs in `dsl --verify` error messages by dejmedus · Pull Request #2592 · Shopify/tapioca

dejmedus · 2026-04-10T17:32:16Z

Motivation

When tapioca dsl --verify detects out of date RBI files, the error message lists which files were added, removed, or changed. We could make it easier to visualize these changes by including the line diff output in the error message

Output example

Implementation

This PR diffs updated files against the originals during RBI verification and then includes the output in the error message when the diff is 250 lines or less, or whatever value is provided to --max-diff-lines flag

Tests

updated verify tests to include line diff
added a test for multi file changes and changes longer than max diff lines
added tests for diff behaviour

KaanOzkan · 2026-04-22T15:27:02Z

          end.join("\n")

-          raise Tapioca::Error, <<~ERROR
+          file_diff = if diff_output.lines.count.between?(1, 250)


Should this 250 limit be a CLI option instead and default to 250? dsl --verify --max-diff-lines=1000

Great idea 👌🏼

KaanOzkan · 2026-04-22T15:29:28Z

+
+        stdout
+      rescue => e
+        say_error("Failed to create #{filename} diff. #{e.message}", :yellow)


Why did you opt for yellow and not red? I think red is more consistent with rest of our errors.

I looked through say_error uses and it seemed like :yellow was used for warnings and :red for errors. I leaned towards this being a warning. Makes sense, changed to :red :)

KaanOzkan · 2026-04-22T15:45:56Z

+          @command = DslVerify.new(
+            requested_constants: [],
+            requested_paths: [],
+            outpath: Pathname.new("tmp"),


Does DslVerify write something to the file system? I think other DslVerify tests run on a temporary directory that's cleaned up but this is not an integration test so it shouldn't use that, it's more costly.

If it is writing something we should clean up after, maybe we can create @tmp = Dir.mktmpdir in the before block, use that as the outpath and then clean it up in an after block.

If im understanding correctly, outpath is required to create DslVerify but the command isn't being executed here, I just need an instance to pull file_diff from. I changed it to /dev/null to make it clearer that its unused. tbh I wouldn't be that against removing this test file if we don't find it helpful, file diff is being tested by verify tests also

Actually ignore above, I moved out file_diff to rbi_files_helpers and the tests look much nicer now :)

KaanOzkan

Sorry I'm suggesting a refactor that I didn't noticed before.

KaanOzkan · 2026-04-27T13:57:58Z

+        old_path.to_s,
+        new_path.to_s,
+      )
+      Kernel.raise stderr.chomp unless [0, 1].include?(status.exitstatus)


Using exceptions for control flow is frowned upon and slow. So we shouldn't rely on it when we can just call say_error here instead. If Open3 itself can raise while running keep the rescue block too.

Suggested change

Kernel.raise stderr.chomp unless [0, 1].include?(status.exitstatus)

say_error("Failed to create #{filename} diff. #{e.message}", :red) unless [0, 1].include?(status.exitstatus)

Using exceptions for control flow is frowned upon and slow

Noted, thank you !

KaanOzkan · 2026-04-27T14:11:56Z

      #: (Pathname dir) -> void
      def perform_dsl_verification(dir)
-        diff = verify_dsl_rbi(tmp_dir: dir)
+        diff, diff_output = verify_dsl_rbi(tmp_dir: dir)


I don't love the tuple return here and I also forgot about report_diff_and_exit_if_out_of_date. What do you think about moving the calculation of diff output to report_diff_and_exit_if_out_of_date? It's the same class but it'll move some logic away from perform_dsl_verification/verify_dsl_rbi and make it easier to read. Something like this:

#: (Hash[String, Symbol] diff, tmp_dir: Pathname, command: Symbol) -> void def report_diff_and_exit_if_out_of_date(diff, tmp_dir:, command:) if diff.empty? say("Nothing to do, all RBIs are up-to-date.") return end reasons = diff.group_by(&:last).sort.map do |cause, diff_for_cause| build_error_for_files(cause, diff_for_cause.map(&:first)) end.join("\n") diff_output = build_diff_output(diff, tmp_dir) diff_lines = diff_output.count("\n") diff_section = if diff_lines.between?(1, @max_diff_lines) "#{set_color("Diff:", :red)}\n#{diff_output.chomp}" elsif diff_lines > @max_diff_lines set_color("Diff not displayed as it exceeds #{@max_diff_lines} lines", :red) else "" end raise Tapioca::Error, <<~ERROR.rstrip #{set_color("RBI files are out-of-date. In your development environment, please run:", :green)} #{set_color("`#{default_command(command)}`", :green, :bold)} #{set_color("Once it is complete, be sure to commit and push any changes", :green)} If you don't observe any changes after running the command locally, ensure your database is in a good state e.g. run `bin/rails db:reset` #{set_color("Reason:", :red)} #{reasons} #{diff_section} ERROR end #: (Hash[String, Symbol] diff, Pathname tmp_dir) -> String def build_diff_output(diff, tmp_dir) out = String.new line_count = 0 diff.each do |file, status| chunk = case status when :added then file_diff(file, File::NULL, tmp_dir / file) when :removed then file_diff(file, @outpath / file, File::NULL) when :changed then file_diff(file, @outpath / file, tmp_dir / file) end out << chunk line_count += chunk.count("\n") break if line_count > @max_diff_lines end out end

Agree, changed. Thanks!

KaanOzkan · 2026-04-27T14:15:10Z

+  def initialize(name)
+    Minitest::Runnable.instance_method(:initialize).bind(self).call(name)
+  end
+end


Instead of this could we get away with defining our own say_error method? It's simpler if that's all we need.

We could, but I believe we'd have to turn off type check in the file since files helpers require Thor as ancestor

Hmm yeah I guess we could also extract file_diff to another file and remove the say_error dependency. Curious if anyone else has a better suggestion, worth asking on slack.

extract file_diff to another file and remove the say_error dependency

Done:) I moved file_diff to its own file and replaced say_error with a warn

amomchilov · 2026-06-29T16:59:26Z

+      stdout, stderr, status = Open3.capture3(
+        "diff",
+        "-u",
+        "--label",
+        filename,
+        "--label",
+        filename,
+        old_path,
+        new_path,
+      )


You can also use the = form to keep the --label flag name with its value, and you can interleave labels and file paths:

Suggested change

stdout, stderr, status = Open3.capture3(

"diff",

"-u",

"--label",

filename,

"--label",

filename,

old_path,

new_path,

)

stdout, stderr, status = Open3.capture3(

"diff",

"-u",

"--label=\"Current #{filename}\"",

old_path,

"--label=\"Correct #{filename} (After running `bin/tapioca dsl`)\"",

new_path,

)

Example:

$ diff -u --label="before" <(echo "hello") --label "after" <(echo "world") --- before +++ after @@ -1 +1 @@ -hello +world

amomchilov · 2026-06-29T17:04:38Z

+          filename = file.to_s
+          old_path = (@outpath / file).to_s
+          new_path = (tmp_dir / file).to_s


Move the to_s calls into file_diff

amomchilov · 2026-06-29T17:06:20Z

+          chunk = case status
+          when :added then file_diff(filename, File::NULL, new_path)
+          when :removed then file_diff(filename, old_path, File::NULL)
+          when :changed then file_diff(filename, old_path, new_path)


Just to confirm, does this ever call file_diff at most once per path? Or is it possible diff contains multiple entries pertaining to the same file?

Yes, each file can only have one status (added, removed, or changed)

amomchilov · 2026-06-29T22:56:59Z

+      end
+
+      stdout
+    rescue => e


What are we aiming to rescue here, exactly?

I narrowed this to SystemCallError. The thought was that if for some reason we couldn't execute diff we'd continue, though maybe thats overkill?

amomchilov · 2026-06-29T22:57:25Z

+
+            Diff:
+            --- Current post.rbi
+            +++ Correct post.rbi (After running `bin/tapioca dsl`)


amomchilov · 2026-06-29T22:57:45Z

+            -
+            -# DO NOT EDIT MANUALLY
+            -# This is an autogenerated file for dynamic methods in `Post`.
+            -# Please instead update this file by running `bin/tapioca dsl Post`.


Oooo can we reuse whatever logic is used to generate this bin/tapioca dsl Post, and use that in the +++ label above?

This would be awesome! Currently we don't have easy access to the constant by the time we're here, but I'll work on a separate PR for this

amomchilov · 2026-06-29T22:58:12Z

+            --- Current post.rbi
+            +++ Correct post.rbi (After running `bin/tapioca dsl`)
+            @@ -10,6 +10,12 @@
+            #{" "}


intentional spaces? Kinda weird

Kinda weird

Agree, diff -u uses single space as the "unchanged" marker

amomchilov · 2026-06-29T23:00:56Z

+      Dir.mktmpdir do |dir|
+        a = File.join(dir, "a")
+        b = File.join(dir, "b")
+        File.write(a, "line1\nline2\n")
+        File.write(b, "line1\nline3\n")


Suggested change

Dir.mktmpdir do |dir|

a = File.join(dir, "a")

b = File.join(dir, "b")

File.write(a, "line1\nline2\n")

File.write(b, "line1\nline3\n")

Pathname.mktmpdir do |dir|

a = dir / "a"

b = dir / "b"

a.write("line1\nline2\n")

b.write("line1\nline3\n")

Likewise below

It seems like we don't get Pathname.mktmpdir until Ruby 3.4 and gemspec min is 3.2, but I just wrapped it

KaanOzkan · 2026-07-03T14:09:43Z

+            "#{set_color("Diff:", :red)}\n#{diff_output.chomp}"
+          elsif diff_lines > @max_diff_lines
+            truncated_output = diff_output.lines.first(@max_diff_lines).join
+            "#{set_color("Diff truncated to #{@max_diff_lines} lines:", :red)}\n#{truncated_output.rstrip}"


Could mention the new CLI flag here.

Good idea!

message

KaanOzkan · 2026-07-03T14:13:20Z

        halt_upon_load_error: options[:halt_upon_load_error],
        compiler_options: options[:compiler_options],
        lsp_addon: self.class.addon_mode,
+        max_diff_lines: options[:max_diff_lines],


We should check that it's a positive number just in case. It'd error later on anyways but I think it's good to validate user input at this layer.

amomchilov · 2026-07-06T20:01:22Z

+        assert_includes(result, "--- Current x.rbi")
+        assert_includes(result, "+++ Correct x.rbi (After running `bin/tapioca dsl`)")
+        assert_includes(result, "-line2")
+        assert_includes(result, "+line3")


Any reason not to assert over the entire output? That'll ensure the order of these snippets is correct

amomchilov · 2026-07-06T20:05:03Z

+          line_count += chunk.count("\n")
+          break if line_count > @max_diff_lines


Minor bug: this can permit too many lines. Imagine the limit is 250, and there's 3 chunks of 200 each.

The first one will be let through, but so would the second (without truncation). The line_count will hit 400, so we would break before the third chunk get's concatenated in, but we won't trim off the 150 excess lines of the second chunk.

Truncation and the this was truncated message output were happening in report_diff. The intention with break if line_count > @max_diff_lines was just to not loop longer than necessary

I can see how that was odd/confusing tho, so I've moved truncation into build_diff and now return both the output and whether it was truncated

When `dsl --verify` detects out of date RBI files, the error message lists which files were added, removed, or changed. We could make it easier to understand what has changed by including the line diff output in the error message as well This commit grabs the line diff output during RBI verification and includes it in the error message when the diff is 250 lines or less

Morriar · 2026-07-08T18:08:32Z

+        "-u",
+        "--label=Current #{filename}",
+        old_path.to_s,
+        "--label=Correct #{filename} (After running `bin/tapioca dsl`)",


Suggested change

"--label=Correct #{filename} (After running `bin/tapioca dsl`)",

"--label=Expected #{filename} (After running `bin/tapioca dsl`)",

Morriar · 2026-07-08T18:09:51Z

+
+      unless [0, 1].include?(status.exitstatus)
+        error_msg("Failed to create #{filename} diff. #{stderr.chomp}")
+        return ""


I'd return nil if it couldn't be computed

dejmedus added the enhancement New feature or request label Apr 10, 2026

dejmedus force-pushed the jb-add-diff-output-to-error-msg branch 3 times, most recently from f3c5b51 to 94ecc33 Compare April 21, 2026 19:31

dejmedus marked this pull request as ready for review April 21, 2026 20:46

dejmedus requested a review from a team as a code owner April 21, 2026 20:46

KaanOzkan reviewed Apr 22, 2026

View reviewed changes

dejmedus force-pushed the jb-add-diff-output-to-error-msg branch 3 times, most recently from 0aa9400 to 6bdba8a Compare April 24, 2026 21:11

dejmedus requested a review from KaanOzkan April 24, 2026 23:57

KaanOzkan reviewed Apr 27, 2026

View reviewed changes

dejmedus force-pushed the jb-add-diff-output-to-error-msg branch from 6bdba8a to 85ec455 Compare April 30, 2026 16:50

dejmedus requested a review from KaanOzkan April 30, 2026 18:01

amomchilov reviewed Jun 29, 2026

View reviewed changes

dejmedus force-pushed the jb-add-diff-output-to-error-msg branch from 85ec455 to 70de51e Compare June 29, 2026 22:47

amomchilov reviewed Jun 29, 2026

View reviewed changes

dejmedus force-pushed the jb-add-diff-output-to-error-msg branch from 70de51e to abe279f Compare July 3, 2026 02:24

dejmedus requested a review from amomchilov July 3, 2026 03:01

KaanOzkan reviewed Jul 3, 2026

View reviewed changes

dejmedus force-pushed the jb-add-diff-output-to-error-msg branch from abe279f to 384c457 Compare July 6, 2026 16:11

dejmedus requested a review from KaanOzkan July 6, 2026 16:50

KaanOzkan approved these changes Jul 6, 2026

View reviewed changes

dejmedus force-pushed the jb-add-diff-output-to-error-msg branch from 384c457 to 1d85148 Compare July 6, 2026 18:05

amomchilov approved these changes Jul 6, 2026

View reviewed changes

dejmedus force-pushed the jb-add-diff-output-to-error-msg branch from 1d85148 to 7ebcc35 Compare July 7, 2026 16:01

dejmedus force-pushed the jb-add-diff-output-to-error-msg branch from 7ebcc35 to b7963a2 Compare July 7, 2026 16:02

Morriar reviewed Jul 8, 2026

View reviewed changes

	Kernel.raise stderr.chomp unless [0, 1].include?(status.exitstatus)
	say_error("Failed to create #{filename} diff. #{e.message}", :red) unless [0, 1].include?(status.exitstatus)

		line_count += chunk.count("\n")
		break if line_count > @max_diff_lines

	"--label=Correct #{filename} (After running `bin/tapioca dsl`)",
	"--label=Expected #{filename} (After running `bin/tapioca dsl`)",

Uh oh!

Conversation

dejmedus commented Apr 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Implementation

Tests

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dejmedus Apr 23, 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!

Choose a reason for hiding this comment

Uh oh!

KaanOzkan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dejmedus Apr 30, 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!

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!

dejmedus Apr 30, 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!

Choose a reason for hiding this comment

Uh oh!

dejmedus Jul 3, 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!

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!

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

dejmedus commented Apr 10, 2026 •

edited

Loading

dejmedus Apr 23, 2026 •

edited

Loading

dejmedus Apr 30, 2026 •

edited

Loading

dejmedus Apr 30, 2026 •

edited

Loading

dejmedus Jul 3, 2026 •

edited

Loading

dejmedus Jul 3, 2026 •

edited

Loading

dejmedus Jul 7, 2026 •

edited

Loading