`define` vs `define/private` inside `class`

[rfindler]

An easy to overlook error is when folks write define in a class body to define a function, using per-instance storage instead of per-class storage. It would be great if resyntax offered to fix this; there have, historically, been many of these bugs in the framework and in drracket; it is really hard to see them (especially when you're looking at a giant class; you kind of forget that you're in a class at all!).

For example, this is bad:

(define conan%
  (class object%
    (define (kick-soccer-ball) ...)
    (define (dart-mori-and-reveal-culprit) ...)
    (super-new)))

as each object will have a slot for kick-soccer-ball and dart-mori and instead it should have been:

(define conan%
  (class object%
    (define/private (kick-soccer-ball) ...)
    (define/private (dart-mori-and-reveal-culprit) ...)
    (super-new)))

The is a subtle point, however, as sometimes private fields are bound to functions intentionally as they might be mutated. Even more subtle, sometimes the functions that are on private fields are used with the framework preference library to cause preference callbacks to be garbage collected when an object is itself no longer reachable. The field font-size-callback in drracket/private/tooltip is an example.

I think a reasonable rule would be to suggest changing define to define/private in the body of a class when the field is immediately a lambda (or (define (f x) ...) notation was used) and all occurrences of the defined variable appear only in the function position of an application. That would avoid having resyntax make incorrect suggestions in the example in the previous paragraph (and other, similar situations).

[jackfirth]

Could you give an example of a bug that this caused?

[rfindler]

https://github.com/racket/drracket/commit/4b80dfb14e8a68c8c9093530c0e99e8ea8c9ee05 and https://github.com/racket/drracket/commit/85e2514c43fbbfd300f41d580cd58d6711024967