jackfirth / resyntax

A Racket refactoring engine
Apache License 2.0
56 stars 10 forks source link

refactoring let and curried define #179

Closed spdegabrielle closed 1 year ago

spdegabrielle commented 1 year ago

I tried resyntax analyze --file coroutines-example.rkt on

(define (resume-maker update-proc!)
  (λ (next-coroutine value)
    (let ([receiver (λ (continuation)
                      (update-proc! continuation)
                      (next-coroutine value))])
      (call-with-current-continuation receiver))))

but it made no suggestions:

% resyntax analyze --file coroutines-example.rkt 
resyntax: --- analyzing code ---
resyntax: --- displaying results ---
% 

I was expecting the default rule would turn the (let ([])) into a define. I think this is the default rule https://github.com/jackfirth/resyntax/blob/master/default-recommendations/private/let-binding.rkt

I could not find a refactoring for the curried form of define. I'm wondering if this is what stopped the `let' refactoring?

This is my manual attempt using the 'Choosing the right construct' section of the style guide

(define ((rresume-maker update-proc!) next-coroutine value) 
  (define (receiver continuation)
    (update-proc! continuation)
    (next-coroutine value))
  (call-with-current-continuation receiver))
spdegabrielle commented 1 year ago

another example

(define (coroutine-maker proc)
  (let ([saved-continuation '()])
    (let ([update-continuation! (λ (v)
                                  (displayln "updating")
                                  (set! saved-continuation v))])
      (let ([resumer (rresume-maker update-continuation!)] [first-time #t])
        (λ (value)
          (cond
            [first-time
             (set! first-time #f)
             (proc resumer value)]
            [else (saved-continuation value)]))))))

my translation

(define (rcoroutine-maker proc)
  (define saved-continuation '())
  (define (update-continuation! v)
    (displayln "updating")
    (set! saved-continuation v))
  (define resumer (rresume-maker update-continuation!))
  (define first-time #t)
  (define (f value)
    (cond
      [first-time
       (set! first-time #f)
       (proc resumer value)]
      [else (saved-continuation value)]))
  f)
jackfirth commented 1 year ago

Do your files have #lang racket/base at the top? I just wrote a test checking the first example you gave, and Resyntax refactors it to this:

(define (resume-maker update-proc!)
  (λ (next-coroutine value)
    (define (receiver continuation)
      (update-proc! continuation)
      (next-coroutine value))
    (call-with-current-continuation receiver)))

I actually disabled some of the curried-define refactorings because they have a good chance of making the function header too long or making it multi-line. That tends to look worse than the explicit lambda does - or at least, it looks worse often-enough that I don't think it's a good idea to make Resyntax go out of its way to rewrite it every time it occurs.

jackfirth commented 1 year ago

I'm gonna close this for now since I can't reproduce it, but let me know if it remains an issue.

spdegabrielle commented 1 year ago

My apologies @jackfirth When my sample program started with #lang Racket - it is clear in the documentation for resyntax that it is for #lang racket/base.

resyntax: coroutines-example2.rkt [define-lambda-to-define]

  The `define` form supports a shorthand for defining functions (including function-returning functions).

  18 (define (resume-maker update-proc!)
  19   (λ (next-coroutine value)
  20     (let ([receiver (λ (continuation)
  21                       (update-proc! continuation)
  22                       (next-coroutine value))])
  23       (call-with-current-continuation receiver))))

  18 (define ((resume-maker update-proc!) next-coroutine value)
  19   (let ([receiver (λ (continuation)
  20                     (update-proc! continuation)
  21                     (next-coroutine value))])
  22     (call-with-current-continuation receiver)))
jackfirth commented 1 year ago

Yup, that looks right. Note that Resyntax won't try to combine the multiple possible refactorings together that it could apply to that code. You'd have to run resyntax fix multiple times to get it to the end state I posted above.