Make File_bindings.Expanded.t type more precise

[rgrinberg]

The dst is a local path, so it should be reflected as such

Signed-off-by: Rudi Grinberg [email protected]

[aalekseyev]

I have to say that I don't like that Path.Local.t has a dual meaning of

1. a path relative to Path.root
2. a relative path with unspecified root

(and Path.t seems to have even more meanings)

The doc comment supports the first meaning, but in this feature you're using the second. The API is of split mind about this too:

In support of meaning (1):

  type Kind.t = private
    | External of External.t
    | Local    of Local.t

  (* this instead of [of_parts] and [append]: *)
  val Local.L.relative : ?error_loc:Loc0.t -> t -> string list -> t 

  val of_local : Local.t -> t

In support of meaning (2):

val append_local : t -> Local.t -> t
val append : t -> t -> t (* indirectly; also, this seems a bit crazy *)
val split_first_component : t -> (string * t) option (* indirectly *)
val local_part : t -> Local.t (* but this is weird *)

My personal opinion is that Path.Local.t should have meaning (1) and meaning (2) should have a different name (e.g. Path.Relative.t).

[rgrinberg]

Yeah, indeed it's a bit confusing. But note that in this case, the relative path returned by the File_binding.t is not allowed to escape root. So the use case is valid.

I support the introduction of Path.Relative.t to clarify the distinction.

[aalekseyev]

Allowing to escape or not is not a difference I have in mind. I would expect neither local path nor relative (when used relative to local) to be able to escape the root.

[rgrinberg]

I see. Would it make sense to change the path module to be something like:

type local =
  | In_build_dir of Relative.t
  | In_source of Relative.t
type t =
  | Local of local
  | External of External.t

[aalekseyev]

Yeah, sounds like a reasonable start. (it might equally make sense to have separate type for In_build_dir and In_source too, but one thing at a time)

[rgrinberg]

Looking at this again, it seems like (1) is essentially equivalent to In_source_tree.

So I think it would be better to have something like this:

type relative (* relative paths with an unspecified root *)
type in_build (* paths in the build dir *)
type in_source (* relative to Path.root. It should not be possible to access the build dir this way *)
type managed =
  | In_build of in_build
  | In_source of in_source
type external (* external paths that dune is not allowed to edit *)
type t =
  | Managed of managed
  | External of external

[aalekseyev]

Sounds reasonable to me!