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.
The word regex is a best-effort attempt to conform
to R6RS[1] valid identifiers, symbols and numbers.
[1] http://www.r6rs.org/final/html/r6rs/r6rs-Z-H-7.html#node_chap_4
Signed-off-by: Atharva Raykar <redacted>
---
Hi, first-time contributor here, I wanted to have a go at this as
a microproject.
A few things I had to consider:
- Going through the mailing list, there have already been two other
patches that are for lispy languages that have taken slightly
different approaches: Elisp[1] and Clojure[2]. Would it make any
sense to have a single userdiff driver for lisp that just captures
all top level forms in the hunk? I personally felt it's better to
differentiate the drivers for each language, as they have different
constructs.
- It was hard to decide exactly which forms should appear on the hunk
headers. Having programmed in a Scheme before, I went with the forms I
would have liked to see when looking at git diffs, which would be the
nearest `define` along with `define-syntax` and other define forms that
are created as user-defined macros. I am willing to ask around in
certain active scheme communities for some kind of consensus, but there
is no single large consolidated group of schemers (the closest is
probably comp.lang.scheme?).
- By best-effort attempt at the wordregex, I mean that it is a little
more permissive than it has to be, as it accepts a few words that are
technically invalid in Scheme.
Making it handle all cases like numbers and identifiers with separate
regexen would be greatly complicated (Eg: #x#e10.2f3 is a valid number
but #x#f10.2e3 is not; 10t1 is a valid identifier, but 10s1 is a number
-- my wordregex just clubs all of these into a generic 'word match' which
trades of granularity for simplicity, and it usually does the right thing).
[1] http://public-inbox.org/git/20210213192447.6114-1-git@adamspiers.org/
[2] http://public-inbox.org/git/pull.902.git.1615667191368.gitgitgadget@gmail.com/
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 | 8 ++++++++
11 files changed, 51 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
@@ -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.
@@ -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>))))
@@ -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))))
From: Johannes Sixt <hidden> Date: 2021-03-27 23:59:57
Am 27.03.21 um 18:39 schrieb Atharva Raykar:
- By best-effort attempt at the wordregex, I mean that it is a little
more permissive than it has to be, as it accepts a few words that are
technically invalid in Scheme.
Making it handle all cases like numbers and identifiers with separate
regexen would be greatly complicated (Eg: #x#e10.2f3 is a valid number
but #x#f10.2e3 is not; 10t1 is a valid identifier, but 10s1 is a number
-- my wordregex just clubs all of these into a generic 'word match' which
trades of granularity for simplicity, and it usually does the right thing).
It is ok to have regex that capture tokens that are not valid. A
userdiff driver can assume that it operates only text that is valid in
the language.
This test is suspicious. Notice the "ChangeMe" above? That is sufficient
to let the test case succeed. The "ChangeMe" in the last line below
should be the only one.
But then there is this indented '(define' that is not marked as RIGHT,
and I wonder how is it different from...
quoted hunk
+ (let ((tests
+ `((name . ,test) ...)))
+ (lambda ()
+ (ChangeMe 'suite-name tests)))))))
\ No newline at end of file
This "optional hyphen followed by anything" in the regex is strange.
Wouldn't that also capture a line that looks like, e.g.,
(defined-foo bar)
Perhaps we want "define[- \t].*" in the regex?
quoted hunk
+ /* + * Scheme allows symbol names to have any character,+ * as long as it is not a form of a parenthesis.+ * The spaces must be escaped.+ */+ "(\\.|[^][)(\\}\\{ ])+"), PATTERNS("bibtex", "(@[a-zA-Z]{1,}[ \t]*\\{{0,1}[ \t]*[^ \t\"@',\\#}{~%]*).*$", "[={}\"]|[^={}\" \t]+"), PATTERNS("tex", "^(\\\\((sub)*section|chapter|part)\\*{0,1}\\{.*)$",
This test is suspicious. Notice the "ChangeMe" above? That is sufficient
to let the test case succeed. The "ChangeMe" in the last line below
should be the only one.
Thanks for pointing this out. The second "ChangeMe" was not supposed to be
there.
What I wanted to test was the hunk header showing the line for
'(define-syntax ...' and not the internal '(define ...' below it. Thus the
ChangeMe should be located above the internal define so that the hunk header
would show define-syntax and not the local define.
But then there is this indented '(define' that is not marked as RIGHT,
and I wonder how is it different from...
quoted
+ (let ((tests
+ `((name . ,test) ...)))
+ (lambda ()
+ (ChangeMe 'suite-name tests)))))))
\ No newline at end of file
@@ -0,0 +1,4 @@+(define (higher-order)+ (define local-function RIGHT
... this one, which is also indented and *is* marked as RIGHT.
In this test case, I was explicitly testing for an indented '(define'
whereas in the former, I was testing for the top-level '(define-syntax',
which happened to have an internal define (which will inevitably show up
in a lot of scheme code).
quoted
+ (lambda (x)
+ (car "this is" "ChangeMe"))))
\ No newline at end of file
This "optional hyphen followed by anything" in the regex is strange.
Wouldn't that also capture a line that looks like, e.g.,
(defined-foo bar)
Perhaps we want "define[- \t].*" in the regex?
Yes, this is what I intended to do, thanks for correcting it.
@@ -0,0 +1,4 @@+(define (higher-order)+ (define local-function RIGHT
... this one, which is also indented and *is* marked as RIGHT.
In this test case, I was explicitly testing for an indented '(define'
whereas in the former, I was testing for the top-level '(define-syntax',
which happened to have an internal define (which will inevitably show up
in a lot of scheme code).
It would be nice to include indented define forms but including them means that any change to the body of a function is attributed to the last internal definition rather than the actual function. For example
(define (f arg)
(define (g x)
(+ 1 x))
(some-func ...)
;;any change here will have '(define (g x)' in the hunk header, not '(define (f arg)'
I don't think this can be avoided as we rely on regexs rather than parsing the source so it is probably best to only match toplevel defines.
Best Wishes
Phillip
quoted
quoted
+ (lambda (x)
+ (car "this is" "ChangeMe"))))
\ No newline at end of file
This "optional hyphen followed by anything" in the regex is strange.
Wouldn't that also capture a line that looks like, e.g.,
(defined-foo bar)
Perhaps we want "define[- \t].*" in the regex?
Yes, this is what I intended to do, thanks for correcting it.
From: Johannes Sixt <hidden> Date: 2021-03-29 10:49:05
Am 29.03.21 um 12:18 schrieb Phillip Wood:
It would be nice to include indented define forms but including them
means that any change to the body of a function is attributed to the
last internal definition rather than the actual function. For example
(define (f arg)
(define (g x)
(+ 1 x))
(some-func ...)
;;any change here will have '(define (g x)' in the hunk header, not
'(define (f arg)'
I don't think this can be avoided as we rely on regexs rather than
parsing the source so it is probably best to only match toplevel defines.
There can be two rules, one that matches '(define-' that is indented,
and another one that matches all non-indented forms of definitions. If
that is what you mean.
-- Hannes
It would be nice to include indented define forms but including them
means that any change to the body of a function is attributed to the
last internal definition rather than the actual function. For example
(define (f arg)
(define (g x)
(+ 1 x))
(some-func ...)
;;any change here will have '(define (g x)' in the hunk header, not
'(define (f arg)'
I don't think this can be avoided as we rely on regexs rather than
parsing the source so it is probably best to only match toplevel defines.
There can be two rules, one that matches '(define-' that is indented,
and another one that matches all non-indented forms of definitions. If
that is what you mean.
Yes, but that doesn't help in these sorts of cases because what a rule
like that really wants is some version of "don't match this line, but
only if you can reasonably match this other rule".
We can only do rule precedence on a per-line basis via the inverted
matches.
So for languages like cl/elisp/scheme and others where it's common to
have nested function definitions (then -W would like the top-level) *OR*
similarly looking nested function definitions, but the top-level isn't a
function but a (setq) or whatever we're basically stuck with picking one
or the other.
I've pondered how to get around this problem in my userdiff.c hacking
without resorting to supporting some general-purpose Turing machine, and
have so far come up with nothing.
You can see lots of prior art by grepping Emacs's source code for
beginning-of-defun, it solves this problem by exposing a Turing machine
:)
On 29/03/2021 14:12, Ævar Arnfjörð Bjarmason wrote:
On Mon, Mar 29 2021, Johannes Sixt wrote:
quoted
Am 29.03.21 um 12:18 schrieb Phillip Wood:
quoted
It would be nice to include indented define forms but including them
means that any change to the body of a function is attributed to the
last internal definition rather than the actual function. For example
(define (f arg)
(define (g x)
(+ 1 x))
(some-func ...)
;;any change here will have '(define (g x)' in the hunk header, not
'(define (f arg)'
I don't think this can be avoided as we rely on regexs rather than
parsing the source so it is probably best to only match toplevel defines.
There can be two rules, one that matches '(define-' that is indented,
and another one that matches all non-indented forms of definitions. If
that is what you mean.
Yes, but that doesn't help in these sorts of cases because what a rule
like that really wants is some version of "don't match this line, but
only if you can reasonably match this other rule".
We can only do rule precedence on a per-line basis via the inverted
matches.
So for languages like cl/elisp/scheme and others where it's common to
have nested function definitions (then -W would like the top-level) *OR*
similarly looking nested function definitions, but the top-level isn't a
function but a (setq) or whatever we're basically stuck with picking one
or the other.
Exactly
I've pondered how to get around this problem in my userdiff.c hacking
without resorting to supporting some general-purpose Turing machine, and
have so far come up with nothing.
I think using an indentation heuristic would probably work quite well for most languages - see https://public-inbox.org/git/20200923215859.102981-1-rtzoeller@rtzoeller.com/ for a discussion from last year (from memory there were some problems with the approach in those patches but I think there are some suggestion from Peff and me later in the thread on how they could be overcome)
Best Wishes
Phillip
You can see lots of prior art by grepping Emacs's source code for
beginning-of-defun, it solves this problem by exposing a Turing machine
:)
@@ -0,0 +1,4 @@+(define (higher-order)+ (define local-function RIGHT
... this one, which is also indented and *is* marked as RIGHT.
In this test case, I was explicitly testing for an indented '(define'
whereas in the former, I was testing for the top-level '(define-syntax',
which happened to have an internal define (which will inevitably show up
in a lot of scheme code).
It would be nice to include indented define forms but including them means that any change to the body of a function is attributed to the last internal definition rather than the actual function. For example
(define (f arg)
(define (g x)
(+ 1 x))
(some-func ...)
;;any change here will have '(define (g x)' in the hunk header, not '(define (f arg)'
The reason I went for this over the top level forms, is because
I felt it was useful to see the nearest definition for internal
functions that often have a lot of the actual business logic of
the program (at least a lot of SICP seems to follow this pattern).
The disadvantage is as you said, it might also catch trivial inner
functions and the developer might lose context.
Another problem is it may match more trivial bindings, like:
(define (some-func things)
...
(define items '(eggs
ham
peanut-butter))
...)
What I have noticed *anecdotally* is that this is not common enough
to be too much of a problem, and local define bindings seem to be more
favoured in Racket than other Schemes, that use 'let' more often.
I don't think this can be avoided as we rely on regexs rather than parsing the source so it is probably best to only match toplevel defines.
The other issue with only matching top level defines is that a
lot of scheme programs are library definitions, something like
(library
(foo bar)
(export ...)
(define ...)
(define ...)
;; and a bunch of other definitions...
)
Only matching top level defines will completely ignore matching all
the definitions in these files.
@@ -0,0 +1,4 @@+(define (higher-order)+ (define local-function RIGHT
... this one, which is also indented and *is* marked as RIGHT.
In this test case, I was explicitly testing for an indented '(define'
whereas in the former, I was testing for the top-level '(define-syntax',
which happened to have an internal define (which will inevitably show up
in a lot of scheme code).
It would be nice to include indented define forms but including them means that any change to the body of a function is attributed to the last internal definition rather than the actual function. For example
(define (f arg)
(define (g x)
(+ 1 x))
(some-func ...)
;;any change here will have '(define (g x)' in the hunk header, not '(define (f arg)'
The reason I went for this over the top level forms, is because
I felt it was useful to see the nearest definition for internal
functions that often have a lot of the actual business logic of
the program (at least a lot of SICP seems to follow this pattern).
The disadvantage is as you said, it might also catch trivial inner
functions and the developer might lose context.
Never mind this message, I had misunderstood the problem you were trying to
demonstrate. I wholeheartedly agree with what you are trying to say, and
the indentation heuristic discussed does look interesting. I shall have a
glance at the RFC you linked in the other reply.
The disadvantage is as you said, it might also catch trivial inner
functions and the developer might lose context.
Feel free to disregard me misquoting you here. You did not say that (:
Another problem is it may match more trivial bindings, like:
(define (some-func things)
...
(define items '(eggs
ham
peanut-butter))
...)
What I have noticed *anecdotally* is that this is not common enough
to be too much of a problem, and local define bindings seem to be more
favoured in Racket than other Schemes, that use 'let' more often.
quoted
I don't think this can be avoided as we rely on regexs rather than parsing the source so it is probably best to only match toplevel defines.
The other issue with only matching top level defines is that a
lot of scheme programs are library definitions, something like
(library
(foo bar)
(export ...)
(define ...)
(define ...)
;; and a bunch of other definitions...
)
Only matching top level defines will completely ignore matching all
the definitions in these files.
That said, I still stand by the fact that only catching top level defines
will lead to a lot of definitions being ignored. Maybe the occasional
mismatch may be worth the gain in the number of function contexts being
detected?
Hello all,
This is v2 of the patch I sent to add userdiff support to Scheme. I have
modified my approach to be more inclusive of as many Schemes as possible and
thus added many more forms. Since Ævar suggested we veer on the side of
inclusion when talking about the Gerbil scheme syntax, I felt it made sense to
extend the driver to include common, but non-standard extensions of Scheme,
including the forms of Racket and Guile.
Forms added since last patch:
- Variants of define such as def, defsyntax etc as suggested by Phillip and the
other reviewers. - The library form of R6RS, as well as module definitions of
Racket, Gerbil and Guile. * These forms were added on the recommendation of
Göran Weinholt, creator of the Akku scheme package manager and the Loko
Scheme implementation:
https://groups.google.com/g/comp.lang.scheme/c/Aczn0TNEr5g/m/Jq3AlKvZBgAJ -
The Racket forms for defining structs and classes.
I have restricted the "def" forms to only certain keywords, so that it does not
over-match to words like "deflate", "deform", "defer" etc.
I have also allowed the use of "/" after a define or def form, as some scheme
code uses it as a convention for defines in a certain context, such as Racket's
"define/public".
I have also fixed some a test case which had a redundant "ChangeMe".
Finally in the word regex, which has been simplified a lot, while retaining the
same functioning after taking into account Junio's suggestions.
Atharva Raykar (1):
userdiff: add support for Scheme
Documentation/gitattributes.txt | 2 ++
t/t4018-diff-funcname.sh | 1 +
t/t4018/scheme-class | 7 +++++++
t/t4018/scheme-def | 4 ++++
t/t4018/scheme-def-variant | 4 ++++
t/t4018/scheme-define-slash-public | 7 +++++++
t/t4018/scheme-define-syntax | 8 ++++++++
t/t4018/scheme-define-variant | 4 ++++
t/t4018/scheme-library | 11 +++++++++++
t/t4018/scheme-local-define | 4 ++++
t/t4018/scheme-module | 6 ++++++
t/t4018/scheme-top-level-define | 4 ++++
t/t4018/scheme-user-defined-define | 6 ++++++
t/t4034-diff-words.sh | 1 +
t/t4034/scheme/expect | 10 ++++++++++
t/t4034/scheme/post | 5 +++++
t/t4034/scheme/pre | 5 +++++
userdiff.c | 4 ++++
18 files changed, 93 insertions(+)
create mode 100644 t/t4018/scheme-class
create mode 100644 t/t4018/scheme-def
create mode 100644 t/t4018/scheme-def-variant
create mode 100644 t/t4018/scheme-define-slash-public
create mode 100644 t/t4018/scheme-define-syntax
create mode 100644 t/t4018/scheme-define-variant
create mode 100644 t/t4018/scheme-library
create mode 100644 t/t4018/scheme-local-define
create mode 100644 t/t4018/scheme-module
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
--
2.31.1
Add a diff driver for Scheme-like languages which recognizes top level
and local `define` forms, whether it is a function definition, binding,
syntax definition or a user-defined `define-xyzzy` form.
Also supports R6RS `library` forms, `module` forms along with class and
struct declarations used in Racket (PLT Scheme).
Alternate "def" syntax such as those in Gerbil Scheme are also
supported, like defstruct, defsyntax and so on.
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 it is common practice to extend syntax with variants of a form
like `module+`, `class*` etc, those have been supported as well.
The word regex is a best-effort attempt to conform to R6RS[1] valid
identifiers, symbols and numbers.
[1] http://www.r6rs.org/final/html/r6rs/r6rs-Z-H-7.html#node_chap_4
Signed-off-by: Atharva Raykar <redacted>
---
Documentation/gitattributes.txt | 2 ++
t/t4018-diff-funcname.sh | 1 +
t/t4018/scheme-class | 7 +++++++
t/t4018/scheme-def | 4 ++++
t/t4018/scheme-def-variant | 4 ++++
t/t4018/scheme-define-slash-public | 7 +++++++
t/t4018/scheme-define-syntax | 8 ++++++++
t/t4018/scheme-define-variant | 4 ++++
t/t4018/scheme-library | 11 +++++++++++
t/t4018/scheme-local-define | 4 ++++
t/t4018/scheme-module | 6 ++++++
t/t4018/scheme-top-level-define | 4 ++++
t/t4018/scheme-user-defined-define | 6 ++++++
t/t4034-diff-words.sh | 1 +
t/t4034/scheme/expect | 10 ++++++++++
t/t4034/scheme/post | 5 +++++
t/t4034/scheme/pre | 5 +++++
userdiff.c | 4 ++++
18 files changed, 93 insertions(+)
create mode 100644 t/t4018/scheme-class
create mode 100644 t/t4018/scheme-def
create mode 100644 t/t4018/scheme-def-variant
create mode 100644 t/t4018/scheme-define-slash-public
create mode 100644 t/t4018/scheme-define-syntax
create mode 100644 t/t4018/scheme-define-variant
create mode 100644 t/t4018/scheme-library
create mode 100644 t/t4018/scheme-local-define
create mode 100644 t/t4018/scheme-module
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
@@ -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.
@@ -0,0 +1,10 @@+<BOLD>diff --git a/pre b/post<RESET>+<BOLD>index cb56df5..09d9506 100644<RESET>+<BOLD>--- a/pre<RESET>+<BOLD>+++ b/post<RESET>+<CYAN>@@ -1,5 +1,5 @@<RESET>+(define (<RED>myfunc a b<RESET><GREEN>my-func first second<RESET>)+ ; This is a <RED>really<RESET><GREEN>(moderately)<RESET> cool function.+ (<RED>this\place<RESET><GREEN>that\place<RESET> (+ 3 4))+ (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>))))
@@ -0,0 +1,5 @@+(define (my-func first second)+ ; This is a (moderately) cool function.+ (that\place (+ 3 4))+ (let ((c (add1 first)))+ (format "one more than the total is %d" (+ c second))))
@@ -0,0 +1,5 @@+(define (myfunc a b)+ ; This is a really cool function.+ (this\place (+ 3 4))+ (let ((c (+ a b)))+ (format "one more than the total is %d" (add1 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|def(struct|syntax|class|method|rules|record|proto|alias)?)[-*/ \t]|(library|module|struct|class)[*+ \t]).*)$",+/* All words should be delimited by spaces or parentheses */+"([^][)(}{[ \t])+"),PATTERNS("bibtex","(@[a-zA-Z]{1,}[ \t]*\\{{0,1}[ \t]*[^ \t\"@',\\#}{~%]*).*$","[={}\"]|[^={}\"\t]+"),PATTERNS("tex","^(\\\\((sub)*section|chapter|part)\\*{0,1}\\{.*)$",
@@ -0,0 +1,4 @@+(define (higher-order)+ (define local-function RIGHT
... this one, which is also indented and *is* marked as RIGHT.
In this test case, I was explicitly testing for an indented '(define'
whereas in the former, I was testing for the top-level '(define-syntax',
which happened to have an internal define (which will inevitably show up
in a lot of scheme code).
It would be nice to include indented define forms but including them means that any change to the body of a function is attributed to the last internal definition rather than the actual function. For example
(define (f arg)
(define (g x)
(+ 1 x))
(some-func ...)
;;any change here will have '(define (g x)' in the hunk header, not '(define (f arg)'
The reason I went for this over the top level forms, is because
I felt it was useful to see the nearest definition for internal
functions that often have a lot of the actual business logic of
the program (at least a lot of SICP seems to follow this pattern).
The disadvantage is as you said, it might also catch trivial inner
functions and the developer might lose context.
Never mind this message, I had misunderstood the problem you were trying to
demonstrate. I wholeheartedly agree with what you are trying to say, and
the indentation heuristic discussed does look interesting. I shall have a
glance at the RFC you linked in the other reply.
quoted
The disadvantage is as you said, it might also catch trivial inner
functions and the developer might lose context.
Feel free to disregard me misquoting you here. You did not say that (:
quoted
Another problem is it may match more trivial bindings, like:
(define (some-func things)
...
(define items '(eggs
ham
peanut-butter))
...)
What I have noticed *anecdotally* is that this is not common enough
to be too much of a problem, and local define bindings seem to be more
favoured in Racket than other Schemes, that use 'let' more often.
quoted
I don't think this can be avoided as we rely on regexs rather than parsing the source so it is probably best to only match toplevel defines.
The other issue with only matching top level defines is that a
lot of scheme programs are library definitions, something like
(library
(foo bar)
(export ...)
(define ...)
(define ...)
;; and a bunch of other definitions...
)
Only matching top level defines will completely ignore matching all
the definitions in these files.
That said, I still stand by the fact that only catching top level defines
will lead to a lot of definitions being ignored. Maybe the occasional
mismatch may be worth the gain in the number of function contexts being
detected?
I'm not sure that the mismatches will be occasional - every time you have an internal definition in a function the hunk header will be wrong when you change the main body of the function. This will affect grep --function-context and diff -W as well as the normal hunk headers. The problem is there is no way to avoid that and provide something useful in the library example you have above. It would be useful to find some code bases and diff the output of 'git log --patch' with and without the leading whitespace match in the function pattern to see how often this is a problem (i.e. when the funcnames do not match see which one is correct).
Best Wishes
Phillip
Hi Atharva
On 03/04/2021 14:16, Atharva Raykar wrote:
quoted hunk
Add a diff driver for Scheme-like languages which recognizes top level
and local `define` forms, whether it is a function definition, binding,
syntax definition or a user-defined `define-xyzzy` form.
Also supports R6RS `library` forms, `module` forms along with class and
struct declarations used in Racket (PLT Scheme).
Alternate "def" syntax such as those in Gerbil Scheme are also
supported, like defstruct, defsyntax and so on.
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 it is common practice to extend syntax with variants of a form
like `module+`, `class*` etc, those have been supported as well.
The word regex is a best-effort attempt to conform to R6RS[1] valid
identifiers, symbols and numbers.
[1] http://www.r6rs.org/final/html/r6rs/r6rs-Z-H-7.html#node_chap_4
Signed-off-by: Atharva Raykar <redacted>
[...]
@@ -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|def(struct|syntax|class|method|rules|record|proto|alias)?)[-*/ \t]|(library|module|struct|class)[*+ \t]).*)$",+/* All words should be delimited by spaces or parentheses */+"([^][)(}{[ \t])+"),
I think it would be nice to match single '(' and '[' to highlight when they have been added or deleted - I find this useful when I get a syntax error. Also it would be nice to handle r7rs identifiers like | this is a symbol |. Maybe something like
"(\\|([^\\\\|]*(\\\\|)*)*\\||[^][}{)( \t]|[][(){}])"
Best Wishes
Phillip
From: Johannes Sixt <hidden> Date: 2021-04-05 17:58:34
Am 05.04.21 um 12:04 schrieb Phillip Wood:
Hi Atharva
On 30/03/2021 11:22, Atharva Raykar wrote:
quoted
quoted
On 30-Mar-2021, at 12:34, Atharva Raykar [off-list ref] wrote:
quoted
On 29-Mar-2021, at 15:48, Phillip Wood [off-list ref]
wrote:
Hi Atharva
On 28/03/2021 13:23, Atharva Raykar wrote:
quoted
On 28-Mar-2021, at 05:16, Johannes Sixt [off-list ref] wrote:
[...]
quoted
quoted
diff --git a/t/t4018/scheme-local-define
b/t/t4018/scheme-local-define
new file mode 100644
index 0000000000..90e75dcce8
--- /dev/null+++ b/t/t4018/scheme-local-define
@@ -0,0 +1,4 @@+(define (higher-order)+ (define local-function RIGHT
... this one, which is also indented and *is* marked as RIGHT.
In this test case, I was explicitly testing for an indented '(define'
whereas in the former, I was testing for the top-level
'(define-syntax',
which happened to have an internal define (which will inevitably
show up
in a lot of scheme code).
It would be nice to include indented define forms but including them
means that any change to the body of a function is attributed to the
last internal definition rather than the actual function. For example
(define (f arg)
(define (g x)
(+ 1 x))
(some-func ...)
;;any change here will have '(define (g x)' in the hunk header, not
'(define (f arg)'
The reason I went for this over the top level forms, is because
I felt it was useful to see the nearest definition for internal
functions that often have a lot of the actual business logic of
the program (at least a lot of SICP seems to follow this pattern).
The disadvantage is as you said, it might also catch trivial inner
functions and the developer might lose context.
Never mind this message, I had misunderstood the problem you were
trying to
demonstrate. I wholeheartedly agree with what you are trying to say, and
the indentation heuristic discussed does look interesting. I shall have a
glance at the RFC you linked in the other reply.
quoted
The disadvantage is as you said, it might also catch trivial inner
functions and the developer might lose context.
Feel free to disregard me misquoting you here. You did not say that (:
quoted
Another problem is it may match more trivial bindings, like:
(define (some-func things)
...
(define items '(eggs
ham
peanut-butter))
...)
What I have noticed *anecdotally* is that this is not common enough
to be too much of a problem, and local define bindings seem to be more
favoured in Racket than other Schemes, that use 'let' more often.
quoted
I don't think this can be avoided as we rely on regexs rather than
parsing the source so it is probably best to only match toplevel
defines.
The other issue with only matching top level defines is that a
lot of scheme programs are library definitions, something like
(library
(foo bar)
(export ...)
(define ...)
(define ...)
;; and a bunch of other definitions...
)
Only matching top level defines will completely ignore matching all
the definitions in these files.
That said, I still stand by the fact that only catching top level defines
will lead to a lot of definitions being ignored. Maybe the occasional
mismatch may be worth the gain in the number of function contexts being
detected?
I'm not sure that the mismatches will be occasional - every time you
have an internal definition in a function the hunk header will be wrong
when you change the main body of the function. This will affect grep
--function-context and diff -W as well as the normal hunk headers. The
problem is there is no way to avoid that and provide something useful in
the library example you have above. It would be useful to find some code
bases and diff the output of 'git log --patch' with and without the
leading whitespace match in the function pattern to see how often this
is a problem (i.e. when the funcnames do not match see which one is
correct).
--function-context is just one application of the function matcher. To
work properly with nested function definitions, it would have to
understand the nesting. But it does not; there is nothing that we can do
about it without a proper language parser. Therefore, the argument that
the matcher does not work well with --function-context for nested
functions is of little relevance.
IMO, the primary concern should be whether the matcher decorates hunk
contexts sufficiently well.
-- Hannes
On 05-Apr-2021, at 15:51, Phillip Wood [off-list ref] wrote:
Hi Atharva
On 03/04/2021 14:16, Atharva Raykar wrote:
quoted
Add a diff driver for Scheme-like languages which recognizes top level
and local `define` forms, whether it is a function definition, binding,
syntax definition or a user-defined `define-xyzzy` form.
Also supports R6RS `library` forms, `module` forms along with class and
struct declarations used in Racket (PLT Scheme).
Alternate "def" syntax such as those in Gerbil Scheme are also
supported, like defstruct, defsyntax and so on.
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 it is common practice to extend syntax with variants of a form
like `module+`, `class*` etc, those have been supported as well.
The word regex is a best-effort attempt to conform to R6RS[1] valid
identifiers, symbols and numbers.
[1] http://www.r6rs.org/final/html/r6rs/r6rs-Z-H-7.html#node_chap_4
Signed-off-by: Atharva Raykar <redacted>
[...]
@@ -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|def(struct|syntax|class|method|rules|record|proto|alias)?)[-*/ \t]|(library|module|struct|class)[*+ \t]).*)$",+/* All words should be delimited by spaces or parentheses */+"([^][)(}{[ \t])+"),
I think it would be nice to match single '(' and '[' to highlight when they have been added or deleted - I find this useful when I get a syntax error. Also it would be nice to handle r7rs identifiers like | this is a symbol |. Maybe something like
"(\\|([^\\\\|]*(\\\\|)*)*\\||[^][}{)( \t]|[][(){}])"
My patch seems to detect additions and removals of singular parentheses
already -- I am not sure why it works, but my suspicion is that the
userdiff code seems to fall back to some default rules for additions and
removals that do not match the current word regex? Either way that seems
to work.
As for the R7RS identifiers, I can definitely add that, thanks for
pointing that out!
@@ -0,0 +1,4 @@+(define (higher-order)+ (define local-function RIGHT
... this one, which is also indented and *is* marked as RIGHT.
In this test case, I was explicitly testing for an indented '(define'
whereas in the former, I was testing for the top-level '(define-syntax',
which happened to have an internal define (which will inevitably show up
in a lot of scheme code).
It would be nice to include indented define forms but including them means that any change to the body of a function is attributed to the last internal definition rather than the actual function. For example
(define (f arg)
(define (g x)
(+ 1 x))
(some-func ...)
;;any change here will have '(define (g x)' in the hunk header, not '(define (f arg)'
The reason I went for this over the top level forms, is because
I felt it was useful to see the nearest definition for internal
functions that often have a lot of the actual business logic of
the program (at least a lot of SICP seems to follow this pattern).
The disadvantage is as you said, it might also catch trivial inner
functions and the developer might lose context.
Never mind this message, I had misunderstood the problem you were trying to
demonstrate. I wholeheartedly agree with what you are trying to say, and
the indentation heuristic discussed does look interesting. I shall have a
glance at the RFC you linked in the other reply.
quoted
The disadvantage is as you said, it might also catch trivial inner
functions and the developer might lose context.
Feel free to disregard me misquoting you here. You did not say that (:
quoted
Another problem is it may match more trivial bindings, like:
(define (some-func things)
...
(define items '(eggs
ham
peanut-butter))
...)
What I have noticed *anecdotally* is that this is not common enough
to be too much of a problem, and local define bindings seem to be more
favoured in Racket than other Schemes, that use 'let' more often.
quoted
I don't think this can be avoided as we rely on regexs rather than parsing the source so it is probably best to only match toplevel defines.
The other issue with only matching top level defines is that a
lot of scheme programs are library definitions, something like
(library
(foo bar)
(export ...)
(define ...)
(define ...)
;; and a bunch of other definitions...
)
Only matching top level defines will completely ignore matching all
the definitions in these files.
That said, I still stand by the fact that only catching top level defines
will lead to a lot of definitions being ignored. Maybe the occasional
mismatch may be worth the gain in the number of function contexts being
detected?
I'm not sure that the mismatches will be occasional - every time you have an internal definition in a function the hunk header will be wrong when you change the main body of the function. This will affect grep --function-context and diff -W as well as the normal hunk headers. The problem is there is no way to avoid that and provide something useful in the library example you have above. It would be useful to find some code bases and diff the output of 'git log --patch' with and without the leading whitespace match in the function pattern to see how often this is a problem (i.e. when the funcnames do not match see which one is correct).
You are right -- on trying out the function on a two other scheme
codebases, I noticed that there are a lot more wrongly matched functions
than I initially thought. About half of them identify the wrong function
in one of the repositories I tried. However, removing the leading
whitespace in the pattern did not lead to better matching; it just led
to a lot of the hunk headers going blank. I am not sure what causes this
behaviour, but my guess is that the function contexts are shown only if
it is within a certain distance from the function definition?
Even if it did match only the top level defines correctly, the functions
matched would still often be technically wrong -- it will show the outer
function as the context when the user has edited an internal function
(and in Scheme, there is heavy usage of internal functions).
After running 'git grep --function-context' with the leading whitespace
removed, it seems to match too aggressively, as it captures a huge
region to match all the way upto the top level. Especially for files
where all the definitions are in a 'library'.
Overall, I personally felt that there were more downsides to matching
only at the top level. I'd rather the hunk header have the nearest
function to provide the context, than have no function displayed at all.
Even when the match is wrong, it at least helps me locate where the
change was made more easily.
@@ -0,0 +1,4 @@+(define (higher-order)+ (define local-function RIGHT
... this one, which is also indented and *is* marked as RIGHT.
In this test case, I was explicitly testing for an indented '(define'
whereas in the former, I was testing for the top-level '(define-syntax',
which happened to have an internal define (which will inevitably show up
in a lot of scheme code).
It would be nice to include indented define forms but including them means that any change to the body of a function is attributed to the last internal definition rather than the actual function. For example
(define (f arg)
(define (g x)
(+ 1 x))
(some-func ...)
;;any change here will have '(define (g x)' in the hunk header, not '(define (f arg)'
The reason I went for this over the top level forms, is because
I felt it was useful to see the nearest definition for internal
functions that often have a lot of the actual business logic of
the program (at least a lot of SICP seems to follow this pattern).
The disadvantage is as you said, it might also catch trivial inner
functions and the developer might lose context.
Never mind this message, I had misunderstood the problem you were trying to
demonstrate. I wholeheartedly agree with what you are trying to say, and
the indentation heuristic discussed does look interesting. I shall have a
glance at the RFC you linked in the other reply.
quoted
The disadvantage is as you said, it might also catch trivial inner
functions and the developer might lose context.
Feel free to disregard me misquoting you here. You did not say that (:
quoted
Another problem is it may match more trivial bindings, like:
(define (some-func things)
...
(define items '(eggs
ham
peanut-butter))
...)
What I have noticed *anecdotally* is that this is not common enough
to be too much of a problem, and local define bindings seem to be more
favoured in Racket than other Schemes, that use 'let' more often.
quoted
I don't think this can be avoided as we rely on regexs rather than parsing the source so it is probably best to only match toplevel defines.
The other issue with only matching top level defines is that a
lot of scheme programs are library definitions, something like
(library
(foo bar)
(export ...)
(define ...)
(define ...)
;; and a bunch of other definitions...
)
Only matching top level defines will completely ignore matching all
the definitions in these files.
That said, I still stand by the fact that only catching top level defines
will lead to a lot of definitions being ignored. Maybe the occasional
mismatch may be worth the gain in the number of function contexts being
detected?
I'm not sure that the mismatches will be occasional - every time you have an internal definition in a function the hunk header will be wrong when you change the main body of the function. This will affect grep --function-context and diff -W as well as the normal hunk headers. The problem is there is no way to avoid that and provide something useful in the library example you have above. It would be useful to find some code bases and diff the output of 'git log --patch' with and without the leading whitespace match in the function pattern to see how often this is a problem (i.e. when the funcnames do not match see which one is correct).
You are right -- on trying out the function on a two other scheme
codebases, I noticed that there are a lot more wrongly matched functions
than I initially thought. About half of them identify the wrong function
in one of the repositories I tried. However, removing the leading
whitespace in the pattern did not lead to better matching; it just led
to a lot of the hunk headers going blank. I am not sure what causes this
behaviour, but my guess is that the function contexts are shown only if
it is within a certain distance from the function definition?
Even if it did match only the top level defines correctly, the functions
matched would still often be technically wrong -- it will show the outer
function as the context when the user has edited an internal function
(and in Scheme, there is heavy usage of internal functions).
After running 'git grep --function-context' with the leading whitespace
removed, it seems to match too aggressively, as it captures a huge
region to match all the way upto the top level. Especially for files
where all the definitions are in a 'library'.
Overall, I personally felt that there were more downsides to matching
only at the top level. I'd rather the hunk header have the nearest
function to provide the context, than have no function displayed at all.
Even when the match is wrong, it at least helps me locate where the
change was made more easily.
Thanks for taking the time to check the differences between the two approaches, as there is no perfect solution I'm happy to go with the one that seemed to be best in your investigations
Best Wishes
Phillip
Add a diff driver for Scheme-like languages which recognizes top level
and local `define` forms, whether it is a function definition, binding,
syntax definition or a user-defined `define-xyzzy` form.
Also supports R6RS `library` forms, `module` forms along with class and
struct declarations used in Racket (PLT Scheme).
Alternate "def" syntax such as those in Gerbil Scheme are also
supported, like defstruct, defsyntax and so on.
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 it is common practice to extend syntax with variants of a form
like `module+`, `class*` etc, those have been supported as well.
The word regex is a best-effort attempt to conform to R7RS[1] valid
identifiers, symbols and numbers.
[1] https://small.r7rs.org/attachment/r7rs.pdf (section 2.1)
Signed-off-by: Atharva Raykar <redacted>
---
Documentation/gitattributes.txt | 2 ++
t/t4018-diff-funcname.sh | 1 +
t/t4018/scheme-class | 7 +++++++
t/t4018/scheme-def | 4 ++++
t/t4018/scheme-def-variant | 4 ++++
t/t4018/scheme-define-slash-public | 7 +++++++
t/t4018/scheme-define-syntax | 8 ++++++++
t/t4018/scheme-define-variant | 4 ++++
t/t4018/scheme-library | 11 +++++++++++
t/t4018/scheme-local-define | 4 ++++
t/t4018/scheme-module | 6 ++++++
t/t4018/scheme-top-level-define | 4 ++++
t/t4018/scheme-user-defined-define | 6 ++++++
t/t4034-diff-words.sh | 1 +
t/t4034/scheme/expect | 11 +++++++++++
t/t4034/scheme/post | 6 ++++++
t/t4034/scheme/pre | 6 ++++++
userdiff.c | 9 +++++++++
18 files changed, 101 insertions(+)
create mode 100644 t/t4018/scheme-class
create mode 100644 t/t4018/scheme-def
create mode 100644 t/t4018/scheme-def-variant
create mode 100644 t/t4018/scheme-define-slash-public
create mode 100644 t/t4018/scheme-define-syntax
create mode 100644 t/t4018/scheme-define-variant
create mode 100644 t/t4018/scheme-library
create mode 100644 t/t4018/scheme-local-define
create mode 100644 t/t4018/scheme-module
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
@@ -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.
@@ -0,0 +1,11 @@+<BOLD>diff --git a/pre b/post<RESET>+<BOLD>index 74b6605..63b6ac4 100644<RESET>+<BOLD>--- a/pre<RESET>+<BOLD>+++ b/post<RESET>+<CYAN>@@ -1,6 +1,6 @@<RESET>+(define (<RED>myfunc a b<RESET><GREEN>my-func first second<RESET>)+ ; This is a <RED>really<RESET><GREEN>(moderately)<RESET> cool function.+ (<RED>this\place<RESET><GREEN>that\place<RESET> (+ 3 4))+ (define <RED>some-text<RESET><GREEN>|a greeting|<RESET> "hello")+ (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>))))
@@ -0,0 +1,6 @@+(define (my-func first second)+ ; This is a (moderately) cool function.+ (that\place (+ 3 4))+ (define |a greeting| "hello")+ (let ((c (add1 first)))+ (format "one more than the total is %d" (+ c second))))
@@ -0,0 +1,6 @@+(define (myfunc a b)+ ; This is a really cool function.+ (this\place (+ 3 4))+ (define some-text "hello")+ (let ((c (+ a b)))+ (format "one more than the total is %d" (add1 c))))
@@ -191,6 +191,15 @@ 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|def(struct|syntax|class|method|rules|record|proto|alias)?)[-*/ \t]|(library|module|struct|class)[*+ \t]).*)$",+/*+*R7RSvalididentifiersincludeanysequenceenclosed+*withinverticallineshavingnobackslashes+*/+"\\|([^\\\\]*)\\|"+/* All other words should be delimited by spaces or parentheses */+"|([^][)(}{[ \t])+"),PATTERNS("bibtex","(@[a-zA-Z]{1,}[ \t]*\\{{0,1}[ \t]*[^ \t\"@',\\#}{~%]*).*$","[={}\"]|[^={}\"\t]+"),PATTERNS("tex","^(\\\\((sub)*section|chapter|part)\\*{0,1}\\{.*)$",