Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Powerup show_source by enabling RubyVM.keep_script_lines #862

Merged
merged 6 commits into from
Feb 12, 2024

Conversation

tompng
Copy link
Member

@tompng tompng commented Feb 4, 2024

With RubyVM.keep_script_lines = true, we can show method source defined in IRB.

% irb
irb(main):001> def foo; end
=> :foo
irb(main):002> $ foo

From: (irb):1

def foo; end

=> nil
% ruby -Ilib a.rb
From: a.rb @ line 1 :

 => 1: binding.irb

irb(main):001> def foo; end
=> :foo
irb(main):002> $ foo

From: /path/to/a.rb(irb):1

def foo; end

=> nil

Other changes

irb(main):003> $ StringIO

Defined in binary file: /path/to/lib/stringio.bundle

=> nil
irb(main):004> $ Object#tap

From: <internal:kernel>:89

Source not available

=> nil

SourceFinder#find_source

Error: Couldn't locate a definition for ...

Even if the file is a binary file or does not exist, I think find_source can return a value.
We actually located the definition, but the content of the source is just not available.

find_source('StringIO') #=> { file: 'lib/stringio.bundle', first_line: nil, last_line: nil }
find_source('foobar') #=> { file: '(irb)', first_line: 1, last_line: nil, content: 'def foobar; end' }
find_source('tap') #=> { file: '<internal:kernel>', first_line: 89, last_line: nil }

lib/irb.rb Outdated
@@ -1523,6 +1528,8 @@ def irb(show_code: true)
debugger_irb = IRB.instance_variable_get(:@debugger_irb)

irb_path = File.expand_path(source_location[0])
# We need to change the irb_path to distinguish source_location of method defined in the actual file and method defined in irb session.
irb_path = "#{irb_path}(#{IRB.conf[:IRB_NAME]})" if File.exist?(irb_path)
Copy link
Member Author

@tompng tompng Feb 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we don't append "(#{irb_name})", show_source won't work correctly.

Save this to a.rb

def bar; end
binding.irb

And run ruby -I path/to/irb/lib a.rb

irb(main):001> def foo; end
irb(main):002> $ foo

Without appending, method(:foo).source_location and method(:bar).source_location will both return the same value ["a.rb", 1]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the edit command uses context.irb_path to open the breakpoint's file, this will break it.

irb(main):001> edit
command: 'code'
   path: /Users/hung-wulo/src/github.com/ruby/irb/test.rb
=> true

With the PR:

From: test.rb @ line 7 :

    2:   def bar
    3:     puts '123'
    4:   end
    5: end
    6: 
 => 7: binding.irb

irb(main):001> edit
/Users/hung-wulo/src/github.com/ruby/irb/lib/irb/source_finder.rb:35:in `eval': (eval):1: unknown regexp options - hg (SyntaxError)
/Users/hung-wulo/src/github.com/ruby/irb...

So instead of changing the value of context#irb_path, let's modify the one we pass to @workspace.evaluate? I feel it's a better solution even without the issue as it makes the in-session definitions have clearer source location too.

Copy link
Member Author

@tompng tompng Feb 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch! changed to:

eval_path = File.exist?(irb_path) ? "#{irb_path}(#{IRB.conf[:IRB_NAME]})" : irb_path
...
result = @workspace.evaluate(line, eval_path, line_no)

I also changed (1ec0b06) edit and add a test for non-existing irb_path for:

% ruby -I path/to/irb/lib a.rb
From: a.rb @ line 1 :

 => 1: binding.irb

irb(main):001> IRB.CurrentContext.irb_path
=> "/Users/tomoya.ishida/github/ruby/irb/a.rb"
irb(main):002> edit
command: 'emacs'
   path: /Users/tomoya.ishida/github/ruby/irb/a.rb
=> true
irb(main):003> binding.irb # this part is evaluated with eval_path="a.rb(irb)"
irb(main):001> IRB.CurrentContext.irb_path
=> "/Users/tomoya.ishida/github/ruby/irb/a.rb(irb)"
irb(main):002> edit
Can not find file: /Users/tomoya.ishida/github/ruby/irb/a.rb(irb)
=> nil

To avoid path passed to SourceFinder (SourceFinder will evaluate path as ruby code)

irb(main):002> edit
`eval': unknown regexp options - tya (SyntaxError)
/Users/tomoya.ishida/github/ruby/irb/a
      ^~~~~~~

@st0012
Copy link
Member

st0012 commented Feb 6, 2024

I think we can simplify the source handling conditions a bit:

diff --git a/lib/irb/cmd/show_source.rb b/lib/irb/cmd/show_source.rb
index 5400500..c201d03 100644
--- a/lib/irb/cmd/show_source.rb
+++ b/lib/irb/cmd/show_source.rb
@@ -45,23 +45,29 @@ module IRB
       private
 
       def show_source(source)
-        if source.content
-          code = IRB::Color.colorize_code(source.content)
-        elsif source.first_line && source.last_line
-          file_content = IRB::Color.colorize_code(File.read(source.file))
-          code = file_content.lines[(source.first_line - 1)...source.last_line].join
-        elsif source.first_line
-          code = 'Source not available'
-        else
-          content = "\n#{bold('Defined in binary file')}: #{source.file}\n\n"
-        end
-        content ||= <<~CONTENT
+        content =
+          if source.first_line
+            code =
+              if source.content
+                IRB::Color.colorize_code(source.content)
+              else
+                'Source not available'
+              end
 
-          #{bold("From")}: #{source.file}:#{source.first_line}
+            <<~CONTENT
 
-          #{code.chomp}
+              #{bold("From")}: #{source.file}:#{source.first_line}
 
-        CONTENT
+              #{code.chomp}
+
+            CONTENT
+          else
+            <<~CONTENT
+            #{bold('Defined in binary file')}: #{source.file}
+
+
+            CONTENT
+          end
 
         Pager.page_content(content)
       end
diff --git a/lib/irb/source_finder.rb b/lib/irb/source_finder.rb
index ddb125c..7362e40 100644
--- a/lib/irb/source_finder.rb
+++ b/lib/irb/source_finder.rb
@@ -7,7 +7,6 @@ module IRB
     Source = Struct.new(
       :file,       # @param [String]  - file name
       :first_line, # @param [Integer] - first line (unless binary file)
-      :last_line,  # @param [Integer] - last line (if available from file)
       :content,    # @param [String]  - source (if available from AST)
       keyword_init: true,
     )
@@ -44,7 +43,10 @@ module IRB
           # If the line is zero, it means that the target's source is probably in a binary file.
           Source.new(file: file)
         else
-          Source.new(file: file, first_line: line, last_line: find_end(file, line))
+          file_lines = File.read(file).lines
+          last_line = find_end(file_lines, line)
+          content = file_lines[(line - 1)...last_line].join
+          Source.new(file: file, first_line: line, content: content)
         end
       elsif method
         # Method defined with eval, probably in IRB session
@@ -55,9 +57,9 @@ module IRB
 
     private
 
-    def find_end(file, first_line)
+    def find_end(file_lines, first_line)
       lex = RubyLex.new
-      lines = File.read(file).lines[(first_line - 1)..-1]
+      lines = file_lines[(first_line - 1)..-1]
       tokens = RubyLex.ripper_lex_without_warning(lines.join)
       prev_tokens = []

This also reduces file reads from 2 to 1. WDYT?

@st0012 st0012 assigned st0012 and unassigned st0012 Feb 6, 2024
@st0012 st0012 added the enhancement New feature or request label Feb 6, 2024
@tompng
Copy link
Member Author

tompng commented Feb 7, 2024

Yes, I though of it, it's simple, and it is consistent that source.content always exist if content is available.
But it will break the coloring.

name = 'br'
Object.class_eval <<CODE, __FILE__, __LINE__ + 1
  def #{name}() = puts("<#{name}/>")
CODE

Result of show_source br, (first: colorize(code).lines[first..last], second: colorize(content)
show_source_coloring

@tompng
Copy link
Member Author

tompng commented Feb 7, 2024

Changed:
Always set source.content if content is available (even if it is currently not used in show_source)
Add source.file_content. This will avoid double reading the file
Add comment of why coloring source.file_content instead of source.content

lib/irb.rb Outdated
@@ -1523,6 +1528,8 @@ def irb(show_code: true)
debugger_irb = IRB.instance_variable_get(:@debugger_irb)

irb_path = File.expand_path(source_location[0])
# We need to change the irb_path to distinguish source_location of method defined in the actual file and method defined in irb session.
irb_path = "#{irb_path}(#{IRB.conf[:IRB_NAME]})" if File.exist?(irb_path)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the edit command uses context.irb_path to open the breakpoint's file, this will break it.

irb(main):001> edit
command: 'code'
   path: /Users/hung-wulo/src/github.com/ruby/irb/test.rb
=> true

With the PR:

From: test.rb @ line 7 :

    2:   def bar
    3:     puts '123'
    4:   end
    5: end
    6: 
 => 7: binding.irb

irb(main):001> edit
/Users/hung-wulo/src/github.com/ruby/irb/lib/irb/source_finder.rb:35:in `eval': (eval):1: unknown regexp options - hg (SyntaxError)
/Users/hung-wulo/src/github.com/ruby/irb...

So instead of changing the value of context#irb_path, let's modify the one we pass to @workspace.evaluate? I feel it's a better solution even without the issue as it makes the in-session definitions have clearer source location too.

@@ -37,7 +37,8 @@ def execute(*args)
# in this case, we should just ignore the error
end

if source
# Ignore `source.file == "(irb)"` and `source.first_line == nil` (binary file)
if source && File.exist?(source.file) && source.first_line
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel this should be encapsulated in a Source#editable? method.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍
Added Source#file_exist? and Source#binary_file?. source.binary_file? is also used from show_source

file_content = IRB::Color.colorize_code(source.file_content)
code = file_content.lines[(source.first_line - 1)...source.last_line].join
elsif source.content
code = IRB::Color.colorize_code(source.content)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just an idea: how about we make Source a normal Ruby class and encapsulate these conditions 2 conditions inside a colorized_content method.

And then we can add a unit test for it to cover the partial coloring failure case too.

@tompng tompng force-pushed the show_source_plusplus branch from fffd02e to 3362f30 Compare February 12, 2024 14:40
@tompng tompng force-pushed the show_source_plusplus branch from 3362f30 to 902727f Compare February 12, 2024 14:50
@@ -555,9 +555,12 @@ def evaluate(line, line_no) # :nodoc:
IRB.set_measure_callback
end

# We need to use differente path to distinguish source_location of method defined in the actual file and method defined in irb session.
eval_path = File.exist?(irb_path) ? "#{irb_path}(#{IRB.conf[:IRB_NAME]})" : irb_path
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be an ivar for Context instead or a memoised method so we don't run File.exist? on every input? It'd mean IRB.conf[:IRB_NAME] changes between inputs won't be picked up, but IMO that's ok.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IRB_NAME does not change frequently, but irb_path does. (in irb_load and rdbg)
Updated to memoize irb_path existence considering irb_path change.

@tompng tompng force-pushed the show_source_plusplus branch from 1ec0b06 to 125693c Compare February 12, 2024 16:40
Copy link
Member

@st0012 st0012 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an awesome enhancement 🎉
It also checks one of the points mentioned in #775

@tompng tompng merged commit 239683a into ruby:master Feb 12, 2024
29 checks passed
@tompng tompng deleted the show_source_plusplus branch February 12, 2024 18:38
matzbot pushed a commit to ruby/ruby that referenced this pull request Feb 12, 2024
(ruby/irb#862)

* Powerup show_source by enabling RubyVM.keep_script_lines

* Add file_content field to avoid reading file twice while show_source

* Change path passed to eval, don't change irb_path.

* Encapsulate source coloring logic and binary file check insode class Source

* Add edit command testcase when irb_path does not exist

* Memoize irb_path existence to reduce file existence check calculating eval_path

ruby/irb@239683a937
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

2 participants