tree-sitter / tree-sitter-ruby

Ruby grammar for tree-sitter
MIT License
176 stars 58 forks source link

Wrong Ruby indent when entering a `.` before `end` #230

Closed xiaochuanyu closed 4 months ago

xiaochuanyu commented 1 year ago

If you type . before an end, you get wrong indentation as seen below (in neovim). Before:

image

After:

image

See video in: https://github.com/nvim-treesitter/nvim-treesitter/issues/3363

Based on what @theHamsta said in that issue, it sounds like this might be fixable in the grammar?

The problem is in the ruby parser. It parses self.end as function call. Is end allowed in Ruby as an identifier? There were plans to better support keyword forbidden in identifiers (https://github.com/tree-sitter/tree-sitter/pull/1635).

This is how the ruby parser sees your file in the moment you enter the .

class User < ApplicationRecord

def foo
  if true
    self.
  end
end

Here what vscode shows (which doesn't use treesitter and doesn't have the same issue):

image
Slotos commented 1 year ago
[1] pry(main)> class A
[1] pry(main)*   def end
[1] pry(main)*     puts 3
[1] pry(main)*   end  
[1] pry(main)* end  
=> :end
[2] pry(main)> A.new.end
3
=> nil

#end is a valid ruby method name. As such, parser does its job correctly.

dgmora commented 1 year ago

hi! There hasn't been any answer in some months and this effectively makes indentation unusable, is there any plan to fix it?

I don't think one can really stick to end is technically a valid method if indentation is broken

Slotos commented 1 year ago

You're asking a third-party parser to fix something that ruby interpreter itself cannot deal with.

Create a file with the following contents, run it with ruby, check which line the interpreter complains about.

class A
  def a
    self.
  end

  def end
    42
  end

  private

  def even_better
    66.
  end
end

The only reason you know the indentation is wrong is because you're operating in a dimension the parser doesn't operate in - time. You intend to write more and so you know there will be a method that's not #end. Without human input the situation in undecidable, because someone else could instead go and modify the code above to look like this:

class A
  def a
    self.
      end
  end

  def end
    42
  end

  private

  def even_better
    66.to_s
  end
end

Parser doesn't know your intentions, it operates here and now, and here and now you're calling #end.

You can update your editor behaviour to reindent when a new method call appears in the tree. You can propose a general tree-sitter update that will make it operate on current input and an optional previous tree state, introducing temporal dimension that way. You could go to main nvim-treesitter repo and argue that https://github.com/nvim-treesitter/nvim-treesitter/blob/master/queries/ruby/indents.scm needs to be updated so that following lines are shifted to the right instead of current one shifting to the left.

What you can't do is claim that this parser is working incorrectly in this case.

Vagab commented 1 year ago

@Slotos any idea how does VSCode do this? What I'm getting at is that if they were able to do it - it's possible. The comments you suggested are not really helpful, at least with my level of experience, moreover they seem passive-aggressive. Nevertheless, the undoubtedly correct parser renders indentation useless. Which is far from ideal.

Slotos commented 1 year ago

I'm actively, openly, and aggressively stating that the claim "parser works incorrectly" is wrong. Meeting a direct criticism with accusation of passive aggression is the definition of irony.

Back to the topic, however. https://github.com/microsoft/vscode/blob/main/extensions/ruby/syntaxes/ruby.tmLanguage.json - this is how VSCode does it. https://github.com/vim-ruby/vim-ruby does indent ok-ish too. Neither is relevant to the question of generating a correct AST representation of the file.

The problem you're facing is indentation is wrong. You then jump to a conclusion that generated AST is wrong. But that's a textbook logical mistake: yes, wrong AST could lead to wrong indentation, but other things could lead to that too. In fact, this issue is not new and there are plugins implementing fallback regex indentation, for example https://github.com/yioneko/nvim-yati. Indent is marked as experimental in nvim-treesitter for a reason.

Indentation problems in case of the example I produced come from the fact that "class" doesn't get parsed as a class node, because it's not terminated. Parser can't assume the location of the terminator: there are six locations producing distinct valid versions of ruby code in my example. As such, class becomes a hanging node with no children. First def statement starts indentation, and as such its position is 0, and the next line position is +1. That's where that shift back when typing a dot comes from: class node disappears, def node becomes the root, node you're typing drop down in indentation index, nvim-treesitter indentation mechanism reflects that visually.

And all that boils down to limitation of nvim-treesitter indentation mechanism. It deals with nodes and their children. Whereas to deal with our ruby case, one should add an ability to indent following siblings. Alternatively, one could deal with the moment a new character is typed after the dot. Tree is in a state that nvim-treesitter indentation can deal with and so an automated reindent would remove the issue altogether.

in short - tree-sitter-ruby works correctly, the issue you're seeing comes from nvim-treesitter. Specifically, its indentation implementation.

Vagab commented 1 year ago

I did not meet the criticism with the accusation of passive-aggressiveness per se. Because the criticism was absent. You were the only one in this thread to use either wording “parser is incorrect” or “ast is incorrect”, and you are the only one who accused the other people on this thread of concluding that, albeit that didn’t happen. Moreover, what I see are people who are confused by the way the nvim-treesitter works and trying to find a way to work efficiently with it, work around it, or help the ones maintaining in any way.

Now to the explanation you provided - thanks a lot, it was very clear. Would you happen to know whether nvim-treesitter is planning to add an ability to indent following siblings or maybe you know how one would go about doing that? I’d be glad to help in any way. Alternatively, have you tried any of the workarounds yourself, e.g. regex based callback? Would be splendid to hear your suggestions to mitigate this issue. Regarding the auto-indent, I think it’s enabled on my machine, but it doesn’t always happen, when I have a moment I’ll try to provide an example when it doesn’t. Any way, I imagine until this is fixed on nvim-treesitter level the most obvious solution is to remove indentation all together and zealously re-learn to press tab key.

dgutov commented 1 year ago

Any way, I imagine until this is fixed on nvim-treesitter level the most obvious solution is to remove indentation all together and zealously re-learn to press tab key.

Why does nvim reindent on you typing ., though? Dropping that behavior by default seems like the easiest fix.

dgutov commented 1 year ago

Although that seems more like a bug with indentation code that with the parser anyway.

Vagab commented 1 year ago

@dgutov I don't think it's a bug, @Slotos was correct, this behaviour is perfectly explainable and expectable. The thing is though, if we want users to have better experience, we would need to add some exceptions. Afaik that's how VSCode does this and it makes sense

dgutov commented 1 year ago

The bug is that self. is indented to align with def. Why would it?

Maybe it's followed by a method called end on the next line. Or maybe it's followed by a different method name (again, on the next line). Why would self. be indented the same as def?

Slotos' examples don't mention that, only the original message shows it. And the videos in the linked issue.

For reference, I'm hacking on a Ruby major mode for Emacs using TS, and it doesn't have this indentation problem. Nor does it use any specific workaround for this case.

Vagab commented 1 year ago

@dgutov I'm by no means a tree-sitter expert, but to me it seems logical since end can be a method name, so maybe anything before . is aligned to the level of def. I might be wrong tho.

dgutov commented 1 year ago

so maybe anything before . is aligned to the level of def

But why? Does self.\nbar also align like that?

def foo
self.
  bar
end
sharkby7e commented 6 months ago

adding vim.cmd('autocmd FileType ruby setlocal indentkeys-=.') in my top level init.lua did the trick for me! very interesting conversation, reminding me that ruby doesn't care about white space!

credit: https://github.com/nvim-treesitter/nvim-treesitter/issues/3363#issuecomment-1538607633

willian commented 4 months ago

I disabled ruby indentation completely as I described here: https://github.com/nvim-treesitter/nvim-treesitter/issues/3363#issuecomment-2115177169