Re: [GSOC][PATCH] userdiff: add support for Scheme

Junio C Hamano [off-list ref] writes:

Atharva Raykar [off-list ref] writes:
...

quoted

+           (ChangeMe 'suite-name tests)))))))
\ No newline at end of file

Is there a good reason to leave the final line incomplete?  ...
I am also trying to figure out what you wanted to achieve ...

Taking all of them together, here is what I hope you may agree as
its improved version.  The only differences from what you posted are
corrections to all the "\ No newline at end of file" and the simplification
of the pattern (remove "a dot" from the alternative and add \t next
to SP).  Without changes, the new tests still pass so ... ;-)

    diff --git c/userdiff.c w/userdiff.c
    index 5fd0eb31ec..685fe712aa 100644
    --- c/userdiff.c
    +++ w/userdiff.c
    @@ -193,12 +193,8 @@ PATTERNS("rust",
             "|[-+*\\/<>%&^|=!:]=|<<=?|>>=?|&&|\\|\\||->|=>|\\.{2}=|\\.{3}|::"),
     PATTERNS("scheme",
             "^[\t ]*(\\(define-?.*)$",
    -	 /*
    -	  * Scheme allows symbol names to have any character,
    -	  * as long as it is not a form of a parenthesis.
    -	  * The spaces must be escaped.
    -	  */
    -	 "(\\.|[^][)(\\}\\{ ])+"),
    +	 /* whitespace separated tokens, but parentheses also can delimit words */
    +	 "([^][)(\\}\\{ \t])+"),
     PATTERNS("bibtex", "(@[a-zA-Z]{1,}[ \t]*\\{{0,1}[ \t]*[^ \t\"@',\\#}{~%]*).*$",
             "[={}\"]|[^={}\" \t]+"),
     PATTERNS("tex", "^(\\\\((sub)*section|chapter|part)\\*{0,1}\\{.*)$",

----- >8 ---------- >8 ---------- >8 ---------- >8 ---------- >8 -----
From: Atharva Raykar <redacted>
Date: Sat, 27 Mar 2021 23:09:38 +0530
Subject: [PATCH] userdiff: add support for Scheme

Add a diff driver for Scheme (R5RS and R6RS) which
recognizes top level and local `define` forms,
whether it is a function definition, binding, syntax
definition or a user-defined `define-xyzzy` form.

The rationale for picking `define` forms for the
hunk headers is because it is usually the only
significant form for defining the structure of the
program, and it is a common pattern for schemers to
have local function definitions to hide their
visibility, so it is not only the top level
`define`'s that are of interest. Schemers also
extend the language with macros to provide their
own define forms (for example, something like a
`define-test-suite`) which is also captured in the
hunk header.

Since the identifier syntax is quite forgiving, we start
our word regexp from "words delimited by whitespaces" and
then loosen to include various forms of parentheses characters
to word-delimiters.

Signed-off-by: Atharva Raykar <redacted>
[jc: simplified word regex and its explanation; fixed whitespace errors]
Signed-off-by: Junio C Hamano <redacted>
---
 Documentation/gitattributes.txt    | 2 ++
 t/t4018-diff-funcname.sh           | 1 +
 t/t4018/scheme-define-syntax       | 8 ++++++++
 t/t4018/scheme-local-define        | 4 ++++
 t/t4018/scheme-top-level-define    | 4 ++++
 t/t4018/scheme-user-defined-define | 6 ++++++
 t/t4034-diff-words.sh              | 1 +
 t/t4034/scheme/expect              | 9 +++++++++
 t/t4034/scheme/post                | 4 ++++
 t/t4034/scheme/pre                 | 4 ++++
 userdiff.c                         | 4 ++++
 11 files changed, 47 insertions(+)
 create mode 100644 t/t4018/scheme-define-syntax
 create mode 100644 t/t4018/scheme-local-define
 create mode 100644 t/t4018/scheme-top-level-define
 create mode 100644 t/t4018/scheme-user-defined-define
 create mode 100644 t/t4034/scheme/expect
 create mode 100644 t/t4034/scheme/post
 create mode 100644 t/t4034/scheme/pre

diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 0a60472bb5..cfcfa800c2 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt

@@ -845,6 +845,8 @@ patterns are available:
 
 - `rust` suitable for source code in the Rust language.
 
+- `scheme` suitable for source code in the Scheme language.
+
 - `tex` suitable for source code for LaTeX documents.
 
 

diff --git a/t/t4018-diff-funcname.sh b/t/t4018-diff-funcname.sh
index 9675bc17db..823ea96acb 100755
--- a/t/t4018-diff-funcname.sh
+++ b/t/t4018-diff-funcname.sh

@@ -48,6 +48,7 @@ diffpatterns="
 	python
 	ruby
 	rust
+	scheme
 	tex
 	custom1
 	custom2

diff --git a/t/t4018/scheme-define-syntax b/t/t4018/scheme-define-syntax
new file mode 100644
index 0000000000..33fa50c844
--- /dev/null
+++ b/t/t4018/scheme-define-syntax

@@ -0,0 +1,8 @@
+(define-syntax define-test-suite RIGHT
+  (syntax-rules ()
+    ((_ suite-name (name test) ChangeMe ...)
+     (define suite-name
+       (let ((tests
+              `((name . ,test) ...)))
+         (lambda ()
+           (ChangeMe 'suite-name tests)))))))

diff --git a/t/t4018/scheme-local-define b/t/t4018/scheme-local-define
new file mode 100644
index 0000000000..bc6d8aebbe
--- /dev/null
+++ b/t/t4018/scheme-local-define

@@ -0,0 +1,4 @@
+(define (higher-order)
+  (define local-function RIGHT
+    (lambda (x)
+     (car "this is" "ChangeMe"))))

diff --git a/t/t4018/scheme-top-level-define b/t/t4018/scheme-top-level-define
new file mode 100644
index 0000000000..624743c22b
--- /dev/null
+++ b/t/t4018/scheme-top-level-define

@@ -0,0 +1,4 @@
+(define (some-func x y z) RIGHT
+  (let ((a x)
+        (b y))
+        (ChangeMe a b)))

diff --git a/t/t4018/scheme-user-defined-define b/t/t4018/scheme-user-defined-define
new file mode 100644
index 0000000000..70e403c5e2
--- /dev/null
+++ b/t/t4018/scheme-user-defined-define

@@ -0,0 +1,6 @@
+(define-test-suite record-case-tests RIGHT
+  (record-case-1 (lambda (fail)
+                   (let ((a (make-foo 1 2)))
+                     (record-case a
+                       ((bar x) (ChangeMe))
+                       ((foo a b) (+ a b)))))))

diff --git a/t/t4034-diff-words.sh b/t/t4034-diff-words.sh
index 56f1e62a97..ee7721ab91 100755
--- a/t/t4034-diff-words.sh
+++ b/t/t4034-diff-words.sh

@@ -325,6 +325,7 @@ test_language_driver perl
 test_language_driver php
 test_language_driver python
 test_language_driver ruby
+test_language_driver scheme
 test_language_driver tex
 
 test_expect_success 'word-diff with diff.sbe' '

diff --git a/t/t4034/scheme/expect b/t/t4034/scheme/expect
new file mode 100644
index 0000000000..eed21e803c
--- /dev/null
+++ b/t/t4034/scheme/expect

@@ -0,0 +1,9 @@
+<BOLD>diff --git a/pre b/post<RESET>
+<BOLD>index 6a5efba..7c4a6b4 100644<RESET>
+<BOLD>--- a/pre<RESET>
+<BOLD>+++ b/post<RESET>
+<CYAN>@@ -1,4 +1,4 @@<RESET>
+(define (<RED>myfunc a b<RESET><GREEN>my-func first second<RESET>)
+  ; This is a <RED>really<RESET><GREEN>(moderately)<RESET> cool function.
+  (let ((c (<RED>+ a b<RESET><GREEN>add1 first<RESET>)))
+    (format "one more than the total is %d" (<RED>add1<RESET><GREEN>+<RESET> c <GREEN>second<RESET>))))

diff --git a/t/t4034/scheme/post b/t/t4034/scheme/post
new file mode 100644
index 0000000000..28f59c6584
--- /dev/null
+++ b/t/t4034/scheme/post

@@ -0,0 +1,4 @@
+(define (my-func first second)
+  ; This is a (moderately) cool function.
+  (let ((c (add1 first)))
+    (format "one more than the total is %d" (+ c second))))

diff --git a/t/t4034/scheme/pre b/t/t4034/scheme/pre
new file mode 100644
index 0000000000..4bd0069493
--- /dev/null
+++ b/t/t4034/scheme/pre

@@ -0,0 +1,4 @@
+(define (myfunc a b)
+  ; This is a really cool function.
+  (let ((c (+ a b)))
+    (format "one more than the total is %d" (add1 c))))

diff --git a/userdiff.c b/userdiff.c
index 3f81a2261c..685fe712aa 100644
--- a/userdiff.c
+++ b/userdiff.c

@@ -191,6 +191,10 @@ PATTERNS("rust",
 	 "[a-zA-Z_][a-zA-Z0-9_]*"
 	 "|[0-9][0-9_a-fA-Fiosuxz]*(\\.([0-9]*[eE][+-]?)?[0-9_fF]*)?"
 	 "|[-+*\\/<>%&^|=!:]=|<<=?|>>=?|&&|\\|\\||->|=>|\\.{2}=|\\.{3}|::"),
+PATTERNS("scheme",
+	 "^[\t ]*(\\(define-?.*)$",
+	 /* whitespace separated tokens, but parentheses also can delimit words */
+	 "([^][)(\\}\\{ \t])+"),
 PATTERNS("bibtex", "(@[a-zA-Z]{1,}[ \t]*\\{{0,1}[ \t]*[^ \t\"@',\\#}{~%]*).*$",
 	 "[={}\"]|[^={}\" \t]+"),
 PATTERNS("tex", "^(\\\\((sub)*section|chapter|part)\\*{0,1}\\{.*)$",

-- 
2.31.1-255-g3df2b433e7

On Sun, Mar 28 2021, Junio C Hamano wrote:

The "define-?.*" can be simplified to just "define.*", but looking at
the tests is that the intent? From the tests it looks like "define[- ]"
is what the author wants, unless this is meant to also match
"(definements".

Has this been tested on some real-world scheme code? E.g. I have guile
installed locally, and it has really large top-level eval-when
blocks. These rules would jump over those to whatever the function above
them is.

On 28-Mar-2021, at 08:46, Ævar Arnfjörð Bjarmason [off-list ref] wrote:

The "define-?.*" can be simplified to just "define.*", but looking at
the tests is that the intent? From the tests it looks like "define[- ]"
is what the author wants, unless this is meant to also match
"(definements".

Yes, you captured my intent correctly. Will fix it.

Has this been tested on some real-world scheme code? E.g. I have guile
installed locally, and it has really large top-level eval-when
blocks. These rules would jump over those to whatever the function above
them is.

I do not have a large scheme codebase on my own, I usually use Racket,
which is a much larger language with many more forms. Other Schemes like
Guile also extend the language a lot, like in your example, eval-when is
an extension provided by Guile (and Chicken and Chez), but not a part of
the R6RS document when I searched its index.

So the 'define' forms are the only one that I know would reliably be present
across all schemes. But one can also make a case where some of these non-standard
forms may be common enough that they are worth adding in. In that case which
forms to include? Should we consider everything in the SRFI's[1]? Should the
various module definitions of Racket be included? It's a little tricky to know
where to stop.

That being said, I will try to run this through more Scheme codebases that I can
find and see if there are any forms that seem to show up commonly enough that they
are worth including.

[1] https://en.wikipedia.org/wiki/Scheme_Requests_for_Implementation

On 28-Mar-2021, at 04:39, Junio C Hamano [off-list ref] wrote:

Junio C Hamano [off-list ref] writes:

quoted

Atharva Raykar [off-list ref] writes:
...

quoted

+           (ChangeMe 'suite-name tests)))))))
\ No newline at end of file

Is there a good reason to leave the final line incomplete?  ...
I am also trying to figure out what you wanted to achieve ...

Taking all of them together, here is what I hope you may agree as
its improved version.  The only differences from what you posted are
corrections to all the "\ No newline at end of file" and the simplification
of the pattern (remove "a dot" from the alternative and add \t next
to SP).  Without changes, the new tests still pass so ... ;-)

   diff --git c/userdiff.c w/userdiff.c
   index 5fd0eb31ec..685fe712aa 100644
   --- c/userdiff.c
   +++ w/userdiff.c
   @@ -193,12 +193,8 @@ PATTERNS("rust",
            "|[-+*\\/<>%&^|=!:]=|<<=?|>>=?|&&|\\|\\||->|=>|\\.{2}=|\\.{3}|::"),
    PATTERNS("scheme",
            "^[\t ]*(\\(define-?.*)$",
   -	 /*
   -	  * Scheme allows symbol names to have any character,
   -	  * as long as it is not a form of a parenthesis.
   -	  * The spaces must be escaped.
   -	  */
   -	 "(\\.|[^][)(\\}\\{ ])+"),
   +	 /* whitespace separated tokens, but parentheses also can delimit words */
   +	 "([^][)(\\}\\{ \t])+"),
    PATTERNS("bibtex", "(@[a-zA-Z]{1,}[ \t]*\\{{0,1}[ \t]*[^ \t\"@',\\#}{~%]*).*$",
            "[={}\"]|[^={}\" \t]+"),
    PATTERNS("tex", "^(\\\\((sub)*section|chapter|part)\\*{0,1}\\{.*)$",

Thanks for these. I will eventually send another patch with the whitespaces corrected,
and try to see if there is a better way to handle backslashes, other than the regex I
suggested. I will also be writing another test case to check that case properly.

I will also incorporate the other changes suggested by Johannes and Ævar as well,
My regex was not supposed to capture forms like `defined-thing`. And there are a
few rough edges with some of my test cases, which I will correct as well in the next
patch. It is also worth spending some more time and see if there is any other form other
than definitions that a Scheme programmer other than myself may be interested in. I will
consult a few Scheme communities and mailing lists and see what more experienced
programmers have to say.

Hi Atharva

On 28/03/2021 13:40, Atharva Raykar wrote:

On 28-Mar-2021, at 08:46, Ævar Arnfjörð Bjarmason [off-list ref] wrote:

quoted

The "define-?.*" can be simplified to just "define.*", but looking at
the tests is that the intent? From the tests it looks like "define[- ]"
is what the author wants, unless this is meant to also match
"(definements".

Yes, you captured my intent correctly. Will fix it.

quoted

Has this been tested on some real-world scheme code? E.g. I have guile
installed locally, and it has really large top-level eval-when
blocks. These rules would jump over those to whatever the function above
them is.

I do not have a large scheme codebase on my own, I usually use Racket,
which is a much larger language with many more forms. Other Schemes like
Guile also extend the language a lot, like in your example, eval-when is
an extension provided by Guile (and Chicken and Chez), but not a part of
the R6RS document when I searched its index.

So the 'define' forms are the only one that I know would reliably be present
across all schemes. But one can also make a case where some of these non-standard
forms may be common enough that they are worth adding in. In that case which
forms to include? Should we consider everything in the SRFI's[1]? Should the
various module definitions of Racket be included? It's a little tricky to know
where to stop.

If there are some common forms such as eval-when then it would be good to include them, otherwise we end up needing a different rule for each scheme implementation as they all seem to tweak something. Gerbil uses 'def...' e.g def, defsyntax, defstruct, defrules rather than define, define-syntax, define-record etc. I'm not user if we want to accommodate that or not.

Best Wishes

Phillip

That being said, I will try to run this through more Scheme codebases that I can
find and see if there are any forms that seem to show up commonly enough that they
are worth including.

[1] https://en.wikipedia.org/wiki/Scheme_Requests_for_Implementation

On 29-Mar-2021, at 15:38, Phillip Wood [off-list ref] wrote:

On 28/03/2021 13:40, Atharva Raykar wrote:

quoted

On 28-Mar-2021, at 08:46, Ævar Arnfjörð Bjarmason [off-list ref] wrote:

quoted

The "define-?.*" can be simplified to just "define.*", but looking at
the tests is that the intent? From the tests it looks like "define[- ]"
is what the author wants, unless this is meant to also match
"(definements".

Yes, you captured my intent correctly. Will fix it.

quoted

Has this been tested on some real-world scheme code? E.g. I have guile
installed locally, and it has really large top-level eval-when
blocks. These rules would jump over those to whatever the function above
them is.

I do not have a large scheme codebase on my own, I usually use Racket,
which is a much larger language with many more forms. Other Schemes like
Guile also extend the language a lot, like in your example, eval-when is
an extension provided by Guile (and Chicken and Chez), but not a part of
the R6RS document when I searched its index.
So the 'define' forms are the only one that I know would reliably be present
across all schemes. But one can also make a case where some of these non-standard
forms may be common enough that they are worth adding in. In that case which
forms to include? Should we consider everything in the SRFI's[1]? Should the
various module definitions of Racket be included? It's a little tricky to know
where to stop.

If there are some common forms such as eval-when then it would be good to include them, otherwise we end up needing a different rule for each scheme implementation as they all seem to tweak something. Gerbil uses 'def...' e.g def, defsyntax, defstruct, defrules rather than define, define-syntax, define-record etc. I'm not user if we want to accommodate that or not.

Yes, this is the part that is hard for me to figure out. I am going by
two heuristics: what Scheme communities in other places would generally
prefer, and what patterns I see happen more often in scheme code.

The former is tricky to do. I posted to a few mailing lists about this,
but they don't seem active enough to garner any responses.

The latter is a little easier to measure quickly. I did a GitHub search,
where I filtered results to only consider Scheme files (language:scheme).

Some armchair stats, just for a broad understanding:

  Total number of scheme files: 529,339
  No. of times a construct is used in those files:
    define and its variants : 431,090 (81.4%)
    def and its variants    :  18,466 ( 3.5%)
    eval-when               :   3,375 ( 0.6%)

There was no way for me to quickly know which of these uses are at the top
level, but either way of the more structural forms that do show up in Scheme
code, define and its variants seem like a clear winner. I am not sure if
it's worth adding more rules to check for def and its variants, given that
they are not nearly as common.

On Tue, Mar 30 2021, Atharva Raykar wrote:

On 29-Mar-2021, at 15:38, Phillip Wood [off-list ref] wrote:

quoted

On 28/03/2021 13:40, Atharva Raykar wrote:

quoted

On 28-Mar-2021, at 08:46, Ævar Arnfjörð Bjarmason [off-list ref] wrote:

quoted

The "define-?.*" can be simplified to just "define.*", but looking at
the tests is that the intent? From the tests it looks like "define[- ]"
is what the author wants, unless this is meant to also match
"(definements".

Yes, you captured my intent correctly. Will fix it.

quoted

Has this been tested on some real-world scheme code? E.g. I have guile
installed locally, and it has really large top-level eval-when
blocks. These rules would jump over those to whatever the function above
them is.

I do not have a large scheme codebase on my own, I usually use Racket,
which is a much larger language with many more forms. Other Schemes like
Guile also extend the language a lot, like in your example, eval-when is
an extension provided by Guile (and Chicken and Chez), but not a part of
the R6RS document when I searched its index.
So the 'define' forms are the only one that I know would reliably be present
across all schemes. But one can also make a case where some of these non-standard
forms may be common enough that they are worth adding in. In that case which
forms to include? Should we consider everything in the SRFI's[1]? Should the
various module definitions of Racket be included? It's a little tricky to know
where to stop.

If there are some common forms such as eval-when then it would be good to include them, otherwise we end up needing a different rule for each scheme implementation as they all seem to tweak something. Gerbil uses 'def...' e.g def, defsyntax, defstruct, defrules rather than define, define-syntax, define-record etc. I'm not user if we want to accommodate that or not.

Yes, this is the part that is hard for me to figure out. I am going by
two heuristics: what Scheme communities in other places would generally
prefer, and what patterns I see happen more often in scheme code.

The former is tricky to do. I posted to a few mailing lists about this,
but they don't seem active enough to garner any responses.

The latter is a little easier to measure quickly. I did a GitHub search,
where I filtered results to only consider Scheme files (language:scheme).

Some armchair stats, just for a broad understanding:

  Total number of scheme files: 529,339
  No. of times a construct is used in those files:
    define and its variants : 431,090 (81.4%)
    def and its variants    :  18,466 ( 3.5%)
    eval-when               :   3,375 ( 0.6%)

There was no way for me to quickly know which of these uses are at the top
level, but either way of the more structural forms that do show up in Scheme
code, define and its variants seem like a clear winner. I am not sure if
it's worth adding more rules to check for def and its variants, given that
they are not nearly as common.

In those cases we should veer on the side of inclusion. The only problem
we'll have is if "eval-when" is a "setq"-like function top-level form in
some other scheme dialect, so we'll have a conflict.

Otherwise it's fine, programs that only use "define" won't be bothered
by an eval-when rule.

On 30-Mar-2021, at 18:26, Ævar Arnfjörð Bjarmason [off-list ref] wrote:

On Tue, Mar 30 2021, Atharva Raykar wrote:

quoted

On 29-Mar-2021, at 15:38, Phillip Wood [off-list ref] wrote:

quoted

On 28/03/2021 13:40, Atharva Raykar wrote:

quoted

On 28-Mar-2021, at 08:46, Ævar Arnfjörð Bjarmason [off-list ref] wrote:

quoted

The "define-?.*" can be simplified to just "define.*", but looking at
the tests is that the intent? From the tests it looks like "define[- ]"
is what the author wants, unless this is meant to also match
"(definements".

Yes, you captured my intent correctly. Will fix it.

quoted

Has this been tested on some real-world scheme code? E.g. I have guile
installed locally, and it has really large top-level eval-when
blocks. These rules would jump over those to whatever the function above
them is.

I do not have a large scheme codebase on my own, I usually use Racket,
which is a much larger language with many more forms. Other Schemes like
Guile also extend the language a lot, like in your example, eval-when is
an extension provided by Guile (and Chicken and Chez), but not a part of
the R6RS document when I searched its index.
So the 'define' forms are the only one that I know would reliably be present
across all schemes. But one can also make a case where some of these non-standard
forms may be common enough that they are worth adding in. In that case which
forms to include? Should we consider everything in the SRFI's[1]? Should the
various module definitions of Racket be included? It's a little tricky to know
where to stop.

If there are some common forms such as eval-when then it would be good to include them, otherwise we end up needing a different rule for each scheme implementation as they all seem to tweak something. Gerbil uses 'def...' e.g def, defsyntax, defstruct, defrules rather than define, define-syntax, define-record etc. I'm not user if we want to accommodate that or not.

Yes, this is the part that is hard for me to figure out. I am going by
two heuristics: what Scheme communities in other places would generally
prefer, and what patterns I see happen more often in scheme code.

The former is tricky to do. I posted to a few mailing lists about this,
but they don't seem active enough to garner any responses.

The latter is a little easier to measure quickly. I did a GitHub search,
where I filtered results to only consider Scheme files (language:scheme).

Some armchair stats, just for a broad understanding:

 Total number of scheme files: 529,339
 No. of times a construct is used in those files:
   define and its variants : 431,090 (81.4%)
   def and its variants    :  18,466 ( 3.5%)
   eval-when               :   3,375 ( 0.6%)

There was no way for me to quickly know which of these uses are at the top
level, but either way of the more structural forms that do show up in Scheme
code, define and its variants seem like a clear winner. I am not sure if
it's worth adding more rules to check for def and its variants, given that
they are not nearly as common.

In those cases we should veer on the side of inclusion. The only problem
we'll have is if "eval-when" is a "setq"-like function top-level form in
some other scheme dialect, so we'll have a conflict.

Otherwise it's fine, programs that only use "define" won't be bothered
by an eval-when rule.

I would like some clarification, since my knowledge of Common Lisp's setq
and Guile's/Other's eval-when is pretty surface level.

The only problem we'll have is if "eval-when" is a "setq"-like function
top-level form in some other scheme dialect, so we'll have a conflict.

I am not sure what you mean when you say if "eval-when" is a "setq"-like
top level form, and exactly what kind of problem it may cause.

I also realized from my understanding of the Guile Documentation[1],
that "eval-when" is used to tell the compiler which expressions should be
made available during the expansion phase.

It does not seem to have anything that may help identify the location of the
hunk, which I understand is the primary purpose of these hunk headers.
All uses of "eval-when" would be some variation of:

	(eval-when (expand load eval) ; no identifier in this form
	  ...)

unlike a "define" which will always name the nearest function, which helps as
a landmark.

Would that be a valid reason to exclude "eval-when"?