Prettify the output of Filename.quote{,_command} on Unix by only quoting when necessary

[kit-ty-kate]

Using Filename.quote and Filename.quote_command is the right thing to use when dealing with command line arguments passed to a shell (System.command, Unix.open_process*, …), however when someone cares about how the commands will look (e.g. logging, use by other projects, …) the commands will suddenly have ballooned in size and have quotes everywhere which makes reading them quite tedious at times.

Here i propose to quote command line arguments only when necessary by following the POSIX.1-2024 standard. If that matters, i'm expecting the resulting function to be being slightly faster if it contains more than 18 special characters, if the argument is empty, or when the argument doesn't need quoting (avoids a string copy), and probably slightly slower otherwise.

If there is interest in this work, i'm happy to also work on a version of this for Windows. If there is interest in this work but replacing the current behaviour of Filename.quote{,_command} doesn't seem like a good idea, i'm also happy to move it to a separate new Filename.pretty_quote function or something like that (or add a new ?pretty:bool argument)

[dbuenzli]

Also I think you added a dependency on Int to Filename this is why check typo is complaining, you need to do a make alldepend.

[dra27]

Both these functions specifically state that they return a quoted version of the argument, not a maybe-quoted version. I'm virtually certain there's code out there relying on that contract (because I'm fairly sure I wrote some of it).

I like the idea of having the function, but I think it has to be a separate function?

[dbuenzli]

I'm virtually certain there's code out there relying on that contract (because I'm fairly sure I wrote some of it).

FTR. It seems that I also wrote some of it.

[gasche]

(Note: Printf.sprintf "%S" is guaranteed to quote strings, and the content is escaped according to String.escaped which follows the OCaml lexical convention (rather than shell quoting rule).)

How do we want to deal with backward-compatibility? I see three approaches.

1. We can decide that "quoting" means "protecting the string for shell use", and in particular it does not necessarily imply adding double-quote characters, and so the code that currently assumes this is wrong and needs to use another function. This is the approach used in the current state of the PR.

2. We can introduce a Filename.maybe_quote command to be extra-explicit about this. This is fine, but then would Filename.quote_command switch to it? This is also a small breakage (given that currently it specifies that it uses Filename.quote), my intuition is that it is more manageable. (In other words, I think we could decide that Filename.quote must quote, but that Filename.quote_command may quote as its goal is specifically to produce well-formed commands and nothing else.)

3. We could introduce an optional parameter ?pretty:bool as suggested by @kit-ty-kate. I think that this could also endanger backward-compatibility by creating weird warnings or errors in higher-order usage condition, but we can test for this by building the opam-repository. (This change injects static errors, not dynamic errors, so it is easier to diagnose and fix impacted packages.)

In general I am not in favor of adding new optional parameters (my intuition is that it's not a good way to evolve APIs), but in this particular case (3) looks like the best appraoch of the three to me.

[dbuenzli]

I'm fine with an optional argument, at least it's not silent breakage (let us remind ourselves that the very act of adding functions to module is potential breakage). However I'd rather have ?always:bool or ?maybe:bool than ?pretty:bool.

[dra27]

The only concern is if Filename.quote is used as a string -> string value, as in:

foo
|> operation
|> operation
|> Filename.quote

[dbuenzli]

@dra27 actually it seems that this doesn't pose a problem:

# let f ?(quote = true) s = if quote then String.concat "" ["'";s;"'"] else s;;
val f : ?quote:bool -> string -> string = <fun>
# "bla" |> f;;
- : string = "'bla'"

If that comes as a surprise to you, you are not alone.

I personally was more concerned by List.map Filename.quote args but that works too. I wandered a bit randomly on sherlocode and couldn't find a breaking occurence (but some can certainly exist).

[gasche]

I believe that this would still work with an optional parameter:

OCaml version 5.4.0
# let f ?(x="default") s = (x, s);;
val f : ?x:string -> 'a -> string * 'a = <fun>
# "foo" |> (^) "bar" |> f;;
- : string * string = ("default", "barfoo")

[jberdine]

Just in case it isn't already clear, note that uses of |> aren't going to provide representative tests of the possible breakage for general higher-order usage. Since https://github.com/ocaml/ocaml/pull/10081, |> type-checks like application, and so dodges needing (some?) of the eta-expansions that adding an optional argument normally entails.

[dbuenzli]

So it seems that I actually don't have a good mental model of API breakage by optional argument addition.

Concretely functors applications may break (not widespread for that occurence I suspect). Which other higher-order usage may break (e.g. List.map does not) ?

[jberdine]

Regarding the question of what API breakage can result from adding an optional argument, I don't know that there is a short and clear answer. Enabling warning 48 [eliminated-optional-arguments] can be useful to explore cases where the type-checker is happy to eliminate extra optional arguments. But just what that condition is, looking at typecore, is non-trivial. In particular, eliminating extra optional arguments relies on knowing the expected type of an expression when type-checking it, and so the effectiveness of the optional argument elimination is sensitive to the effectiveness of type propagation. There is some related discussion: https://github.com/ocaml/ocaml/issues/5554, https://github.com/ocaml/ocaml/issues/5748, https://github.com/ocaml/ocaml/issues/6352, https://github.com/ocaml/ocaml/pull/10081.

But to be concrete, here is one contrived usage that would be broken by adding an optional argument:

module Filename_test : sig
  val quote : ?maybe:bool -> string -> string
end = struct
  let quote ?(maybe = false) s = Filename.quote s
end

let map_quote x =
  List.map (let f = Filename_test.quote in f) x
                                           ^
Error: The value f has type ?maybe:bool -> string -> string
       but an expression was expected of type 'a -> 'b
       The first argument is labeled ?maybe,
       but an unlabeled argument was expected

Examples using other syntactic forms for which is_inferred is false (e.g. match) will also be possible.