feat(lua)!: switch from our fork to MunifTanjim's

[clason]

also take queries from https://github.com/MunifTanjim/nvim-treesitter-lua/tree/main/queries/lua

@MunifTanjim (it would help if you could summarize the advantages of your parser over the old one in a comment).

BREAKING CHANGE: queries are not compatible; modules will have to update

[clason]

Test failures are due to upstream queries missing

1. builtin functions highlighting
2. indentation

as well as a (new?) Markdown test fail.

[MunifTanjim]

Well, wow! That happened fast! 😂

it would help if you could summarize the advantages of your parser over the old one in a comment

Let me take it from my comment on reddit.

---

Block Comment/String

The old one didn't have separate nodes for block comment/string start/end. Since lua's block comment/string's start/end can be of arbritary length (e.g. [=[ or [======[, you get the idea), having separate nodes for them helps when you're writing query that targets string content (e.g. injections queries).

It also had some bugs handling these block comments/strings.

• Lua parser fails to find ending of string enclosed by level 1 long brackets if the last character of the string is a square bracket tree-sitter-lua#7

Table access

tbl.a["b"]:c()

Old parser:

(program
  (function_call
    (field_expression
      (identifier)
      (property_identifier))
    (string)
    (method)
    (arguments)))

This parser:

(chunk
  (function_call
    name: (method_index_expression
      table: (bracket_index_expression
        table: (dot_index_expression
          table: (identifier)
          field: (identifier))
        field: (string))
      method: (identifier))
    arguments: (arguments)))

If you're using the generated tree-sitter ast to do refactoring or some complex stuff in a script, these extra information will surely help.

Expression vs Statement

The old parser doesn't abide by some rules of lua about what is an expression and what is a statement and where can those go. For example:

Old parser:

42

---

(program
  (expression (number)))

Would not show any error and accept a whole line that's an expression.

This parser:

42

---

(chunk
  (ERROR (number)))

Would show error, because this is not valid Lua.

Tests

This one has corpus tests for various edge cases and parses the whole luvit/luvit repository (more can be added) as part of testing.

[MunifTanjim]

indentation

I didn't get satisfactory result for indents queries. That's why I didn't push it. Here's a related discussion:

• How exactly indents work? #1668

Here's the indents.scm file sitting on my disk:

[
  (function_definition)
  (variable_declaration)
  (field)
  (do_statement)
  (while_statement)
  (repeat_statement)
  (if_statement)
  (for_statement)
  (return_statement)
  (table_constructor)
  (arguments)
  (block)
] @indent

(comment) @ignore

[clason]

Here's the indents.scm file sitting on my disk:

yoink!

[clason]

One thing I worry about is that the query coverage seems to be a bit of a regression compared to what we had. It might be better to start from our queries and just adapt them where necessary?

[MunifTanjim]

One thing I worry about is that the query coverage seems to be a bit of a regression compared to what we had. It might be better to start from our queries and just adapt them where necessary?

I haven't checked the current queries in nvim-treesitter/nvim-treesitter/queries/lua for a while. I just checked and seems like only highlights.scm is lacking a bit. Pushing it in a few minutes.

[theHamsta]

@MunifTanjim should we Auto-Import the queries from your repo (curl during parser update CI) assuming that they're made for Neovim?

[theHamsta]

We should also check the queries in nvim-treesitter-textobjects before merging this

[theHamsta]

Test failure comes probably from hocon or something unrelated and can be ignored

[clason]

@MunifTanjim should we Auto-Import the queries from your repo (curl during parser update CI) assuming that they're made for Neovim?

That would include moving the queries from the separate plugin to the parser repo (which should be obsolete once we make the switch).

We should also check the queries in nvim-treesitter-textobjects before merging this

I would have made a PR after this gets merged.

Test failure comes probably from hocon or something unrelated and can be ignored

No, there are definitely Lua failures. It seems that indentation with this parser doesn't really work.

[MunifTanjim]

@MunifTanjim should we Auto-Import the queries from your repo (curl during parser update CI) assuming that they're made for Neovim?

If this PR is merged, there's no point in maintaining the MunifTanjim/nvim-treesitter-lua repo. I'll archive that repo in that case. So, nope.

[clason]

If this PR is merged, there's no point in maintaining the MunifTanjim/nvim-treesitter-lua repo. I'll archive that repo in that case. So, nope.

I think @theHamsta meant that you would maintain queries in the parser repo, and we would pull them from there. Other parsers do similar things. This way, you could make improvements without needing to PR to this repo.

[MunifTanjim]

I think @theHamsta meant that you would maintain queries in the parser repo, and we would pull them from there. Other parsers do similar things. This way, you could make improvements without needing to PR to this repo.

Ah, I see. I can do that too. But doesn't it get confusing for the contributors? 🤔 How do they know where to update the queries?

It seems that indentation with this parser doesn't really work.

Just curious, is it Lua in general? or this new parser? As far as I remember I couldn't get proper indentation with the current parser either. I'm just using whatever indentation comes with vim by default for Lua.

[clason]

Ah, I see. I can do that too. But doesn't it get confusing for the contributors? 🤔 How do they know where to update the queries?

We would tell them ;) (Similarly to how in Neovim, we tell people to report/fix bugs with legacy syntax files in Vim.) But it's not necessary; whatever is easiest to maintain.

Just curious, is it Lua in general? or this new parser? As far as I remember I couldn't get proper indentation with the current parser either. I'm just using whatever indentation comes with vim by default for Lua.

I was under the impression that it was the new parser since the indentation tests are not marked as known failures. But I have to admit I did not check myself.

[theHamsta]

Tests are failing due to Markdown. Sorry, for pushing this to master. Indent tests for Lua might also fail when the new parser is better than the old, then the expected failure need to be updated.

[clason]

The highlight test for Lua is fixed now (thanks, @MunifTanjim). The expected indentation looks like correct indentation, though, while the found indentation is wrong? I also checked master, and the failures are not there. So this looks like a definite regression.

[MunifTanjim]

We would tell them ;) (Similarly to how in Neovim, we tell people to report/fix bugs with legacy syntax files in Vim.) But it's not necessary; whatever is easiest to maintain.

I think these queries are pretty stable and won't change that much in future. So I'd prefer archiving the MunifTanjim/nvim-treesitter-lua repo.

[clason]

We would tell them ;) (Similarly to how in Neovim, we tell people to report/fix bugs with legacy syntax files in Vim.) But it's not necessary; whatever is easiest to maintain.

I think these queries are pretty stable and won't change that much in future. So I'd prefer archiving the MunifTanjim/nvim-treesitter-lua repo.

OK, let's keep them here then. PRs for improving them are still appreciated :)

[clason]

@vigoux (and others) I think this is getting to the point where it'd be good if people familiar with the old parser would test-drive it for a bit and see if there are any noticeable regressions.

[vigoux]

A big green tick on this one, thanks!

[clason]

If @vigoux says it's good, it's good :)

Do we need some sort of "breaking changes" issue where we mention the switch?

[vigoux]

Hmm... I think that the language support issue could do it?

[clason]

No, that's for something different. I'm talking about something like the "Following HEAD" issue on Neovim where breaking changes are mentioned and migration paths are explained.

Like "new parser, please make sure to update queries"

[theHamsta]

Yes, would be good for people to subscribe. Lanugage support would mix-up feature requests with breaking change notification.

I suposse we need to fix also nvim-treesitter-textobjects for this one (or remove the Lua queries as a quickfix.

[theHamsta]

Great work! Also the syntax tree is now tidied up also for Lua

[clason]

Yes, would be good for people to subscribe. Lanugage support would mix-up feature requests with breaking change notification.

Agreed; I'll open up such an issue and pin it.

I suposse we need to fix also nvim-treesitter-textobjects for this one (or remove the Lua queries as a quickfix.

Yep, that was my next todo; luckily, @MunifTanjim's repo already contains replacement queries.

(This leaves nvim-treesitter-textsubjects, but that's third-party.)