Re: master 63d69bd1549 07/11: Use new baseline indent rule in c-ts-mode

Re: master 63d69bd1549 07/11: Use new baseline indent rule in c-ts-mode

[Andrea Corallo]

Yuan Fu <casouri@gmail.com> writes:

> branch: master
> commit 63d69bd154987bcc0434e0f85e09bf5dfa07b827
> Author: Yuan Fu <casouri@gmail.com>
> Commit: Yuan Fu <casouri@gmail.com>
>
>     Use new baseline indent rule in c-ts-mode
>     
>     Rework the indent rules in c-ts-mode using
>     c-ts-common-baseline-indent-rule.  Also reworked internal
>     functions for setting indent styles to make it simpler and (more
>     importantly) more readable.  Now there's a single function,
>     c-ts-mode--simple-indent-rules, that returns a value that can go
>     straight to treesit-simple-indent-rules.
>     
>     * lisp/progmodes/c-ts-mode.el:
>     (c-ts-mode--indent-style-setter): Simplify.
>     (c-ts-mode--get-indent-style): Remove function.
>     (c-ts-mode--prompt-for-style): Use a hard-coded list of styles.
>     This is a tiny downgrade from the computed lists but is
>     acceptable.
>     (c-ts-mode-set-style): Use c-ts-mode--simple-indent-rules.
>     (c-ts-mode--standalone-grandparent): Docstring change.
>     (c-ts-mode--for-loop-indent-rule): New function.
>     (c-ts-mode--preproc-indent-rules): New variable.  Copied from
>     old indent rules and added some new rule.
>     (c-ts-mode--macro-heuristic-rules):
>     (c-ts-mode--simple-indent-rules):
>     (c-ts-mode--parenthesized-expression-indent-rule):
>     (c-ts-mode--label-indent-rules): New functions.
>     (c-ts-mode):
>     (c++-ts-mode): Use c-ts-mode--simple-indent-rules.
> ---

Hi Yuan,

this commit on my test-bench seems to introduce on master the following
two failures: c-ts-mode-test-indentation-bsd,
c-ts-mode-test-indentation.

Could you have a look?

  Andrea

Re: master 63d69bd1549 07/11: Use new baseline indent rule in c-ts-mode

[Yuan Fu]

> On Dec 2, 2024, at 1:02 PM, Andrea Corallo <acorallo@gnu.org> wrote:
> 
> Yuan Fu <casouri@gmail.com> writes:
> 
>> branch: master
>> commit 63d69bd154987bcc0434e0f85e09bf5dfa07b827
>> Author: Yuan Fu <casouri@gmail.com>
>> Commit: Yuan Fu <casouri@gmail.com>
>> 
>>    Use new baseline indent rule in c-ts-mode
>> 
>>    Rework the indent rules in c-ts-mode using
>>    c-ts-common-baseline-indent-rule.  Also reworked internal
>>    functions for setting indent styles to make it simpler and (more
>>    importantly) more readable.  Now there's a single function,
>>    c-ts-mode--simple-indent-rules, that returns a value that can go
>>    straight to treesit-simple-indent-rules.
>> 
>>    * lisp/progmodes/c-ts-mode.el:
>>    (c-ts-mode--indent-style-setter): Simplify.
>>    (c-ts-mode--get-indent-style): Remove function.
>>    (c-ts-mode--prompt-for-style): Use a hard-coded list of styles.
>>    This is a tiny downgrade from the computed lists but is
>>    acceptable.
>>    (c-ts-mode-set-style): Use c-ts-mode--simple-indent-rules.
>>    (c-ts-mode--standalone-grandparent): Docstring change.
>>    (c-ts-mode--for-loop-indent-rule): New function.
>>    (c-ts-mode--preproc-indent-rules): New variable.  Copied from
>>    old indent rules and added some new rule.
>>    (c-ts-mode--macro-heuristic-rules):
>>    (c-ts-mode--simple-indent-rules):
>>    (c-ts-mode--parenthesized-expression-indent-rule):
>>    (c-ts-mode--label-indent-rules): New functions.
>>    (c-ts-mode):
>>    (c++-ts-mode): Use c-ts-mode--simple-indent-rules.
>> ---
> 
> Hi Yuan,
> 
> this commit on my test-bench seems to introduce on master the following
> two failures: c-ts-mode-test-indentation-bsd,
> c-ts-mode-test-indentation.
> 
> Could you have a look?
> 
>  Andrea

Hey Andrea, sorry about that. The tests do pass on my machine. How does the test-bench run the tests?

Yuan

Re: master 63d69bd1549 07/11: Use new baseline indent rule in c-ts-mode

[Andrea Corallo]

Yuan Fu <casouri@gmail.com> writes:

>> On Dec 2, 2024, at 1:02 PM, Andrea Corallo <acorallo@gnu.org> wrote:
>>
>> Yuan Fu <casouri@gmail.com> writes:
>>
>>> branch: master
>>> commit 63d69bd154987bcc0434e0f85e09bf5dfa07b827
>>> Author: Yuan Fu <casouri@gmail.com>
>>> Commit: Yuan Fu <casouri@gmail.com>
>>>
>>>    Use new baseline indent rule in c-ts-mode
>>>
>>>    Rework the indent rules in c-ts-mode using
>>>    c-ts-common-baseline-indent-rule.  Also reworked internal
>>>    functions for setting indent styles to make it simpler and (more
>>>    importantly) more readable.  Now there's a single function,
>>>    c-ts-mode--simple-indent-rules, that returns a value that can go
>>>    straight to treesit-simple-indent-rules.
>>>
>>>    * lisp/progmodes/c-ts-mode.el:
>>>    (c-ts-mode--indent-style-setter): Simplify.
>>>    (c-ts-mode--get-indent-style): Remove function.
>>>    (c-ts-mode--prompt-for-style): Use a hard-coded list of styles.
>>>    This is a tiny downgrade from the computed lists but is
>>>    acceptable.
>>>    (c-ts-mode-set-style): Use c-ts-mode--simple-indent-rules.
>>>    (c-ts-mode--standalone-grandparent): Docstring change.
>>>    (c-ts-mode--for-loop-indent-rule): New function.
>>>    (c-ts-mode--preproc-indent-rules): New variable.  Copied from
>>>    old indent rules and added some new rule.
>>>    (c-ts-mode--macro-heuristic-rules):
>>>    (c-ts-mode--simple-indent-rules):
>>>    (c-ts-mode--parenthesized-expression-indent-rule):
>>>    (c-ts-mode--label-indent-rules): New functions.
>>>    (c-ts-mode):
>>>    (c++-ts-mode): Use c-ts-mode--simple-indent-rules.
>>> ---
>>
>> Hi Yuan,
>>
>> this commit on my test-bench seems to introduce on master the following
>> two failures: c-ts-mode-test-indentation-bsd,
>> c-ts-mode-test-indentation.
>>
>> Could you have a look?
>>
>>  Andrea
>
> Hey Andrea, sorry about that. The tests do pass on my machine. How does the test-bench run the tests?
>
> Yuan

Seems 44fcd37a486399be048fb03b9456497af78632fe fixed
c-ts-mode-test-indentation-bsd.  I still see:

Running 4 tests (2024-12-03 03:51:05+0100, selector `(not (or (tag :expensive-test) (tag :unstable) (tag :nativecomp)))')
   passed  1/4  c-ts-mode-test-filling (0.033575 sec)
Test c-ts-mode-test-indentation backtrace:
  signal(ert-test-failed (("Mismatch in test \"For Loop with Multi-lin
  ert-fail(("Mismatch in test \"For Loop with Multi-line Condition (GN
  ert-test--erts-test(((code lambda nil (c-ts-mode) (setq-local indent
  ert-test-erts-file("/home/andcor03/emacs-ci/emacs-ci-repo1/test/lisp
  #f(compiled-function () #<bytecode 0x165c3552b49c3535>)()
  #f(compiled-function () #<bytecode -0x1132e651e2fb7509>)()
  handler-bind-1(#f(compiled-function () #<bytecode -0x1132e651e2fb750
  ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
  ert-run-test(#s(ert-test :name c-ts-mode-test-indentation :documenta
  ert-run-or-rerun-test(#s(ert--stats :selector (not (or ... ... ...))
  ert-run-tests((not (or (tag :expensive-test) (tag :unstable) (tag :n
  ert-run-tests-batch((not (or (tag :expensive-test) (tag :unstable) (
  ert-run-tests-batch-and-exit((not (or (tag :expensive-test) (tag :un
  eval((ert-run-tests-batch-and-exit '(not (or (tag :expensive-test) (
  command-line-1(("-L" ":." "-l" "ert" "--eval" "(setq treesit-extra-l
  command-line()
  normal-top-level()
Test c-ts-mode-test-indentation condition:
    (ert-test-failed
     ("Mismatch in test \"For Loop with Multi-line Condition (GNU Style)\", file /home/andcor03/emacs-ci/emacs-ci-repo1/test/lisp/progmodes/c-ts-mode-resources/indent.erts"
      #("int main()\n{\n  for (\n       int i = 0;\n       i < b;\n       i++\n      )\n      {\n        return 0;\n      }\n}\n"
        0 108 (fontified nil))
      "int main()\n{\n  for (\n       int i = 0;\n       i < b;\n       i++\n      )\n    {\n      return 0;\n    }\n}\n"))
   FAILED  2/4  c-ts-mode-test-indentation (0.016936 sec) at lisp/progmodes/c-ts-mode-tests.el:26


Configuring with --with-native-compilation=no --with-x-toolkit=no

Regards

  Andrea

Re: master 44fcd37a486 08/11: Add more c-ts-mode indent tests

[Michael Albinus]

Yuan Fu <casouri@gmail.com> writes:

Hi Yuan,

> branch: master
> commit 44fcd37a486399be048fb03b9456497af78632fe
> Author: Yuan Fu <casouri@gmail.com>
> Commit: Yuan Fu <casouri@gmail.com>
>
>     Add more c-ts-mode indent tests

This fails on emba:

--8<---------------cut here---------------start------------->8---
Running 4 tests (2024-12-04 01:34:21+0000, selector `(and (not (or (tag :unstable) (tag :nativecomp))) (or "^treesit" "-ts-"))')
   passed  1/4  c-ts-mode-test-filling (0.242501 sec)
Test c-ts-mode-test-indentation backtrace:
  signal(ert-test-failed (("Mismatch in test \"For Loop with Multi-lin
  ert-fail(("Mismatch in test \"For Loop with Multi-line Condition (GN
  ert-test--erts-test(((code lambda nil (c-ts-mode) (setq-local indent
  ert-test-erts-file("/checkout/test/lisp/progmodes/c-ts-mode-resource
  #f(compiled-function () #<bytecode 0x165c4c7bd62c9ac5>)()
  #f(compiled-function () #<bytecode -0x13aaedb12a291e2f>)()
  handler-bind-1(#f(compiled-function () #<bytecode -0x13aaedb12a291e2
  ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
  ert-run-test(#s(ert-test :name c-ts-mode-test-indentation :documenta
  ert-run-or-rerun-test(#s(ert--stats :selector (and (not ...) (or "^t
  ert-run-tests((and (not (or (tag :unstable) (tag :nativecomp))) (or
  ert-run-tests-batch((and (not (or (tag :unstable) (tag :nativecomp))
  ert-run-tests-batch-and-exit((and (not (or (tag :unstable) (tag :nat
  eval((ert-run-tests-batch-and-exit '(and (not (or (tag :unstable) (t
  command-line-1(("-L" ":." "-l" "ert" "--eval" "(setq treesit-extra-l
  command-line()
  normal-top-level()
Test c-ts-mode-test-indentation condition:
    (ert-test-failed
     ("Mismatch in test \"For Loop with Multi-line Condition (GNU Style)\", file /checkout/test/lisp/progmodes/c-ts-mode-resources/indent.erts"
      #("int main()\n{\n  for (\n       int i = 0;\n       i < b;\n       i++\n      )\n      {\n        return 0;\n      }\n}\n"
	0 108 (fontified nil))
      "int main()\n{\n  for (\n       int i = 0;\n       i < b;\n       i++\n      )\n    {\n      return 0;\n    }\n}\n"))
   FAILED  2/4  c-ts-mode-test-indentation (0.132145 sec) at lisp/progmodes/c-ts-mode-tests.el:26
   passed  3/4  c-ts-mode-test-indentation-bsd (0.087755 sec)
   passed  4/4  c-ts-mode-test-indentation-preproc (0.049779 sec)

Ran 4 tests, 3 results as expected, 1 unexpected (2024-12-04 01:34:23+0000, 1.849321 sec)
--8<---------------cut here---------------end--------------->8---

See <https://emba.gnu.org/emacs/emacs/-/jobs/93972/raw>.

Best regards, Michael.

Re: master 44fcd37a486 08/11: Add more c-ts-mode indent tests

[Yuan Fu]

> On Dec 4, 2024, at 12:19 AM, Michael Albinus <michael.albinus@gmx.de> wrote:
> 
> Yuan Fu <casouri@gmail.com> writes:
> 
> Hi Yuan,
> 
>> branch: master
>> commit 44fcd37a486399be048fb03b9456497af78632fe
>> Author: Yuan Fu <casouri@gmail.com>
>> Commit: Yuan Fu <casouri@gmail.com>
>> 
>>    Add more c-ts-mode indent tests
> 
> This fails on emba:
> 
> --8<---------------cut here---------------start------------->8---
> Running 4 tests (2024-12-04 01:34:21+0000, selector `(and (not (or (tag :unstable) (tag :nativecomp))) (or "^treesit" "-ts-"))')
>   passed  1/4  c-ts-mode-test-filling (0.242501 sec)
> Test c-ts-mode-test-indentation backtrace:
>  signal(ert-test-failed (("Mismatch in test \"For Loop with Multi-lin
>  ert-fail(("Mismatch in test \"For Loop with Multi-line Condition (GN
>  ert-test--erts-test(((code lambda nil (c-ts-mode) (setq-local indent
>  ert-test-erts-file("/checkout/test/lisp/progmodes/c-ts-mode-resource
>  #f(compiled-function () #<bytecode 0x165c4c7bd62c9ac5>)()
>  #f(compiled-function () #<bytecode -0x13aaedb12a291e2f>)()
>  handler-bind-1(#f(compiled-function () #<bytecode -0x13aaedb12a291e2
>  ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
>  ert-run-test(#s(ert-test :name c-ts-mode-test-indentation :documenta
>  ert-run-or-rerun-test(#s(ert--stats :selector (and (not ...) (or "^t
>  ert-run-tests((and (not (or (tag :unstable) (tag :nativecomp))) (or
>  ert-run-tests-batch((and (not (or (tag :unstable) (tag :nativecomp))
>  ert-run-tests-batch-and-exit((and (not (or (tag :unstable) (tag :nat
>  eval((ert-run-tests-batch-and-exit '(and (not (or (tag :unstable) (t
>  command-line-1(("-L" ":." "-l" "ert" "--eval" "(setq treesit-extra-l
>  command-line()
>  normal-top-level()
> Test c-ts-mode-test-indentation condition:
>    (ert-test-failed
>     ("Mismatch in test \"For Loop with Multi-line Condition (GNU Style)\", file /checkout/test/lisp/progmodes/c-ts-mode-resources/indent.erts"
>      #("int main()\n{\n  for (\n       int i = 0;\n       i < b;\n       i++\n      )\n      {\n        return 0;\n      }\n}\n"
> 0 108 (fontified nil))
>      "int main()\n{\n  for (\n       int i = 0;\n       i < b;\n       i++\n      )\n    {\n      return 0;\n    }\n}\n"))
>   FAILED  2/4  c-ts-mode-test-indentation (0.132145 sec) at lisp/progmodes/c-ts-mode-tests.el:26
>   passed  3/4  c-ts-mode-test-indentation-bsd (0.087755 sec)
>   passed  4/4  c-ts-mode-test-indentation-preproc (0.049779 sec)
> 
> Ran 4 tests, 3 results as expected, 1 unexpected (2024-12-04 01:34:23+0000, 1.849321 sec)
> --8<---------------cut here---------------end--------------->8---
> 
> See <https://emba.gnu.org/emacs/emacs/-/jobs/93972/raw>.
> 
> Best regards, Michael.

This is baffling. I assume the pipeline build the latest tree-sitter-c/cpp grammar right? I just updated my grammar and tree-sitter lib to the latest and the tests still pass on my machine. Is tree-sitter version also the latest?

Yuan

Re: master 44fcd37a486 08/11: Add more c-ts-mode indent tests

[Michael Albinus]

Yuan Fu <casouri@gmail.com> writes:

Hi Yuan,

> This is baffling. I assume the pipeline build the latest
> tree-sitter-c/cpp grammar right? I just updated my grammar and
> tree-sitter lib to the latest and the tests still pass on my
> machine. Is tree-sitter version also the latest?

See test/infra/Dockerfile.emba. The emacs-tree-sitter container image is
created prior every run, the treesitter grammars mentioned there are
installed.

If you want to reproduce the problem in the emba environment, see
admin/notes/emba. It explains how to create and run the docker container
locally.

> Yuan

Best regards, Michael.

Re: master 44fcd37a486 08/11: Add more c-ts-mode indent tests

[Yuan Fu]

> On Dec 4, 2024, at 11:37 PM, Michael Albinus <michael.albinus@gmx.de> wrote:
> 
> Yuan Fu <casouri@gmail.com> writes:
> 
> Hi Yuan,
> 
>> This is baffling. I assume the pipeline build the latest
>> tree-sitter-c/cpp grammar right? I just updated my grammar and
>> tree-sitter lib to the latest and the tests still pass on my
>> machine. Is tree-sitter version also the latest?
> 
> See test/infra/Dockerfile.emba. The emacs-tree-sitter container image is
> created prior every run, the treesitter grammars mentioned there are
> installed.

Ah, so the image uses Debian bookworm, which AFAIK is the current stable release, so it probably doesn’t have the latest tree-sitter library. Let me try to run the pipeline with tree-sitter built from upstream.

Yuan

Re: master 44fcd37a486 08/11: Add more c-ts-mode indent tests

[Yuan Fu]

> On Dec 5, 2024, at 11:30 PM, Yuan Fu <casouri@gmail.com> wrote:
> 
> 
> 
>> On Dec 4, 2024, at 11:37 PM, Michael Albinus <michael.albinus@gmx.de> wrote:
>> 
>> Yuan Fu <casouri@gmail.com> writes:
>> 
>> Hi Yuan,
>> 
>>> This is baffling. I assume the pipeline build the latest
>>> tree-sitter-c/cpp grammar right? I just updated my grammar and
>>> tree-sitter lib to the latest and the tests still pass on my
>>> machine. Is tree-sitter version also the latest?
>> 
>> See test/infra/Dockerfile.emba. The emacs-tree-sitter container image is
>> created prior every run, the treesitter grammars mentioned there are
>> installed.
> 
> Ah, so the image uses Debian bookworm, which AFAIK is the current stable release, so it probably doesn’t have the latest tree-sitter library. Let me try to run the pipeline with tree-sitter built from upstream.
> 
> Yuan

I didn’t run the pipeline built from upstream, but I tried running the tests locally with the tree-sitter library used in the pipeline. The pipeline uses tree-sitter provided by bookworm, which is v0.20.7, a pretty old version. And indeed, with that version of tree-sitter I can see the test failure.

Andrea, which version of  tree-sitter do you have on your test bench?

Since tree-sitter is still relatively unstable, maybe we want to use upstream tree-sitter for the test suite?

Yuan

Re: master 44fcd37a486 08/11: Add more c-ts-mode indent tests

[Eli Zaretskii]

> From: Yuan Fu <casouri@gmail.com>
> Date: Fri, 6 Dec 2024 17:04:42 -0800
> Cc: Emacs Devel <emacs-devel@gnu.org>,
>  Andrea Corallo <acorallo@gnu.org>
> 
> 
> 
> > On Dec 5, 2024, at 11:30 PM, Yuan Fu <casouri@gmail.com> wrote:
> > 
> > 
> > 
> >> On Dec 4, 2024, at 11:37 PM, Michael Albinus <michael.albinus@gmx.de> wrote:
> >> 
> >> Yuan Fu <casouri@gmail.com> writes:
> >> 
> >> Hi Yuan,
> >> 
> >>> This is baffling. I assume the pipeline build the latest
> >>> tree-sitter-c/cpp grammar right? I just updated my grammar and
> >>> tree-sitter lib to the latest and the tests still pass on my
> >>> machine. Is tree-sitter version also the latest?
> >> 
> >> See test/infra/Dockerfile.emba. The emacs-tree-sitter container image is
> >> created prior every run, the treesitter grammars mentioned there are
> >> installed.
> > 
> > Ah, so the image uses Debian bookworm, which AFAIK is the current stable release, so it probably doesn’t have the latest tree-sitter library. Let me try to run the pipeline with tree-sitter built from upstream.
> > 
> > Yuan
> 
> I didn’t run the pipeline built from upstream, but I tried running the tests locally with the tree-sitter library used in the pipeline. The pipeline uses tree-sitter provided by bookworm, which is v0.20.7, a pretty old version. And indeed, with that version of tree-sitter I can see the test failure.
> 
> Andrea, which version of  tree-sitter do you have on your test bench?
> 
> Since tree-sitter is still relatively unstable, maybe we want to use upstream tree-sitter for the test suite?

I don't think this is practical.  I'd rather mark that test as
expected failure or "unstable", if we cannot know the version of the
tree-sitter library used for the test.

Or maybe you could rewrite the test so that it works in both old and
new versions of the library.

(FWIW, I use 0.20.8, with the latest grammar libraries, and the test
fails here as well.  Due to the issues with ABI breakage in the
tree-sitter library, I decided that I will upgrade only when 0.25 is
released, because it also bumps the language ABI version, which we can
detect in Emacs.)

Re: master 44fcd37a486 08/11: Add more c-ts-mode indent tests

[Yuan Fu]

> On Dec 6, 2024, at 11:49 PM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> From: Yuan Fu <casouri@gmail.com>
>> Date: Fri, 6 Dec 2024 17:04:42 -0800
>> Cc: Emacs Devel <emacs-devel@gnu.org>,
>> Andrea Corallo <acorallo@gnu.org>
>> 
>> 
>> 
>>> On Dec 5, 2024, at 11:30 PM, Yuan Fu <casouri@gmail.com> wrote:
>>> 
>>> 
>>> 
>>>> On Dec 4, 2024, at 11:37 PM, Michael Albinus <michael.albinus@gmx.de> wrote:
>>>> 
>>>> Yuan Fu <casouri@gmail.com> writes:
>>>> 
>>>> Hi Yuan,
>>>> 
>>>>> This is baffling. I assume the pipeline build the latest
>>>>> tree-sitter-c/cpp grammar right? I just updated my grammar and
>>>>> tree-sitter lib to the latest and the tests still pass on my
>>>>> machine. Is tree-sitter version also the latest?
>>>> 
>>>> See test/infra/Dockerfile.emba. The emacs-tree-sitter container image is
>>>> created prior every run, the treesitter grammars mentioned there are
>>>> installed.
>>> 
>>> Ah, so the image uses Debian bookworm, which AFAIK is the current stable release, so it probably doesn’t have the latest tree-sitter library. Let me try to run the pipeline with tree-sitter built from upstream.
>>> 
>>> Yuan
>> 
>> I didn’t run the pipeline built from upstream, but I tried running the tests locally with the tree-sitter library used in the pipeline. The pipeline uses tree-sitter provided by bookworm, which is v0.20.7, a pretty old version. And indeed, with that version of tree-sitter I can see the test failure.
>> 
>> Andrea, which version of  tree-sitter do you have on your test bench?
>> 
>> Since tree-sitter is still relatively unstable, maybe we want to use upstream tree-sitter for the test suite?
> 
> I don't think this is practical.  I'd rather mark that test as
> expected failure or "unstable", if we cannot know the version of the
> tree-sitter library used for the test.
> 
> Or maybe you could rewrite the test so that it works in both old and
> new versions of the library.
> 
> (FWIW, I use 0.20.8, with the latest grammar libraries, and the test
> fails here as well.  Due to the issues with ABI breakage in the
> tree-sitter library, I decided that I will upgrade only when 0.25 is
> released, because it also bumps the language ABI version, which we can
> detect in Emacs.)

I dug into it, and the culprit is a bug in older tree-sitter versions where (the C equivalent of) treesit-node-field-name-for-child gives the wrong field name. I rewrote the indent function so it uses child node index instead of field name to match indentation rules. Now the tests should pass.

Yuan

Re: master 44fcd37a486 08/11: Add more c-ts-mode indent tests

[Andrea Corallo]

Yuan Fu <casouri@gmail.com> writes:

>> On Dec 6, 2024, at 11:49 PM, Eli Zaretskii <eliz@gnu.org> wrote:
>> 
>>> From: Yuan Fu <casouri@gmail.com>
>>> Date: Fri, 6 Dec 2024 17:04:42 -0800
>>> Cc: Emacs Devel <emacs-devel@gnu.org>,
>>> Andrea Corallo <acorallo@gnu.org>
>>> 
>>> 
>>> 
>>>> On Dec 5, 2024, at 11:30 PM, Yuan Fu <casouri@gmail.com> wrote:
>>>> 
>>>> 
>>>> 
>>>>> On Dec 4, 2024, at 11:37 PM, Michael Albinus <michael.albinus@gmx.de> wrote:
>>>>> 
>>>>> Yuan Fu <casouri@gmail.com> writes:
>>>>> 
>>>>> Hi Yuan,
>>>>> 
>>>>>> This is baffling. I assume the pipeline build the latest
>>>>>> tree-sitter-c/cpp grammar right? I just updated my grammar and
>>>>>> tree-sitter lib to the latest and the tests still pass on my
>>>>>> machine. Is tree-sitter version also the latest?
>>>>> 
>>>>> See test/infra/Dockerfile.emba. The emacs-tree-sitter container image is
>>>>> created prior every run, the treesitter grammars mentioned there are
>>>>> installed.
>>>> 
>>>> Ah, so the image uses Debian bookworm, which AFAIK is the current
>>>> stable release, so it probably doesn’t have the latest tree-sitter
>>>> library. Let me try to run the pipeline with tree-sitter built
>>>> from upstream.
>>>> 
>>>> Yuan
>>> 
>>> I didn’t run the pipeline built from upstream, but I tried running
>>> the tests locally with the tree-sitter library used in the
>>> pipeline. The pipeline uses tree-sitter provided by bookworm, which
>>> is v0.20.7, a pretty old version. And indeed, with that version of
>>> tree-sitter I can see the test failure.
>>> 
>>> Andrea, which version of  tree-sitter do you have on your test bench?
>>> 
>>> Since tree-sitter is still relatively unstable, maybe we want to use upstream tree-sitter for the test suite?
>> 
>> I don't think this is practical.  I'd rather mark that test as
>> expected failure or "unstable", if we cannot know the version of the
>> tree-sitter library used for the test.
>> 
>> Or maybe you could rewrite the test so that it works in both old and
>> new versions of the library.
>> 
>> (FWIW, I use 0.20.8, with the latest grammar libraries, and the test
>> fails here as well.  Due to the issues with ABI breakage in the
>> tree-sitter library, I decided that I will upgrade only when 0.25 is
>> released, because it also bumps the language ABI version, which we can
>> detect in Emacs.)
>
> I dug into it, and the culprit is a bug in older tree-sitter versions
> where (the C equivalent of) treesit-node-field-name-for-child gives
> the wrong field name. I rewrote the indent function so it uses child
> node index instead of field name to match indentation rules. Now the
> tests should pass.
>
> Yuan

Confirm

Thanks

  Andrea

Re: master 44fcd37a486 08/11: Add more c-ts-mode indent tests

[Andrea Corallo]

Andrea Corallo <acorallo@gnu.org> writes:

> Yuan Fu <casouri@gmail.com> writes:
>
>>> On Dec 6, 2024, at 11:49 PM, Eli Zaretskii <eliz@gnu.org> wrote:
>>> 
>>>> From: Yuan Fu <casouri@gmail.com>
>>>> Date: Fri, 6 Dec 2024 17:04:42 -0800
>>>> Cc: Emacs Devel <emacs-devel@gnu.org>,
>>>> Andrea Corallo <acorallo@gnu.org>
>>>> 
>>>> 
>>>> 
>>>>> On Dec 5, 2024, at 11:30 PM, Yuan Fu <casouri@gmail.com> wrote:
>>>>> 
>>>>> 
>>>>> 
>>>>>> On Dec 4, 2024, at 11:37 PM, Michael Albinus <michael.albinus@gmx.de> wrote:
>>>>>> 
>>>>>> Yuan Fu <casouri@gmail.com> writes:
>>>>>> 
>>>>>> Hi Yuan,
>>>>>> 
>>>>>>> This is baffling. I assume the pipeline build the latest
>>>>>>> tree-sitter-c/cpp grammar right? I just updated my grammar and
>>>>>>> tree-sitter lib to the latest and the tests still pass on my
>>>>>>> machine. Is tree-sitter version also the latest?
>>>>>> 
>>>>>> See test/infra/Dockerfile.emba. The emacs-tree-sitter container image is
>>>>>> created prior every run, the treesitter grammars mentioned there are
>>>>>> installed.
>>>>> 
>>>>> Ah, so the image uses Debian bookworm, which AFAIK is the current
>>>>> stable release, so it probably doesn’t have the latest tree-sitter
>>>>> library. Let me try to run the pipeline with tree-sitter built
>>>>> from upstream.
>>>>> 
>>>>> Yuan
>>>> 
>>>> I didn’t run the pipeline built from upstream, but I tried running
>>>> the tests locally with the tree-sitter library used in the
>>>> pipeline. The pipeline uses tree-sitter provided by bookworm, which
>>>> is v0.20.7, a pretty old version. And indeed, with that version of
>>>> tree-sitter I can see the test failure.
>>>> 
>>>> Andrea, which version of  tree-sitter do you have on your test bench?
>>>> 
>>>> Since tree-sitter is still relatively unstable, maybe we want to use upstream tree-sitter for the test suite?
>>> 
>>> I don't think this is practical.  I'd rather mark that test as
>>> expected failure or "unstable", if we cannot know the version of the
>>> tree-sitter library used for the test.
>>> 
>>> Or maybe you could rewrite the test so that it works in both old and
>>> new versions of the library.
>>> 
>>> (FWIW, I use 0.20.8, with the latest grammar libraries, and the test
>>> fails here as well.  Due to the issues with ABI breakage in the
>>> tree-sitter library, I decided that I will upgrade only when 0.25 is
>>> released, because it also bumps the language ABI version, which we can
>>> detect in Emacs.)
>>
>> I dug into it, and the culprit is a bug in older tree-sitter versions
>> where (the C equivalent of) treesit-node-field-name-for-child gives
>> the wrong field name. I rewrote the indent function so it uses child
>> node index instead of field name to match indentation rules. Now the
>> tests should pass.
>>
>> Yuan
>
> Confirm

But introduces

progmodes/c-ts-mode.el:379:13: Warning: the function ‘treesit-node-check’ is not known to be defined.

on non treesit configs :)

  Andrea

Re: master 44fcd37a486 08/11: Add more c-ts-mode indent tests

[Yuan Fu]

> On Dec 11, 2024, at 3:53 PM, Andrea Corallo <acorallo@gnu.org> wrote:
> 
> Andrea Corallo <acorallo@gnu.org> writes:
> 
>> Yuan Fu <casouri@gmail.com> writes:
>> 
>>>> On Dec 6, 2024, at 11:49 PM, Eli Zaretskii <eliz@gnu.org> wrote:
>>>> 
>>>>> From: Yuan Fu <casouri@gmail.com>
>>>>> Date: Fri, 6 Dec 2024 17:04:42 -0800
>>>>> Cc: Emacs Devel <emacs-devel@gnu.org>,
>>>>> Andrea Corallo <acorallo@gnu.org>
>>>>> 
>>>>> 
>>>>> 
>>>>>> On Dec 5, 2024, at 11:30 PM, Yuan Fu <casouri@gmail.com> wrote:
>>>>>> 
>>>>>> 
>>>>>> 
>>>>>>> On Dec 4, 2024, at 11:37 PM, Michael Albinus <michael.albinus@gmx.de> wrote:
>>>>>>> 
>>>>>>> Yuan Fu <casouri@gmail.com> writes:
>>>>>>> 
>>>>>>> Hi Yuan,
>>>>>>> 
>>>>>>>> This is baffling. I assume the pipeline build the latest
>>>>>>>> tree-sitter-c/cpp grammar right? I just updated my grammar and
>>>>>>>> tree-sitter lib to the latest and the tests still pass on my
>>>>>>>> machine. Is tree-sitter version also the latest?
>>>>>>> 
>>>>>>> See test/infra/Dockerfile.emba. The emacs-tree-sitter container image is
>>>>>>> created prior every run, the treesitter grammars mentioned there are
>>>>>>> installed.
>>>>>> 
>>>>>> Ah, so the image uses Debian bookworm, which AFAIK is the current
>>>>>> stable release, so it probably doesn’t have the latest tree-sitter
>>>>>> library. Let me try to run the pipeline with tree-sitter built
>>>>>> from upstream.
>>>>>> 
>>>>>> Yuan
>>>>> 
>>>>> I didn’t run the pipeline built from upstream, but I tried running
>>>>> the tests locally with the tree-sitter library used in the
>>>>> pipeline. The pipeline uses tree-sitter provided by bookworm, which
>>>>> is v0.20.7, a pretty old version. And indeed, with that version of
>>>>> tree-sitter I can see the test failure.
>>>>> 
>>>>> Andrea, which version of  tree-sitter do you have on your test bench?
>>>>> 
>>>>> Since tree-sitter is still relatively unstable, maybe we want to use upstream tree-sitter for the test suite?
>>>> 
>>>> I don't think this is practical.  I'd rather mark that test as
>>>> expected failure or "unstable", if we cannot know the version of the
>>>> tree-sitter library used for the test.
>>>> 
>>>> Or maybe you could rewrite the test so that it works in both old and
>>>> new versions of the library.
>>>> 
>>>> (FWIW, I use 0.20.8, with the latest grammar libraries, and the test
>>>> fails here as well.  Due to the issues with ABI breakage in the
>>>> tree-sitter library, I decided that I will upgrade only when 0.25 is
>>>> released, because it also bumps the language ABI version, which we can
>>>> detect in Emacs.)
>>> 
>>> I dug into it, and the culprit is a bug in older tree-sitter versions
>>> where (the C equivalent of) treesit-node-field-name-for-child gives
>>> the wrong field name. I rewrote the indent function so it uses child
>>> node index instead of field name to match indentation rules. Now the
>>> tests should pass.
>>> 
>>> Yuan
>> 
>> Confirm
> 
> But introduces
> 
> progmodes/c-ts-mode.el:379:13: Warning: the function ‘treesit-node-check’ is not known to be defined.
> 
> on non treesit configs :)
> 
>  Andrea

Ah! I’m sorry :) Pushed the fix to master. I promise we’ll be free of these problems soon ;)

Yuan