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

refactor vim indent plugin to support Vim9 script #11079

Closed
wants to merge 169 commits into from

Conversation

lacygoill
Copy link

This is an attempt at improving the indentation for Vim9 script, and fix the issues mentioned in #11023

It should not be merged as it is; it still needs to be worked on. But it's a start.

I don't know how well it works for legacy Vim scripts.


One thing which would help is for Vim to introduce a v:indent dictionary variable in which we could get and save some values. It would only be set while an indentation is being performed. It could give us a few useful information (e.g. shiftwidth(), has('syntax_items')). But more importantly, it would let us remember some previous state. For example, if we find a heredoc on a given line, we would do:

v:indent.heredoc = {
    lnum: v:lnum,
    startindent: indent(v:lnum),
    endmarker: getline(v:lnum)->matchstr('some regex'),
}

Then, on subsequent lines, we could inspect the value of v:indent.heredoc to know whether we're in a heredoc, and if so how to indent.

Right now, I do it with an ad-hoc buffer-local variable: b:vimindent_heredoc. But I need to delete it, which is not easy. If an error is given during an indentation (or C-c is pressed), then the deletion might fail.

The alternative is to inspect the syntax, which is not reliable, because it depends on the syntax being enabled and being able to parse all heredocs (which is tricky; I have yet to find the right incantation of syntax sync commands to achieve that).

@brammool
Copy link
Contributor

I would appreciate some people trying this out and giving feedback.

I'm not sure if I can include a script where I only know the author's github user name.
If you don't want to reveal your real name to everybody, please send me a message in private.

@lacygoill
Copy link
Author

I would appreciate some people trying this out and giving feedback.

Yes, some feedback is needed.

I'm not sure if I can include a script where I only know the author's github user name.
If you don't want to reveal your real name to everybody, please send me a message in private.

Really sorry but I can't do that. FWIW, I didn't copy any part of the code from anywhere (stackoverflow, another GitHub repo, ...).

runtime/doc/indent.txt Outdated Show resolved Hide resolved
@brammool
Copy link
Contributor

Let me include this now. We can further improve it when more people try it out.

@brammool brammool closed this Sep 27, 2022
@brammool
Copy link
Contributor

Turns out the blockedit test fails. This Vim code is not always indented properly:

var d = {
a: (): asdf => 0,
b: (): asdf => 0,
c: (): asdf => 0,
}

Strangely, using "==" on the first line in the dictionary the indent doesn't change.
But when indenting the whole dictionary then it works fine. Perhaps it does not find the dictionary start when looking back?

@lacygoill
Copy link
Author

Strangely, using "==" on the first line in the dictionary the indent doesn't change.

When the script finds the start of a dictionary, it caches some info in a temporary buffer-local variable. It relies on this cache to properly indent the lines inside the dictionary. Making sure that == works even without the cache might be possible, but also adds more code/complexity.

This Vim code is not always indented properly:

Yes, the GitHub tests have been failing since some patch I pushed, but I was not able to reproduce on my machine.


I've also just noticed that the indentation is wrong when I indent my vimrc with the merged script. But it's fine with my local copy. Not sure why.

Anyway, now that the indent script has been merged, I'll look into all these issues, and submit a PR to fix them if I can. If I can't fix them, I guess we can still revert the script to the old version.

@lacygoill
Copy link
Author

I've also just noticed that the indentation is wrong when I indent my vimrc with the merged script. But it's fine with my local copy. Not sure why.

Actually, there is no issue. It's just that the legacy syntax plugin does not handle #{{{ correctly. My fork does.


Turns out the blockedit test fails.

I think I understand what happens. The old script was able to re-indent the first line of the dictionary. Not the new one. The new one needs to indent the whole dict. But that has nothing to do with the test. The test is there to make sure that the fix for issue #9229 does not break. The latter issue is not about indentation. It's about the text which is inserted during a blockedit being wrong. Before the fix, we had this buffer:

var d = {
a: () => 0,
b: () => 0,
c: () => 0,
}

Which became this after a blockedit:

var d = {
        a: (): number => 0,
b: ()   a: (): number => 0,
c: ()   a: (): number => 0,
}

That was completely wrong. There was no reason for a:(): to be inserted on the b: and c: lines. This has been fixed, and the new indent plugin does not break the fix.

In fact, I prefer the new behavior. Why should the first line of the block be the only one to be indented, and not the others? Maybe as a reminder that the rest of the block needs to be re-indented too?

If necessary, I could try to improve the script so that == works on the first line in a block. Is it worth it?

@lacygoill
Copy link
Author

If necessary, I could try to improve the script so that == works on the first line in a block. Is it worth it?

Actually, I already fixed that issue. But it can still be reproduced with the legacy syntax plugin; not with my fork. I'm going to look for a fix which also works with the legacy syntax plugin.

@habamax
Copy link
Contributor

habamax commented Sep 27, 2022

This function is indented a bit strange
asciicast

export def Buffer()
    var buffer_list = getbufinfo({'buflisted': 1})->mapnew((_, v) => {
        return {bufnr: v.bufnr,
                text: (v.name ?? $'[{v.bufnr}: No Name]'),
                lastused: v.lastused}
    })->sort((i, j) => i.lastused > j.lastused ? -1 : i.lastused == j.lastused ? 0 : 1)
    # Alternate buffer first, current buffer second
    if buffer_list->len() > 1 && buffer_list[0].bufnr == bufnr()
        [buffer_list[0], buffer_list[1]] = [buffer_list[1], buffer_list[0]]
    endif
    popup.FilterMenu("Buffers", buffer_list,
        (res, key) => {
            if key == "\<c-t>"
                exe $":tab sb {res.bufnr}"
            elseif key == "\<c-j>"
                exe $":sb {res.bufnr}"
            elseif key == "\<c-v>"
                exe $":vert sb {res.bufnr}"
            else
                exe $":b {res.bufnr}"
            endif
        },
        (winid) => {
            win_execute(winid, "syn match FilterMenuDirectorySubtle '^.*[\/]'")
            hi def link FilterMenuDirectorySubtle Comment
        })
enddef

@brammool
Copy link
Contributor

brammool commented Sep 27, 2022 via email

@brammool
Copy link
Contributor

brammool commented Sep 27, 2022 via email

@brammool
Copy link
Contributor

brammool commented Sep 27, 2022 via email

@lacygoill
Copy link
Author

Do you mean that "(winid)" is not indented?

Yes, this is a bug. I have a fix, but I want to make sure the code could not be simplified further before submitting any patch. Nested blocks are the most difficult constructs to indent.

That it doesn't re-indent those lines might be inconsistent, but that's what
was happening before.

Yes, but this behavior was not meant to be tested. Or at least, it was not the main purpose. And I doubt anyone had the time to get accustomed to that behavior considering that Vim9 is only a few months old, and that the indentation did not handle other constructs.

Yes, definitely.

OK, but the current test is wrong anyway. We can't write :var in a legacy script. The test forgot to put vim9script at the top of the buffer. And if we add vim9script, the issue disappears; the indentation is correct.

@lacygoill
Copy link
Author

And if we add vim9script, the issue disappears; the indentation is correct.

Although, for some reason, adding vim9script fixes ==, but not a block edit. I wonder what's the difference between the 2. I need more time to make more tests.

@brammool
Copy link
Contributor

brammool commented Sep 27, 2022 via email

@lacygoill
Copy link
Author

This function is indented a bit strange

Should be fixed by this patch:

From bdad71451084e8af78fd6734419d2f8de8e7ade5 Mon Sep 17 00:00:00 2001
From: lacygoill <lacygoill@lacygoill.me>
Date: Wed, 28 Sep 2022 00:08:43 +0200
Subject: [PATCH] fix: nested curly block

---
 runtime/autoload/dist/vimindent.vim | 38 ++++++++++++++---------------
 runtime/indent/testdir/vim.in       | 12 +++++++++
 runtime/indent/testdir/vim.ok       | 12 +++++++++
 3 files changed, 43 insertions(+), 19 deletions(-)

diff --git a/runtime/autoload/dist/vimindent.vim b/runtime/autoload/dist/vimindent.vim
index 572fc7c6c..00977dc3d 100644
--- a/runtime/autoload/dist/vimindent.vim
+++ b/runtime/autoload/dist/vimindent.vim
@@ -4,6 +4,11 @@ vim9script
 # Maintainer:   github user lacygoill
 # Last Change:  2022 Sep 24
 
+# NOTE: Whenever you change the code, make sure the tests are still passing:
+#
+#     $ cd runtime/indent/
+#     $ make clean; make test || vimdiff testdir/vim.{fail,ok}
+
 # Config {{{1
 
 const TIMEOUT: number = get(g:, 'vim_indent', {})
@@ -293,7 +298,7 @@ const START_MIDDLE_END: dict<list<string>> = {
 # EOL {{{2
 # OPENING_BRACKET_AT_EOL {{{3
 
-const OPENING_BRACKET_AT_EOL: string = $'{OPENING_BRACKET}{END_OF_VIM9_LINE}'
+const OPENING_BRACKET_AT_EOL: string = OPENING_BRACKET .. END_OF_VIM9_LINE
 
 # COMMA_AT_EOL {{{3
 
@@ -404,16 +409,15 @@ export def Expr(lnum: number): number # {{{2
         line_A->CacheBracketBlock()
     endif
     if line_A.lnum->IsInside('BracketBlock')
-            && !b:vimindent.block_stack[0].is_curly_block
         for block: dict<any> in b:vimindent.block_stack
-            # Can't call `BracketBlockIndent()` before we're indenting a line *after* the start of the block.{{{
-            #
-            # That's because it might need  the correct indentation of the start
-            # of the block.   But if we're still *on* the  start, we haven't yet
-            # computed that indentation.
-            #}}}
-            if line_A.lnum > block.startlnum
-                    && !block.is_curly_block
+            if line_A.lnum <= block.startlnum
+                continue
+            endif
+            if !block->has_key('startindent')
+                block.startindent = Indent(block.startlnum)
+            endif
+            if !block.is_curly_block
+                    && !b:vimindent.block_stack[0].is_curly_block
                 return BracketBlockIndent(line_A, block)
             endif
         endfor
@@ -481,7 +485,7 @@ export def Expr(lnum: number): number # {{{2
         cursor(line_A.lnum, 1)
 
         var [start: string, middle: string, end: string] = START_MIDDLE_END[kwd]
-        var block_start = SearchPairStart(start, middle, end)
+        var block_start: number = SearchPairStart(start, middle, end)
         if block_start > 0
             return Indent(block_start)
         else
@@ -535,10 +539,10 @@ def Offset( # {{{2
         # Indent twice for  a line continuation in the block  header itself, so that
         # we can easily  distinguish the end of  the block header from  the start of
         # the block body.
-        elseif line_B->EndsWithLineContinuation()
-                && !line_A.isfirst
-                || line_A.text =~ LINE_CONTINUATION_AT_SOL
-                && line_A.text !~ PLUS_MINUS_COMMAND
+        elseif (line_B->EndsWithLineContinuation()
+                && !line_A.isfirst)
+                || (line_A.text =~ LINE_CONTINUATION_AT_SOL
+                && line_A.text !~ PLUS_MINUS_COMMAND)
                 || line_A.text->Is_IN_KeywordForLoop(line_B.text)
             return 2 * shiftwidth()
         else
@@ -646,10 +650,6 @@ def CommentIndent(): number # {{{2
 enddef
 
 def BracketBlockIndent(line_A: dict<any>, block: dict<any>): number # {{{2
-    if !block->has_key('startindent')
-        block.startindent = block.startlnum->Indent()
-    endif
-
     var ind: number = block.startindent
 
     if line_A.text =~ CLOSING_BRACKET_AT_SOL
diff --git a/runtime/indent/testdir/vim.in b/runtime/indent/testdir/vim.in
index 0582a930e..87c044a9b 100644
--- a/runtime/indent/testdir/vim.in
+++ b/runtime/indent/testdir/vim.in
@@ -857,3 +857,15 @@ b: 2}]
 silent! argdel *
 edit file
 " END_INDENT
+
+" START_INDENT
+def Foo()
+Bar(1,
+[]->filter((_, v) => {
+return true
+}),
+() => {
+echo
+})
+enddef
+" END_INDENT
diff --git a/runtime/indent/testdir/vim.ok b/runtime/indent/testdir/vim.ok
index 39efdbaac..2326934f9 100644
--- a/runtime/indent/testdir/vim.ok
+++ b/runtime/indent/testdir/vim.ok
@@ -857,3 +857,15 @@ endfor
 silent! argdel *
 edit file
 " END_INDENT
+
+" START_INDENT
+def Foo()
+    Bar(1,
+	[]->filter((_, v) => {
+	    return true
+	}),
+	() => {
+	    echo
+	})
+enddef
+" END_INDENT
-- 
2.25.1

@brammool
Copy link
Contributor

brammool commented Sep 28, 2022 via email

@dkearns
Copy link
Contributor

dkearns commented Sep 28, 2022

Perhaps trailing operators are not considered idiomatic Vim9 script but I habitually use them and noticed the following issue - "three" is misaligned for var x.

vim9script
var x = "one" ..
  "two" ..
"three"
var y = "one"
  .. "two"
  .. "three"

@lacygoill
Copy link
Author

This is yet again an issue with the legacy syntax plugin which highlights "two" and "three" as comments. There is no issue with my fork.

Not sure it can be improved, but I'll try.

@dkearns
Copy link
Contributor

dkearns commented Sep 28, 2022

Sorry, I've become temporarily blinded to having those strings highlighted as comments. I agree that it should be fixed in the syntax file.

It was on my TODO list to send Charles a patch if I could come up with one but never got to it.

@brammool
Copy link
Contributor

brammool commented Oct 11, 2022 via email

@dkearns
Copy link
Contributor

dkearns commented Oct 13, 2022

Unfortunately, that seems to cause more problems in legacy script than it fixes in Vim9 script.

I posted another quick fix in #11307

We can probably fix these comment bugs to a satisfactory level in the current syntax file without too much trouble. However, after having a quick look at @lacygoill's typically thorough work on the dedicated Vim9 syntax file I think we should at least consider distributing that alongside a simplified version of the existing file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants