search

nushell / tree-sitter-nu

A tree-sitter grammar for nu-lang, the language of nushell
MIT License
122 stars 27 forks source link

Manually control all newline characters #139

Closed blindFS closed 1 week ago

blindFS commented 2 weeks ago

While attempting to fix #122 , I found the ambiguity of newline characters is a big obstacle for multiline expressions to be parsed accurately.

Current grammar put newline characters in extras (\s), allowing arbitrary numbers of them to be present freely between any nodes. While some rules specifically announced newline characters, making them selected with higher priorities when a newline is matched in the exact sequence.

This makes adding the rule for expr_binary_parenthesized extremely difficult since newline characters breaks the precedence order.

This PR did quite a lot changes:

  1. remove newline from extras
  2. manually add newline to related rules
  3. several existing test cases are broken, but the new results seem to be more accurate, so I changed the target results
    • most of them is about the position of comments
    • val_record as register signature
  4. more multiline related test cases added

⚠️ There are probably new errors introduced by this PR! Please take time to review it with caution. I'll continue to test it on more nushell scripts to make it more robust.

If you guys have better solutions, I'm more than happy to switch.

fdncred commented 2 weeks ago

I really don't have a problem moving forward with this because even if it breaks something, you've been super helpful to fix those type of things. If this is going to make things better in the long run, a little temporary instability is a small price to pay.

blindFS commented 2 weeks ago

@fdncred I think this is helpful to decouple intertwined rules and enable finer grained control of the grammar, on the other side, it forces us to write more intricate rules when newline characters are allowed in them.

Let's wait to see whether other people @CabalCrow @mrdgo have different opinions on this, during the on-going test on a broader range of scripts.

CabalCrow commented 2 weeks ago

Back when I was attempting a rewrite, I was trying to put the newlines in scanner due to the same issue. There were plenty of problem with comments and all other types of syntax that is allowed by nushell.

Do note that the bash tree sitter also had some of the issue, so maybe this could be a limitation of what you can do with tree-sitter without relying on the scanner.

blindFS commented 2 weeks ago

@CabalCrow Yeah, external scanner was my initial thought and probably is the ultimate solution. Yet it turns out that most of known errors can still be solved without a scanner, comments are definitely on the list of attention here. I feel that keeping it all js for now helps to recognize the parts that really requires a scanner, for example the unquoted string rule already looks insanely complicated.

mrdgo commented 2 weeks ago

Thanks for the PR and the request for review.

I honestly feel some pain looking at all the places where you now explicitly allow newlines. On the other hand, it's a great advantage to solve it in js. I'll take a closer look tomorrow.

blindFS commented 2 weeks ago

I think it is ready for review. I have tested it on all nu scripts in nushell/nu_scripts

Either it is parsed with no error (probably wrong answer though) or the error is rooted somewhere else (collected a dozen of new issues :-( ).

I left some parenthesized rules unchanged by intention, technically newlines should be allowed in them, but I think nobody writes nushell in a crazy way like this:

(
let
foo
=
1
)

The CI action pipeline has permission issues at the time.

blindFS commented 2 weeks ago

Github action file modified to git rid of permission error in the last commit, not sure it actually overrides the npm as originally intended. @fdncred

fdncred commented 2 weeks ago

I'm fine with landing it and seeing how it goes but we could also wait for @mrdgo and @CabalCrow to see if they have time to review since this is such a fundamental change?

mrdgo commented 1 week ago

I am absolutely fine with this!

As soon as we have a scanner.c, someone could try to implement newline support in there and compare. But I don't really see any show-stopper, here.

blindFS commented 1 week ago

I'm fine with landing it and seeing how it goes but we could also wait for @mrdgo and @CabalCrow to see if they have time to review since this is such a fundamental change?

It's up to you. The test went well, I feel confident for this now. The main concern is whether the change will make other contributors feel uncomfortable.

fdncred commented 1 week ago

Thanks. Let's move forward and see how it goes. Appreciate all the work here.