elp-revive / origami.el

A folding minor mode for Emacs
MIT License
32 stars 6 forks source link

Collapse fails in ruby mode if the file contains single-line class. #36

Open tofuya opened 3 years ago

tofuya commented 3 years ago

Description :octocat:

I get an error when I fold the code in a ruby file that contains a single-line class, module, and method.

As a side note, in Ruby, classes, modules, and methods can each be defined in a single line, as shown below:

class Foo < Bar; end
module Baz; end
def foo;  "hello" end
def bar() "hello" end
def foo(bar) = "hello" # since Ruby3.0

Reproduction guide :beetle:

  1. Start Emacs
  2. Open the following ruby file
    class Foo < Bar; end
    module Baz; end
    def foo;  "hello" end
    def bar() "hello" end
    def baz(v) = "hello"
  3. Run origami-close-all-nodes

Observed behaviour: :eyes: :broken_heart: (error "Offset is not within the range of the node: beg=1 ...")

Expected behaviour: :heart: :smile: No error in ruby file contains a single-line-definition

System Info :computer:

Backtrace :paw_prints:

Debugger entered--Lisp error: (error "Offset is not within the range of the node: beg=1 ...")
  signal(error ("Offset is not within the range of the node: beg=1 ..."))
  error("Offset is not within the range of the node: beg=%s..." 1 0 20)
  origami-fold-node(1 0 20 t nil nil)
  #f(compiled-function (beg end offset children) #<bytecode 0x1fedf5cec791>)(1 0 20 nil)
  #f(compiled-function (positions) #<bytecode 0x1fedf5ce467d>)(((#("class" 0 5 (face font-lock-keyword-face fontified t)) . 1) (#("end" 0 3 (face font-lock-keyword-face fontified t)) . 0) (#("module" 0 6 (face font-lock-keyword-face fontified t)) . 22) (#("end" 0 3 (face font-lock-keyword-face fontified t)) . 21) (#("def" 0 3 (face font-lock-keyword-face fontified t)) . 44) (#("end" 0 3 (face font-lock-keyword-face fontified t)) . 43) (#("def" 0 3 (face font-lock-keyword-face fontified t)) . 66) (#("end" 0 2 (face font-lock-keyword-face fontified t) 2 3 (face font-lock-keyword-face fontified t rear-nonsticky t)) . 65) (#("def" 0 3 (face font-lock-keyword-face fontified t)) . 88)))
  origami-build-pair-tree(#f(compiled-function (beg end offset children) #<bytecode 0x1fedf5cec791>) "\\(s*def\\|class\\|module\\|if\\|unless\\|while\\|until\\|..." "\\(s*end\\)\\_>" "\\(s*else\\|when\\|elsif\\)\\_>" ((#("class" 0 5 (face font-lock-keyword-face fontified t)) . 1) (#("end" 0 3 (face font-lock-keyword-face fontified t)) . 0) (#("module" 0 6 (face font-lock-keyword-face fontified t)) . 22) (#("end" 0 3 (face font-lock-keyword-face fontified t)) . 21) (#("def" 0 3 (face font-lock-keyword-face fontified t)) . 44) (#("end" 0 3 (face font-lock-keyword-face fontified t)) . 43) (#("def" 0 3 (face font-lock-keyword-face fontified t)) . 66) (#("end" 0 2 (face font-lock-keyword-face fontified t) 2 3 (face font-lock-keyword-face fontified t rear-nonsticky t)) . 65) (#("def" 0 3 (face font-lock-keyword-face fontified t)) . 88)) #f(compiled-function (&rest _) #<bytecode 0x1fedf4cb7409>))
  #f(compiled-function (content) #<bytecode 0x1fedf5cec7c9>)(#("class Foo < Bar; end\nmodule Foo < Bar; end\ndef foo..." 0 5 (face font-lock-keyword-face fontified t) 5 6 (fontified t) 6 9 (face font-lock-type-face fontified t) 9 12 (fontified t) 12 15 (face font-lock-type-face fontified t) 15 17 (fontified t) 17 20 (face font-lock-keyword-face fontified t) 20 21 (fontified t rear-nonsticky t) 21 27 (face font-lock-keyword-face fontified t) 27 28 (fontified t) 28 31 (face font-lock-type-face fontified t) 31 34 (fontified t) 34 37 (face font-lock-type-face fontified t) 37 39 (fontified t) 39 42 (face font-lock-keyword-face fontified t) 42 43 (fontified t rear-nonsticky t) ...))
  #f(compiled-function (content) #<bytecode 0x1fedf5ce4341>)(#("class Foo < Bar; end\nmodule Foo < Bar; end\ndef foo..." 0 5 (face font-lock-keyword-face fontified t) 5 6 (fontified t) 6 9 (face font-lock-type-face fontified t) 9 12 (fontified t) 12 15 (face font-lock-type-face fontified t) 15 17 (fontified t) 17 20 (face font-lock-keyword-face fontified t) 20 21 (fontified t rear-nonsticky t) 21 27 (face font-lock-keyword-face fontified t) 27 28 (fontified t) 28 31 (face font-lock-type-face fontified t) 31 34 (fontified t) 34 37 (face font-lock-type-face fontified t) 37 39 (fontified t) 39 42 (face font-lock-keyword-face fontified t) 42 43 (fontified t rear-nonsticky t) ...))
  origami-build-tree(#<buffer test.rb> #f(compiled-function (content) #<bytecode 0x1fedf5ce4341>))
  origami-get-fold-tree(#<buffer test.rb>)
  origami-close-all-nodes(#<buffer test.rb>)
  funcall-interactively(origami-close-all-nodes #<buffer test.rb>)
  call-interactively(origami-close-all-nodes nil nil)
  command-execute(origami-close-all-nodes)
jcs090218 commented 3 years ago

Thanks for the detail bug report!

I think this will require a rewrite to the ruby core parser since the current logic doesn't support single line folding and the parser will just assume the end of line will be the start of the folding region.

def foo;  # Folding start here
  "hello" 
end

in single line

def foo;  "hello" end  # Folding overlaps (to the end of line)

The logic needs to be updated so we can parser the ruby grammar better. Unlike C-like languages, it does not have simple way to find the proper folding region.


P.S. Any similar languages will have the same issue. Ruby, Lua, BASIC, etc.

tofuya commented 3 years ago

As I recall, this is the only pattern of Class, Module, and Method that can be defined in single line in Ruby.

a) class Foo < Bar; end b) module Baz; end c) def foo; "hello" end d) def bar() "hello" end e) def baz(v) = "hello"

For a), b), c), d), It seems to be enough to determine that anything ending with "end" is a single-line-definition. For the pattern in e), we only need to consider the Method, so if " =" (space and =) is included after def, we can assume it is a single-line-method. As an aside, the reason for the space is that the following methods are very commonly used as Setters, and a line without a space should not be considered a single-line-method.

def name=
  ...
end
In summary, the following criteria seem to be good. classification criteria
a) class ... end
b) module ... end
c), d) def ... end
e) def ... " =" ...
jcs090218 commented 3 years ago

Thanks for the suggestions! I have manage to make this to work! Can you upgrade to the latest version and see if it works? Thanks! 👍

tofuya commented 3 years ago

There were a few things that bothered me.

Errors have occurred

1)

class Foo < Bar
  def baz() = "hello"
end

Run origami-close-all-nodes => (error "Tried to construct a node where the children overl...")

2)

class Foo
  def bar(v)
    print v unless v
  end
end

Run origami-close-all-nodes => (wrong-type-argument number-or-marker-p nil)

The class name will be hidden

スクリーンショット 2021-05-31 15 32 11

I dodn't think it was necessary to hide the class name. After all, I'll have to open everything because I won't be able to see the outlines.

jcs090218 commented 3 years ago

(error "Tried to construct a node where the children overl...")

Does the code valid in Ruby? Sorry I am not very familiar to Ruby code. The problem is class is map to the end but missing another end for def.

(wrong-type-argument number-or-marker-p nil)

Can you set debug-on-error and paste the backtrace here? Thanks!

Hiding the class name isn't intentional. As I said, the parser doesn't know where to start and end the folding. I manage to work to shift the starting of the folding region right after the match.

Before

(fold end) (fold start) class ClassName ... end  # folding error, overlaps

After

class (fold start) ClassName ... (fold end) end  # Current workaround

I propose shift another word towards to the right? So it should end up like

class ClassName(fold start) ... (fold end) end  # Current workaround
tofuya commented 3 years ago

1) It's valid since Ruby 3.0 https://bugs.ruby-lang.org/issues/16746 2)

def bar(v)
  print v if v
end

Run origami-close-all-nodes =>

Debugger entered--Lisp error: (wrong-type-argument number-or-marker-p nil)
  #f(compiled-function (node) #<bytecode 0x1ff415b676f1>)([1 2305843009213693951 0 t ([1 27 9 t ([11 21 10 t nil #<overlay from 1 to 1 in test4.rb>]) #<overlay from 1 t$
  origami-fold-find-deepest([1 2305843009213693951 0 t ([1 27 9 t ([11 21 10 t nil #<overlay from 1 to 1 in test4.rb>]) #<overlay from 1 to 1 in test4.rb>]) root] #f(co$
  origami-fold-find-path-containing-range([1 2305843009213693951 0 t ([1 27 9 t ([11 21 10 t nil #<overlay from 1 to 1 in test4.rb>]) #<overlay from 1 to 1 in test4.rb>$
  origami-fold-find-path-with-range([1 2305843009213693951 0 t ([1 27 9 t ([11 21 10 t nil #<overlay from 1 to 1 in test4.rb>]) #<overlay from 1 to 1 in test4.rb>]) roo$
  #f(compiled-function (beg end offset children) #<bytecode 0x1ff4147b5ab5>)(1 nil 10 ([12 26 14 t nil #<overlay from 26 to 26 in test4.rb>]))
  #f(compiled-function (positions) #<bytecode 0x1ff415b5af81>)(((#("def" 0 3 (face font-lock-keyword-face fontified t)) . 1) (#("if" 0 2 (face font-lock-keyword-face fo$
  origami-build-pair-tree(#f(compiled-function (beg end offset children) #<bytecode 0x1ff4147b5ab5>) "\\(s*def\\|class\\|module\\|if\\|unless\\|while\\|until\\|..." "\\$
  #f(compiled-function (content) #<bytecode 0x1ff4147b5aed>)(#("def bar(v)\n  print v if v\nend\n" 0 3 (face font-lock-keyword-face fontified t) 3 4 (fontified t) 4 7 ($
  #f(compiled-function (content) #<bytecode 0x1ff414624f2d>)(#("def bar(v)\n  print v if v\nend\n" 0 3 (face font-lock-keyword-face fontified t) 3 4 (fontified t) 4 7 ($
  origami-build-tree(#<buffer test4.rb> #f(compiled-function (content) #<bytecode 0x1ff414624f2d>))
  origami-get-fold-tree(#<buffer test4.rb>)
  origami-close-all-nodes(#<buffer test4.rb>)
  funcall-interactively(origami-close-all-nodes #<buffer test4.rb>)
  call-interactively(origami-close-all-nodes nil nil)
  command-execute(origami-close-all-nodes)