sorawee / fmt

A code formatter for Racket
Other
72 stars 6 forks source link

Inconvenient linebreak choice in match clause #30

Closed jackfirth closed 2 years ago

jackfirth commented 2 years ago

Formatting this code:

#lang racket

(define (eval exp env)
  (displayln exp)
  (match exp
    [(? symbol?) (unbox (hash-ref env exp))]
    [`(λ ,params ,@body) `(Function (,params ,@body) ,env)]
    [`(,fun ,@args) (apply (eval fun env) (map (lambda (arg) (eval arg env)) args))]))

(define (apply fun args)
  (match fun
    [`(Function (,params ,@body) ,env) (let loop ([params params] [args args] [env env])
                                         (displayln fun)
                                         (if (or (null? params) (null? args))
                                             ;(if (and (null? params) (null? args))
                                                 (eval body env)
                                                 ;(display "Too much or too little arguments given"))
                                             (loop (cdr params) (cdr args) (hash-set env (car params) (box (car args))))))]))

Produces this output:

#lang racket

(define (eval exp env)
  (displayln exp)
  (match exp
    [(? symbol?) (unbox (hash-ref env exp))]
    [`(λ ,params ,@body) `(Function (,params ,@body) ,env)]
    [`(,fun ,@args) (apply (eval fun env)
                           (map (lambda (arg) (eval arg env)) args))]))

(define (apply fun args)
  (match fun
    [`(Function (,params ,@body) ,env)
     (let loop ([params params] [args args] [env env])
       (displayln fun)
       (if (or (null? params) (null? args))
           ;(if (and (null? params) (null? args))
           (eval body env)
           ;(display "Too much or too little arguments given"))
           (loop (cdr params)
                 (cdr args)
                 (hash-set env (car params) (box (car args))))))]))

In that output, these two lines look off:

[`(,fun ,@args) (apply (eval fun env)
                       (map (lambda (arg) (eval arg env)) args))]))

I was expecting this:

[`(,fun ,@args)
 (apply (eval fun env) (map (lambda (arg) (eval arg env)) args))]))

or this:

[`(,fun ,@args)
 (apply (eval fun env)
        (map (lambda (arg) (eval arg env)) args))]))
sorawee commented 2 years ago

It seems like the rule that you want is that, for any [A B], format it as [A B] if both A and B are single line. Otherwise, format it as:

[A
 B]

Is that correct?

When I use this rule, I get the following diffs. I don't think some of these look ideal.

-                       (with-syntax ([(id ...) (syntax-local-bind-syntaxes (syntax->list #'(id ...))
-                                                                           #'rhs
-                                                                           def-ctx)])
+                       (with-syntax
+                           ([(id ...)
+                             (syntax-local-bind-syntaxes (syntax->list #'(id ...)) #'rhs def-ctx)])
                          (cons (copy-prop (syntax/loc e
                                             (define-syntaxes (id ...) rhs))
                                           'disappeared-use
-                    (let ([l (let ([l (syntax->list i)])
-                               (if (ormap (lambda (i) (free-identifier=? (car l) i))
-                                          (syntax-e (quote-syntax (-init -init-field -field))))
-                                   (cddr l)
-                                   (cdr l)))])
+                    (let ([l
+                           (let ([l (syntax->list i)])
+                             (if (ormap (lambda (i) (free-identifier=? (car l) i))
+                                        (syntax-e (quote-syntax (-init -init-field -field))))
+                                 (cddr l)
+                                 (cdr l)))])
                       (if alone
                           (map (lambda (i)
             (flat (match -head
                     [(? node?) fail]
-                    [_ (match tail
-                         ['() (hs-append (pretty -define) (format-head -head))]
-                         [(list -e) (hs-append (pretty -define) (format-head -head) (pretty -e))]
-                         [_ fail])]))))
+                    [_
+                     (match tail
+                       ['() (hs-append (pretty -define) (format-head -head))]
+                       [(list -e) (hs-append (pretty -define) (format-head -head) (pretty -e))]
+                       [_ fail])]))))
           ;; general case
           #;(define (a b)
jackfirth commented 2 years ago

The guideline I like is "prefer to linebreak at the highest level of syntactic nesting." Horizontal space is a lot more valuable than vertical space, especially when making edits to existing code.

The first diff looks great to me. In this code:

                      (with-syntax ([(id ...) (syntax-local-bind-syntaxes (syntax->list #'(id ...))
                                                                           #'rhs
                                                                           def-ctx)])

That syntax->list expression is incredibly painful to read and write. And if I have to edit any of it the lack of horizontal space means small changes could create big diffs. Though to be honest I'd probably use define/with-syntax instead of with-syntax or #:with if that's a syntax-parse macro.

For the second diff, I don't strongly prefer either style. But I would rather extract those subexpressions into define forms, which I think avoids the issue.

For the third diff, I'd write it that way from scratch, but given existing code I don't think it'd be worth the churn to change it.

jackfirth commented 2 years ago

Maybe only insert linebrakes into [A B] when A is longer than a few characters?

sorawee commented 2 years ago

Well, while I agree that the changing stuff like with-syntax with #:with from syntax/parse would be better, that's not in the scope of formatter's responsibility.

For now, I fix this issue by using the following rules:

For bindings (e.g., in let, parameterize, for, etc.), it allows [A B] even if B has several lines.

For clauses (e.g., match, syntax-parse, cond), it does not allow [A B] when B has several lines.

The guideline I like is "prefer to linebreak at the highest level of syntactic nesting." Horizontal space is a lot more valuable than vertical space, especially when making edits to existing code.

A formatter that implements this guideline is said to be greedy, and it can produce a really badly formatted code. There are numerous counterexamples in pretty printing literature for why greedy pretty printing is undesirable. fmt painstakingly avoids such greedy algorithm.

Human using this guideline might be OK, however, because you might subconsciously break this guideline where it makes sense without even noticing it (and so the guideline is only applied to cases where it doesn't really matter much for where to break).

jackfirth commented 2 years ago

Right, it's a soft guideline, not a hard rule. The only hard rule I like for formatting is the rectangle rule. Applying the guideline to clauses and not bindings makes sense to me since bindings usually have trivial left-hand sides.