Skip to content

Make File_bindings.Expanded.t type more precise#2041

Merged
rgrinberg merged 1 commit intoocaml:masterfrom
rgrinberg:local-path-dst-file-bindings
Apr 10, 2019
Merged

Make File_bindings.Expanded.t type more precise#2041
rgrinberg merged 1 commit intoocaml:masterfrom
rgrinberg:local-path-dst-file-bindings

Conversation

@rgrinberg
Copy link
Copy Markdown
Member

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

Signed-off-by: Rudi Grinberg rudi.grinberg@gmail.com

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

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
@rgrinberg rgrinberg requested review from a user, aalekseyev and emillon April 10, 2019 08:42
@rgrinberg rgrinberg force-pushed the local-path-dst-file-bindings branch from 1cc9448 to f3990eb Compare April 10, 2019 08:43
@rgrinberg rgrinberg merged commit a72db2f into ocaml:master Apr 10, 2019
@aalekseyev
Copy link
Copy Markdown
Collaborator

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
Copy link
Copy Markdown
Member Author

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
Copy link
Copy Markdown
Collaborator

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
Copy link
Copy Markdown
Member Author

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
Copy link
Copy Markdown
Collaborator

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
Copy link
Copy Markdown
Member Author

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
Copy link
Copy Markdown
Collaborator

Sounds reasonable to me!

rgrinberg added a commit to rgrinberg/jbuilder that referenced this pull request May 5, 2019
Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
rgrinberg added a commit to rgrinberg/jbuilder that referenced this pull request May 5, 2019
Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
rgrinberg added a commit to rgrinberg/jbuilder that referenced this pull request May 5, 2019
Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
rgrinberg added a commit that referenced this pull request May 6, 2019
Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
rgrinberg added a commit that referenced this pull request May 6, 2019
Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
mlasson pushed a commit to mlasson/dune that referenced this pull request Jul 19, 2019
Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants