Alias0.dir is always in build_dir #746
Conversation
src/build_system.ml
Outdated
| ((struct | ||
| type t = { dir : Path.t; name : string } | ||
| let make name ~dir = | ||
| assert (not (String.contains name '/')); |
There was a problem hiding this comment.
We can include this assert in the condition below
| @@ -222,19 +236,13 @@ module Alias0 = struct | |||
| if not (Path.is_in_build_dir path) then | |||
There was a problem hiding this comment.
Yeah, but then we're changing the user error message in die into something more opaque. I think it's still possible for the user to provide an invalid path here (though I'm not sure).
There was a problem hiding this comment.
Maybe when writing (alias /foo/bar)? Alias.of_path seems to be only used for that. I suggest to change of_path to of_path_written_by_user : loc:Loc.t -> ... and make it raise a properly located error.
There was a problem hiding this comment.
This is not directly related to this PR, but we might as well fix it once and for all
There was a problem hiding this comment.
Yeah, that's a good idea - though i think this is not a trivial change. The only way I can see how to do this is to change Dep_conf.t to preserve the locs. If it's that big, I might as well make it its own PR.
There was a problem hiding this comment.
The argument of Dep_conf.Alias is a String_with_vars.t which already contain a location
There was a problem hiding this comment.
Yeah, that wasn't too hard. Added a test for this error message too.
There was a problem hiding this comment.
Ok, looks good. One last thing: the checks done by M.make are redundant here since we already check that the path is in the build directory and Path.basename x is guaranted not to contain /. Can you move of_user_writen_path in M and create the t value directly?
src/build_system.ml
Outdated
| @@ -212,7 +212,21 @@ module File_spec = struct | |||
| end | |||
|
|
|||
| module Alias0 = struct | |||
| type t = { dir : Path.t; name : string } | |||
| include | |||
There was a problem hiding this comment.
I find this syntax slightly heavy. What about module M : sig ... end = struct ... end include M?
There was a problem hiding this comment.
Okay. I'll use T over M since that's what we do elsewhere as well.
|
I think using phantom types for types would be a bit heavy. After #744 we could change the type of |
rgrinberg
force-pushed
the
alias0-build-dir
branch
2 times, most recently
from
22e2f24 to
7050d06
Compare
the directory must always be inside the build dir, so we make sure that any way to create the record validates this invariant
And add a loc argument for correct error messages
rgrinberg
force-pushed
the
alias0-build-dir
branch
from
7050d06 to
ee97911
Compare
This is done to avoid double check of the path being in build dir
rgrinberg
force-pushed
the
alias0-build-dir
branch
from
ee97911 to
78612e0
Compare
This is useful to catch bugs in implementing #744
The 2nd commit feels a bit heavy handed but it's nice to have the extra safety of this. Another approach to help maintain this invariant would be to have some phantom types for paths to distinguish build vs. otherwise.