Fix format leaks memory

[iwami4438]

• Changed to use the internal function format* instead of format (name change only).
• Updated calls to the existing format interface to use format* (name change only).
• Configured the code to call format* when there is a single argument, and fmt otherwise.
• Fixes format leaks memory #1432

[hellerve]

That does not fix the issue, it just shifts it from format to format*. See, for instance (format* "%d%d" 1). format is intentionally the single argument version, if we want to make it safer, we should make all implementors check the format string.

[iwami4438]

Thank you for the review. I overlooked the fact that format* could be called directly, bypassing the check, sorry.

I'll revert this and intercept each type's implementation instead, e.g.:

(defmodule Int
  (private format*)
  (hidden format*)
  (register format* (Fn [&String Int] String) "Int_format")

  (defn format [s x]
    (let [n (count-specifiers s)]
      (if (= n 1)
        (format* s x)
        (let [msg (String.append "error in format string: expected exactly 1 specifier, found "
                                   &(Int.str n))]
          (do
            (IO.errorln &msg)
            msg)))))
  (implements format Int.format))

Small question: I intend to define a helper function called count-specifiers. count-specifiers needs to be called from every type module, so it ends up effectively global (can't be private). Is that okay? My concern is simply that count-specifiers could be called from anywhere, such as the REPL.

[hellerve]

So the issue with this is that it’s a runtime check (which we try to avoid, which is why we do it at compile time in fmt). Maybe we can try to do it in a macro there, too?

I can sketch out what I mean in case it’s unclear.

[iwami4438]

I don't yet have a concrete idea of ​​the implementation, so it would be helpful if you could provide an overview (or a sketch).

My concerns are as follows:

• Since format is defined via definterface, the appropriate implementation is dispatched based on argument types. Because macros are expanded prior to type checking, I cannot envision a mechanism where the macro itself handles this dispatch logic.
• I tried simply shadowing (overwriting) format with a macro that calls itself:

(defmacro format [s x] (format s x))
(format "%d" 1) ;; Infinite loop

Once format is redefined as a macro, it seems there would be no way to access the original interface using that name (or perhaps it simply keeps overwriting itself?).

Specifically, the behavior of a defmacro inside a defmodule (e.g., (defmodule Int (defmacro format ...))) is unclear. Would it participate in the interface dispatch mechanism in some way, or would it simply become a namespaced macro accessible only as Int.format? In the latter case, I do not see how the compiler could determine which module's macro to use before the argument types are resolved.

[hellerve]

i just played around with it and have a working patch. A hint:

(hidden format-directives)
(doc format-directives "counts the format directives in `s`, ignoring escaped `%%`.")
(defndynamic format-directives [s]
   ; do cool stuff
)

(doc format-unchecked "formats `x` into `s` without checking the format string. Prefer `format`, which checks it at compile time.")
(defn format-unchecked [s x] (format s x))

(doc format "formats `x` into the format string `s`. `s` must contain exactly one format directive, and is required to be a string literal. Use `fmt` to format more than one value.")
(defmacro format [s x]
  (let [n (format-directives s)]
    (if (= n 1)
     `(format-unchecked %s %x)
      (macro-error (str "error in format string: expected exactly one directive, but found " n
          " in \"" s "\". use fmt to format more than one value")))))

my version also required load order to be changed such that the format file is loaded after String.carp.

[iwami4438]

Does anyone know how to catch macro-error? It seems there is no such method; is it safe to assume that not catching macro-error is acceptable in the current situation?

[eriksvedang]

Does anyone know how to catch macro-error? It seems there is no such method; is it safe to assume that not catching macro-error is acceptable in the current situation?

Yeah it's not possible to catch. I think that's acceptable.

[iwami4438]

OK.
This time, we'll remove the test that catches macro-error.

[iwami4438]

I removed the test that checked for macro-error, which was causing the CI to fail. Please review.

[hellerve]

as it stands right now, this doesn’t actually count the directives. it will return 2 for anything over 1 directive.