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

[Atharva Raykar <raykar.ath@gmail.com>]

> On 30-Mar-2021, at 12:34, Atharva Raykar <raykar.ath@gmail.com> wrote:
> 
> 
> 
>> On 29-Mar-2021, at 15:48, Phillip Wood <phillip.wood123@gmail.com> wrote:
>> 
>> Hi Atharva
>> 
>> On 28/03/2021 13:23, Atharva Raykar wrote:
>>> On 28-Mar-2021, at 05:16, Johannes Sixt <j6t@kdbg.org> wrote:
>>> [...]
>>>>> 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.

> 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.
> 
>> 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?