Libguestfs - Jul 2023 - [libnbd PATCH 6/7] golang: Improve whitespace style in generated bindings

On 7/27/23 19:27, Eric Blake wrote:
> On Thu, Jul 27, 2023 at 01:54:25PM +0200, Laszlo Ersek wrote:
>> On 7/26/23 19:50, Eric Blake wrote:
>>> Use Go-preferred whitespace: TAB for indent, and no space before
>>> opening '(', better use of blank lines.  These changes
reduce the size
>>> of a diff produced by gofmt, although there are still other places
>>> where we are not idiomatic in what we generate (mainly in lining up
>>> columns of = in const() blocks).
>>>
>>> Signed-off-by: Eric Blake <eblake at redhat.com>
>>> ---
>>>  generator/GoLang.ml | 244
++++++++++++++++++++++----------------------
>>>  1 file changed, 123 insertions(+), 121 deletions(-)
>>
>> Unfortunately:
>>
>> - This must have been an incredible amount of work for you.
> 
> Some automation involved (emacs macros are nice), but yeah, it's
> grunt-work.
> 
>>
>> - The many \t escape sequences make the generator source code harder to
>> read.
> 
> Agreed.  Maybe a better approach would be figuring out an optional
> argument to pr to insist on TAB instead of space indent?  Or, as
> mentioned elsewhere, not bothering with TAB at all in OCaml but
> instead figuring out how to run gofmt itself as part of the generation
> pipeline.
> 
>>
>> - This patch depends on patch#4 (minimally), so if you modify that one
>> like I've asked, it will create a conflict for this patch. :(
> 
> Yep, I've already encountered a number of conflicts as I shuffle
> around patches, trying to pick out which changes are more than just
> whitespace (such as the RBool change).  Not terribly difficult, but
> back in the grunt-work category.
> 
>>
>> Acked-by: Laszlo Ersek <lersek at redhat.com>
>>
>> Thanks
>> Laszlo
>>
>>>
>>> diff --git a/generator/GoLang.ml b/generator/GoLang.ml
>>> index 77dacadb..a38aa19f 100644
>>> --- a/generator/GoLang.ml
>>> +++ b/generator/GoLang.ml
>>> @@ -175,6 +175,7 @@ let
>>>  let print_binding (name, { args; optargs; ret; shortdesc })
>>>    let cname = camel_case name in
>>>
>>> +  pr "\n";
>>>    (* Tedious method of passing optional arguments in golang. *)
>>>    if optargs <> [] then (
>>>      pr "/* Struct carrying optional arguments for %s.
*/\n" cname;
>>> @@ -182,10 +183,10 @@ let
>>>      List.iter (
>>>        fun optarg ->
>>>          let fname = go_name_of_optarg optarg in
>>> -        pr "  /* %s field is ignored unless %sSet == true.
*/\n"
>>> +        pr "\t/* %s field is ignored unless %sSet == true.
*/\n"
>>>            fname fname;
> 
> I tried:
> 
>     +        pr "\t" ^ "/* %s field is ignored unless %sSet
== true. */\n"
>                fname fname;
> 
> File "GoLang.ml", line 186, characters 8-15:
> 186 |         pr "\t" ^ "/* %s field is ignored unless %sSet
== true. */\n"
>               ^^^^^^^
> Error: This expression has type unit but an expression was expected of type
>          string
> 
> and:
> 
> File "GoLang.ml", line 186, characters 11-71:
> 186 |         pr ("\t" ^ "/* %s field is ignored unless
%sSet == true. */\n")
>                 
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Error: This expression has type string but an expression was expected of
type
>          ('weak2 -> 'weak3 -> 'weak4, unit, string, unit)
format4 >            ('weak2 -> 'weak3 -> 'weak4, unit,
string, string, string, unit)
>            CamlinternalFormatBasics.format6
Sigh. :/

"pr" is declared like this [generator/utils.mli]:

  val pr : ('a, unit, string, unit) format4 -> 'a

The format string would normally be converted to a "format4" type,
which
is a polymorphic type, taking one type variable ('a).

It seems that this automatic conversion/construction, from string to
format4, is performed by a mechanism that is similar to
"Stdlib.format_of_string":

  https://v2.ocaml.org/api/Stdlib.html#1_Operationsonformatstrings

There are two catches however:

- Stdlib.format_of_string only works with string *literals*, according
to the documentation,

- Stdlib.format_of_string produces a format6, not a format4.

The documentation recommends "Scanf.format_from_string":

  https://v2.ocaml.org/api/Scanf.html#VALformat_from_string

so you could *theoretically* do something like

  printf (Scanf.format_from_string ("%d " ^ "%d\n")) 1 2

it still doesn't work though, because Scanf.format_from_string also
produces a format6 (like Stdlib.format_of_string does), but printf
expects a plain "format" (similarly to how "pr" expects
"format4", i.e.,
*not* format6).

So this appears unsolvable. "Scanf.format_from_string" produces a
format6, which takes six type variables, but "pr" expects
"format4",
which is much less generic:

  type ('a, 'b, 'c, 'd) format4 = ('a, 'b, 'c,
'c, 'c, 'd) format6

We'd have to find a way to "narrow" the format6 produced by
"Scanf.format_from_string" into a format4, and I don't think
that's
possible (the parameters are not values but types). And I couldn't find
a Printf module function that took a format6.

Ultimately open-coding the \t escape sequences seems the least intrusive...
> 
> so those are non-starters.  We can get more verbose with:
> 
>     +        pr "\t";
>     +        pr "/* %s field is ignored unless %sSet == true.
*/\n"
>                fname fname;
> 
> but that may not scale as nicely.  Go's use of TAB does not specify
> what tabstop it prefers;
well that I find unfathomable; how can any coding style mandate TABs for
indentation if it doesn't explain how wide a TAB should be? For example,
that makes "maximum line width" mostly impossible to define.
> our .editorconfig suggests using a tabstop of
> 4 (instead of the usual 8) when reading a .go file for less visual
> distraction, and which matches with GoLang.ml currently using 4-space
> indentation.
> 
> Rich, do you have any ideas on any better approach to take here?
> 
>>> -        pr "  %sSet bool\n" fname;
>>> -        pr "  %s " fname;
>>> +        pr "\t%sSet bool\n" fname;
>>> +        pr "\t%s    " fname;
> 
> I also debated about splitting this patch into two (yes, more
> gruntwork): one to address space before '(', and the other to
address
> leading indentation.  Lines like this would be touched by both
> patches if I split.
> 
I wouldn't insist on such a split. :)

Laszlo

On Thu, Jul 27, 2023 at 08:50:31PM +0200, Laszlo Ersek
wrote:
> > File "GoLang.ml", line 186, characters 11-71:
> > 186 |         pr ("\t" ^ "/* %s field is ignored unless
%sSet == true. */\n")
> >                 
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > Error: This expression has type string but an expression was expected
of type
> >          ('weak2 -> 'weak3 -> 'weak4, unit, string,
unit) format4 > >            ('weak2 -> 'weak3 ->
'weak4, unit, string, string, string, unit)
> >            CamlinternalFormatBasics.format6
> 
> Sigh. :/
> 
> "pr" is declared like this [generator/utils.mli]:
> 
>   val pr : ('a, unit, string, unit) format4 -> 'a
> 
> The format string would normally be converted to a "format4"
type, which
> is a polymorphic type, taking one type variable ('a).
You dug even deeper than me.  Thanks, even if it didn't yield a
trivial solution.
> Ultimately open-coding the \t escape sequences seems the least intrusive...
> 
> > 
> > so those are non-starters.  We can get more verbose with:
> > 
> >     +        pr "\t";
> >     +        pr "/* %s field is ignored unless %sSet == true.
*/\n"
> >                fname fname;
> > 
> > but that may not scale as nicely.  Go's use of TAB does not
specify
> > what tabstop it prefers;
> 
> well that I find unfathomable; how can any coding style mandate TABs for
> indentation if it doesn't explain how wide a TAB should be? For
example,
> that makes "maximum line width" mostly impossible to define.
https://go.dev/doc/effective_go#formatting

|  Some formatting details remain. Very briefly:
| 
| Indentation
|     We use tabs for indentation and gofmt emits them by default. Use spaces
only if you must.
| Line length
|     Go has no line length limit. Don't worry about overflowing a punched
card. If a line feels too long, wrap it and indent with an extra tab.

and no mention of tab width.  The Go community really expects you to
use gofmt without questions.
> 
> > our .editorconfig suggests using a tabstop of
> > 4 (instead of the usual 8) when reading a .go file for less visual
> > distraction, and which matches with GoLang.ml currently using 4-space
> > indentation.
> > 
> > Rich, do you have any ideas on any better approach to take here?
> > 
> >>> -        pr "  %sSet bool\n" fname;
> >>> -        pr "  %s " fname;
> >>> +        pr "\t%sSet bool\n" fname;
> >>> +        pr "\t%s    " fname;
> > 
> > I also debated about splitting this patch into two (yes, more
> > gruntwork): one to address space before '(', and the other to
address
> > leading indentation.  Lines like this would be touched by both
> > patches if I split.
> > 
> 
> I wouldn't insist on such a split. :)
The benefit of such a split: changing "int (r)" to "int(r)"
is
non-controversial, while changing "    line" to "\tline"
could be reverted
if we find a cleaner way to get pr to do indentation on our behalf.

There's also the idea that if the main thing that gofmt still
complains about is 4 spaces vs. TAB, we could use coreutils'
unexpand(1) on platforms where 'gofmt' is not installed (although
I'm
not sure unexpand is portable enough to consider it likely to be
installed on other systems).  The more I think about this, the more
I'm leaning towards injecting gofmt into the pipeline, especially
since Tage has already proposed injecting rustfmt into the pipeline.

Still, cleaning up the ' (' to be consistent with language idioms
seems worthwhile, even if I punt on the TAB issue.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

So the way I see this could work is:

  pr ~tab:8 "\n      %s\n" "foo"

would post-process the output to convert spaces to tabs (ie. output
"\n\tfoo\n").  If this was necessary for a whole language you could
do:

  let pr = pr ~tab:8 ;;

at the top of the OCaml generator file for that language.

I don't think it's necessary to do anything very complicated here to
implement this.  You'd just need to adjust this code to keep track of
how many spaces there had been before the current column and output \t
at the right time.  (I mean, it makes the 'pr' function even more
complicated, but ...)

https://gitlab.com/nbdkit/libnbd/-/blob/70329e9585297bc42cf3db3bf508263137dade8d/generator/utils.ml#L135

pr_wrap would also need an optional ?tab parameter, passed straight
through to pr.

But maybe we should just pass the output of the generator through
gofmt, which I believe was the original plan.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v