diff --git a/default-recommendations/one-line-reformatting-test.rkt b/default-recommendations/one-line-reformatting-test.rkt new file mode 100644 index 00000000..e85ffd7e --- /dev/null +++ b/default-recommendations/one-line-reformatting-test.rkt @@ -0,0 +1,24 @@ +#lang resyntax/test +header: - #lang racket/base + + +test: "regression test for issue #605" +---------------------------------------- +(require racket/match) +(define (f expr) + (let loop ([expr expr] + [env '()]) + (match expr + [`(,(and (or '+ '- '* '/ 'and 'or) op) ,as ..2 ,b) `(,op ,(loop `(,op ,@as) env) ,(loop b env))] + [_ (void)]))) +======================================== +(require racket/match) +(define (f expr) + (let loop ([expr expr] + [env '()]) + (match expr + [`(,(and (or '+ '- '* '/ 'and 'or) op) ,as ..2 ,b) (list op + (loop `(,op ,@as) env) + (loop b env))] + [_ (void)]))) +---------------------------------------- diff --git a/private/syntax-replacement.rkt b/private/syntax-replacement.rkt index ee7f375e..9edc9a1e 100644 --- a/private/syntax-replacement.rkt +++ b/private/syntax-replacement.rkt @@ -42,6 +42,7 @@ rebellion/collection/range-set rebellion/private/static-name rebellion/type/record + resyntax/private/linemap resyntax/private/logger resyntax/private/source resyntax/private/string-indent @@ -56,7 +57,9 @@ (module+ test - (require racket/port + (require fmt + racket/port + racket/string rackunit (submod ".."))) @@ -255,7 +258,20 @@ ;; horizontal space is available and indenting the resulting string. However, fmt has some odd ;; behavior in how it handles formatting regions with multiple expressions (sorawee/fmt#70) and ;; regions with commented top-level expressions (sorawee/fmt#68). - (define allowed-width (- (current-width) initial-columns)) + (define base-allowed-width (- (current-width) initial-columns)) + + ;; For single-line replacements, we need to account for trailing text after the replacement + ;; to ensure the formatted code doesn't exceed the line length limit. + (define trailing-text-length + (let ([linemap (string-linemap original)] + [orig-end (string-replacement-original-end replacement)]) + ;; Convert from 0-based string indices to 1-based positions for linemap + (if (= (linemap-position-to-line linemap (add1 start)) + (linemap-position-to-line linemap (add1 orig-end))) + (- (linemap-position-to-end-of-line linemap (add1 orig-end)) (add1 orig-end)) + 0))) + + (define allowed-width (- base-allowed-width trailing-text-length)) (define formatted-code-substring (string-hanging-indent (program-format changed-code-substring #:width allowed-width) #:amount initial-columns)) @@ -315,7 +331,59 @@ #:uses-universal-tagged-syntax? #false)) (define expected (string-replacement #:start 0 #:end 13 #:contents (list (inserted-string "(+ 1 2 3)")))) - (check-equal? (syntax-replacement-render replacement) expected))) + (check-equal? (syntax-replacement-render replacement) expected)) + + (test-case "string-replacement-format considers trailing text on single-line replacements" + ;; Test that when formatting a single-line replacement, we account for trailing text + ;; to avoid exceeding the line length limit. This test simulates the issue from + ;; herbie-fp/herbie#1391 where a quasiquote is replaced with a list call. + (define orig-code " [x `(a b c)]") + (define replacement + (string-replacement #:start 9 + #:end 18 + #:contents (list (inserted-string "(list a b c)")))) + ;; The original code is 18 characters. With trailing "]", it's 19 characters total. + ;; If we format without accounting for the trailing "]", the formatted result might + ;; exceed the desired line length. + (parameterize ([current-width 102]) + (define formatted (string-replacement-format replacement orig-code)) + (define result (string-apply-replacement orig-code formatted)) + ;; The result should fit within a reasonable line length and shouldn't be multiline + (check-false (string-contains? result "\n") + "Result should remain on a single line") + ;; Verify the replacement happened + (check-true (string-contains? result "(list a b c)") + "Result should contain the list form"))) + + (test-case "realistic example from herbie-fp/herbie#1391" + ;; This test simulates the actual issue where a 102-character line would become 103 characters + ;; Without the fix, the formatter would allow the code to exceed the line length limit. + ;; With the fix, the formatter either keeps it on one line within the limit, or breaks it + ;; across multiple lines if it cannot fit. + (define orig-line " [`(,(and (or '+ '- '* '/ 'and 'or) op) ,as ..2 ,b) `(,op ,(loop `(,op ,@as) env) ,(loop b env))]") + (check-equal? (string-length orig-line) 102 "Original line should be 102 characters") + + ;; The quasiquote expression starts at position 57 and ends at position 101 + ;; Original: `(,op ,(loop `(,op ,@as) env) ,(loop b env)) + ;; Replacement: (list op (loop `(,op ,@as) env) (loop b env)) + (define quasiquote-start 57) + (define quasiquote-end 101) ; just before the final ] + + (define replacement + (string-replacement #:start quasiquote-start + #:end quasiquote-end + #:contents (list (inserted-string "(list op (loop `(,op ,@as) env) (loop b env))")))) + + ;; Test with Racket's standard line width of 102 + (parameterize ([current-width 102]) + (define formatted (string-replacement-format replacement orig-line)) + (define result (string-apply-replacement orig-line formatted)) + + ;; If the result is a single line, it should not exceed 102 characters + ;; If it's multi-line, each line should not exceed 102 characters + (for ([line (in-list (string-split result "\n"))]) + (check-true (<= (string-length line) 102) + (format "Line exceeds length limit: ~a chars (should be <= 102)" (string-length line))))))) (define (syntax-replacement-introduces-incorrect-bindings? replacement)