Support empty Location.input_name in input ast

[ryanartecona]

Fixes jaredly/reason-language-server#297

When using reason-language-server in a dune project, I started seeing the following error bubble up from the running language server instance when I introduced ppx_deriving.std:

Fatal error: exception (Invalid_argument Filename.chop_suffix)
Raised at file "parsing/ast_mapper.ml", line 826, characters 12-21
Called from file "parsing/ast_mapper.ml", line 843, characters 38-58
Called from file "parsing/ast_mapper.ml", line 878, characters 14-27
Called from file "src/driver.ml", line 1266, characters 6-39
Re-raised at file "parsing/location.ml", line 465, characters 14-25
Re-raised at file "parsing/location.ml", line 465, characters 14-25
Re-raised at file "parsing/location.ml", line 465, characters 14-25
Re-raised at file "parsing/location.ml", line 465, characters 14-25
Re-raised at file "parsing/location.ml", line 465, characters 14-25
Re-raised at file "parsing/location.ml", line 465, characters 14-25
Called from file "parsing/location.ml" (inlined), line 470, characters 31-61
Called from file "src/driver.ml", line 1271, characters 4-59
Called from file ".ppx/2a2600f2bea99fd7f9b28cf9261397f6/ppx.ml", line 1, characters 9-36
File "", line 1:
Error: Error while running external preprocessor
Command line: /Users/ryanartecona/code/axl/_build/default/.ppx/2a2600f2bea99fd7f9b28cf9261397f6/ppx.exe --as-ppx --cookie 'library-name="axl"' '/var/folders/6y/6ml47_vs4rgbh77xcb2vtzch0000gn/T/camlppx1912f1' '/var/folders/6y/6ml47_vs4rgbh77xcb2vtzch0000gn/T/camlppx90145c'

It wasn't straightforward to get here (dune collapsing multiple ppxes in use but not telling me which one an exception was raised from was annoying), but I eventually traced it to this module_from_input_name function in ppx_deriving.

The reason-language-server server uses refmt and ocamlc to do incremental error checking between saves while the user is working in a file. It turns out, when reason-language-server grabs the contents of an editor buffer, it first passes it straight to refmt (presumably to surface syntax errors first) which outputs a binary format AST, and next that AST file is passed to ocamlc. With a ppx like ppx_deriving in use, that ocamlc invocation will have a big hairy -ppx '...' argument, and it appears the ppx will be given whatever that prior refmt step output. In this case, since refmt was given buffer contents via stdin, it doesn't have a file name to put in its binary AST output, so the ppx sees a Location.input_name of "".

Adding this one match case for an empty string was the simplest thing that fixed my specific error. I don't know how sensitive the rest of ppx_deriving internals are to module_from_input_name generating something meaningful, but if it's not critical, it may be better to have an Invalid_argument _ -> [] exception handler in case that (basename (chop_suffix ...)) bit chokes on other nonsense. Unsure.

[gasche]

Looks good, thanks! I may also extend the patch to return [] in all the cases where chop_suffix fails, for extra robustness.

The inferred module name is used as the first component of the path : string list argument that is recursively passed to most derivers, and is supposed to contain a module path that goes from the toplevel environment (outside any module, no open) to the scope in which the derivation happens. For example, a [@@deriving ..] annotation on a type t within a submodule Foo of file.ml will be called with ~path:["File"; "Foo"]. In your case with the chop_suffix workaround, the path will only be Foo.

This path value is rarely used by the standard derivers plugins (I don't know about other plugins); it is used for example in the show plugin when with_path option is passed: if the t in my example above is defined as type t = Blob of int [@@deriving show { with_path = true ], then the show function on Blob 3 will show "File.Foo.Blob 3" instead of just "Blob 3" -- or just "Foo.Blob 3" in your specific workaround setting.

Overall I think that the fix is sensible: it passes some slightly wrong information to the derivers, but that will not matter much in the vast majority of cases, which is much better than failing with an error as you encountered. Note however that the "proper" fix would be for you to ensure that the tool you run have enough contextual information on the filename to work as intended (and not in degraded mode as here). Maybe there should be a way to pass the module name on ppx_deriving invocation, but I think that it should also be possible to fix the issue at the reason-language-server or refmt level by ensuring that the refmt output properly contains the intended buffer filename -- the binary-ast format accepted by the OCaml compiler (which then invokes the ppx binaries) specifically starts with a string for the filename.

[rgrinberg]

It wasn't straightforward to get here (dune collapsing multiple ppxes in use but not telling me which one an exception was raised from was annoying), but I eventually traced it to this module_from_input_name function in ppx_deriving.

This should be reported as a driver issue I believe. The driver has enough information to know which rewriter raised.

[gasche]

(See #198 for my minor follow-up PR.)

[ryanartecona]

Thanks all! And thanks @gasche especially for that generously detailed explanation.

This should be reported as a driver issue I believe. The driver has enough information to know which rewriter raised.

@rgrinberg by 'driver', do you mean in ppxlib or in the ocaml compiler? When I was digging around I saw some indication that dune is using ppxlib to squash multiple ppxes together, but I didn't get far enough to be certain that's the case or to know that's the one you mean.

[rgrinberg]

Ppxlib and omp. As it stands, both of these libraries provide a driver. Perhaps @diml should consider phasing out one of them :)