From 921f22e563d6372d9f853f87d48b11c479126da9 Mon Sep 17 00:00:00 2001
From: Stan Lo <stan001212@gmail.com>
Date: Tue, 18 Jun 2024 16:15:19 +0100
Subject: [PATCH] [ruby/irb] Improve how command calls' return value is handled
 (https://github.com/ruby/irb/pull/972)

In #934, we changed command calls to return nil only. This PR improves
the behaviour even further by:

- Not echoing the `nil` returned by command calls
- Not overriding previous return value stored in `_` with the
  `nil` from commands

https://github.com/ruby/irb/commit/c844176842
---
 lib/irb/context.rb       |  1 -
 lib/irb/statement.rb     |  2 +-
 test/irb/test_command.rb | 34 ++++++++++++----------------------
 test/irb/test_irb.rb     | 15 +++++++++++++++
 4 files changed, 28 insertions(+), 24 deletions(-)

diff --git a/lib/irb/context.rb b/lib/irb/context.rb
index aafce7aade..74a08ca694 100644
--- a/lib/irb/context.rb
+++ b/lib/irb/context.rb
@@ -602,7 +602,6 @@ module IRB
         set_last_value(result)
       when Statement::Command
         statement.command_class.execute(self, statement.arg)
-        set_last_value(nil)
       end
 
       nil
diff --git a/lib/irb/statement.rb b/lib/irb/statement.rb
index a3391c12a3..9591a40357 100644
--- a/lib/irb/statement.rb
+++ b/lib/irb/statement.rb
@@ -68,7 +68,7 @@ module IRB
       end
 
       def suppresses_echo?
-        false
+        true
       end
 
       def should_be_handled_by_debugger?
diff --git a/test/irb/test_command.rb b/test/irb/test_command.rb
index 8cb8928adb..30c3f5ca2c 100644
--- a/test/irb/test_command.rb
+++ b/test/irb/test_command.rb
@@ -239,7 +239,7 @@ module TestIRB
       )
 
       assert_empty err
-      assert_match(/\A(TIME is added\.\n=> nil\nprocessing time: .+\n=> 3\n=> nil\n=> 3\n){2}/, out)
+      assert_match(/\A(TIME is added\.\nprocessing time: .+\n=> 3\n=> 3\n){2}/, out)
       assert_empty(c.class_variables)
     end
 
@@ -266,7 +266,7 @@ module TestIRB
       )
 
       assert_empty err
-      assert_match(/\ATIME is added\.\n=> nil\nprocessing time: .+\n=> 3\nprocessing time: .+\n=> 3/, out)
+      assert_match(/\ATIME is added\.\nprocessing time: .+\n=> 3\nprocessing time: .+\n=> 3/, out)
       assert_empty(c.class_variables)
     end
 
@@ -291,7 +291,7 @@ module TestIRB
       )
 
       assert_empty err
-      assert_match(/\Aprocessing time: .+\n=> 3\n=> nil\n=> 3\n/, out)
+      assert_match(/\Aprocessing time: .+\n=> 3\n=> 3\n/, out)
     end
 
     def test_measure_enabled_by_rc_with_custom
@@ -321,7 +321,7 @@ module TestIRB
         conf: conf,
       )
       assert_empty err
-      assert_match(/\Acustom processing time: .+\n=> 3\n=> nil\n=> 3\n/, out)
+      assert_match(/\Acustom processing time: .+\n=> 3\n=> 3\n/, out)
     end
 
     def test_measure_with_custom
@@ -353,7 +353,7 @@ module TestIRB
       )
 
       assert_empty err
-      assert_match(/\A=> 3\nCUSTOM is added\.\n=> nil\ncustom processing time: .+\n=> 3\n=> nil\n=> 3\n/, out)
+      assert_match(/\A=> 3\nCUSTOM is added\.\ncustom processing time: .+\n=> 3\n=> 3\n/, out)
     end
 
     def test_measure_toggle
@@ -385,7 +385,7 @@ module TestIRB
       )
 
       assert_empty err
-      assert_match(/\AFOO is added\.\n=> nil\nfoo\n=> 1\nBAR is added\.\n=> nil\nbar\nfoo\n=> 2\n=> nil\nbar\n=> 3\n=> nil\n=> 4\n/, out)
+      assert_match(/\AFOO is added\.\nfoo\n=> 1\nBAR is added\.\nbar\nfoo\n=> 2\nbar\n=> 3\n=> 4\n/, out)
     end
 
     def test_measure_with_proc_warning
@@ -410,7 +410,7 @@ module TestIRB
       )
 
       assert_match(/to add custom measure/, err)
-      assert_match(/\A=> 3\n=> nil\n=> 3\n/, out)
+      assert_match(/\A=> 3\n=> 3\n/, out)
       assert_empty(c.class_variables)
     end
   end
@@ -429,8 +429,7 @@ module TestIRB
         /=> "bug17564"\n/,
         /=> "bug17564"\n/,
         /   => "hi"\n/,
-        /   => nil\n/,
-        /=> "hi"\n/,
+        /   => "hi"\n/,
       ], out)
     end
 
@@ -457,8 +456,7 @@ module TestIRB
           /=> "bug17564"\n/,
           /=> "bug17564"\n/,
           /   => "hi"\n/,
-          /   => nil\n/,
-          /=> "bug17564"\n/,
+          /   => "bug17564"\n/,
         ], out)
     end
 
@@ -760,10 +758,7 @@ module TestIRB
 
   class ShowDocTest < CommandTestCase
     def test_show_doc
-      out, err = execute_lines(
-        "show_doc String#gsub\n",
-        "\n",
-      )
+      out, err = execute_lines("show_doc String#gsub")
 
       # the former is what we'd get without document content installed, like on CI
       # the latter is what we may get locally
@@ -776,15 +771,10 @@ module TestIRB
     end
 
     def test_show_doc_without_rdoc
-      out, err = without_rdoc do
-        execute_lines(
-          "show_doc String#gsub\n",
-          "\n",
-        )
+      _, err = without_rdoc do
+        execute_lines("show_doc String#gsub")
       end
 
-      # if it fails to require rdoc, it only returns the command object
-      assert_match(/=> nil\n/, out)
       assert_include(err, "Can't display document because `rdoc` is not installed.\n")
     ensure
       # this is the only way to reset the redefined method without coupling the test with its implementation
diff --git a/test/irb/test_irb.rb b/test/irb/test_irb.rb
index ece7902909..e8d5be285e 100644
--- a/test/irb/test_irb.rb
+++ b/test/irb/test_irb.rb
@@ -56,6 +56,21 @@ module TestIRB
       assert_include output, "=> 12"
     end
 
+    def test_commands_dont_override_stored_last_result
+      write_ruby <<~'RUBY'
+        binding.irb
+      RUBY
+
+      output = run_ruby_file do
+        type "1 + 1"
+        type "ls"
+        type "_ + 10"
+        type "exit!"
+      end
+
+      assert_include output, "=> 12"
+    end
+
     def test_evaluate_with_encoding_error_without_lineno
       if RUBY_ENGINE == 'truffleruby'
         omit "Remove me after https://github.com/ruby/prism/issues/2129 is addressed and adopted in TruffleRuby"