raise error if packages.toml syntax doesn't respect packagev1/packagev2

Summary:
We currently support both the PackageV1 and PackageV2 package syntax and semantics.  This diff adds a check to emit an error when parsing PACKAGES.toml if
- a package specification with a `uses` statement is encountered when PackageV2 is enabled
- a package specification with an `include_paths` statement is encountered when PackageV1 is enabled

Additionally, it adds a PackageV2 option to HHVM that will be later used to support PackageV2 specifications in HHVM.

Differential Revision: D64045784

fbshipit-source-id: 944c5d2bf8f525f0dcc093d988fb4af8b36cf7c1

hphp/doc/configs.specification

@@ -166,6 +166,10 @@ The format can be found in hphp/tools/configs/generate-configs.rs
 
   Constants from traits behave like constants from interfaces (error on conflict)
 
+- bool Eval.PackageV2 = false, UNKNOWN, globaldata|compileroption(PackageV2)
+
+  Enable support for the PackageV2 package specification.
+
 - std::string Eval.ActiveDeployment = "", UNKNOWN, globaldata|compileroption(ActiveDeployment)
 
   Describes the active deployment for selecting the set of packages

hphp/hack/src/client/ide_service/clientIdeDaemon.ml

@@ -357,8 +357,10 @@ let initialize1
   (* Ignore package loading errors for now TODO(jjwu) *)
   let open GlobalOptions in
   log "Loading package configuration";
-  let (_, package_info) = PackageConfig.load_and_parse () in
   let tco = ServerConfig.typechecker_options config in
+  let (_, package_info) =
+    PackageConfig.load_and_parse ~package_v2:tco.tco_package_v2 ()
+  in
   let config =
     ServerConfig.set_tc_options
       config

hphp/hack/src/hh_single_type_check.ml

@@ -2492,6 +2492,7 @@ let decl_and_run_mode
     | Some _ ->
       let (errors, info) =
         PackageConfig.load_and_parse
+          ~package_v2:tcopt.GlobalOptions.tco_package_v2
           ~strict:false
           ~pkgs_config_abs_path:packages_config_path
           ()

hphp/hack/src/package/config.rs

@@ -12,6 +12,7 @@ use toml::Spanned;
 use crate::error::Error;
 use crate::types::DeploymentMap;
 use crate::types::NameSet;
+pub use crate::types::Package;
 pub use crate::types::PackageMap;
 
 #[derive(Debug, Deserialize)]
@@ -20,7 +21,18 @@ pub struct Config {
     pub deployments: Option<DeploymentMap>,
 }
 impl Config {
-    pub fn check_config(&self, errors: &mut Vec<Error>) {
+    pub fn check_config(&self, package_v2: bool, errors: &mut Vec<Error>) {
+        let check_package_v2_fields =
+            |errors: &mut Vec<Error>, package_name: &Spanned<String>, package: &Package| {
+                if !package_v2 {
+                    if package.include_paths.is_some() {
+                        errors.push(Error::include_path_in_package_v1(package_name));
+                    }
+                } else if package.uses.is_some() {
+                    errors.push(Error::uses_in_package_v2(package_name));
+                }
+            };
+
         let check_packages_are_defined =
             |errors: &mut Vec<Error>, pkgs: &Option<NameSet>, soft_pkgs: &Option<NameSet>| {
                 if let Some(packages) = pkgs {
@@ -74,9 +86,10 @@ impl Config {
                     ));
                 }
             };
-        for (_, package) in self.packages.iter() {
+        for (package_name, package) in self.packages.iter() {
             check_packages_are_defined(errors, &package.includes, &package.soft_includes);
             check_each_glob_is_used_once(errors, &package.uses);
+            check_package_v2_fields(errors, package_name, package);
         }
         if let Some(deployments) = &self.deployments {
             for (positioned_name, deployment) in deployments.iter() {

hphp/hack/src/package/error.rs

@@ -34,6 +34,14 @@ pub enum Error {
         include_path: String,
         span: (usize, usize),
     },
+    IncludePathInPackageV1 {
+        package_name: String,
+        span: (usize, usize),
+    },
+    UsesInPackageV2 {
+        package_name: String,
+        span: (usize, usize),
+    },
 }
 
 impl Error {
@@ -83,13 +91,31 @@ impl Error {
         }
     }
 
+    pub fn include_path_in_package_v1(package_name: &Spanned<String>) -> Self {
+        let Range { start, end } = package_name.span();
+        Self::IncludePathInPackageV1 {
+            package_name: package_name.get_ref().into(),
+            span: (start, end),
+        }
+    }
+
+    pub fn uses_in_package_v2(package_name: &Spanned<String>) -> Self {
+        let Range { start, end } = package_name.span();
+        Self::UsesInPackageV2 {
+            package_name: package_name.get_ref().into(),
+            span: (start, end),
+        }
+    }
+
     pub fn span(&self) -> (usize, usize) {
         match self {
             Self::DuplicateUse { span, .. }
             | Self::UndefinedInclude { span, .. }
             | Self::IncompleteDeployment { span, .. }
             | Self::InvalidIncludePath { span, .. }
-            | Self::MalformedIncludePath { span, .. } => *span,
+            | Self::MalformedIncludePath { span, .. }
+            | Self::IncludePathInPackageV1 { span, .. }
+            | Self::UsesInPackageV2 { span, .. } => *span,
         }
     }
 
@@ -141,6 +167,16 @@ impl Display for Error {
                     include_path
                 )?;
             }
+            Self::IncludePathInPackageV1 { package_name, .. } => write!(
+                f,
+                "The `include_paths` field is not supported by PackageV1 in package {}",
+                package_name
+            )?,
+            Self::UsesInPackageV2 { package_name, .. } => write!(
+                f,
+                "The `uses` field is not supported by PackageV2 in package {}",
+                package_name
+            )?,
         };
         Ok(())
     }

hphp/hack/src/package/ffi_bridge/package_ffi.rs

@@ -25,6 +25,7 @@ mod ffi {
         uses: Vec<String>,
         includes: Vec<String>,
         soft_includes: Vec<String>,
+        include_paths: Vec<String>,
     }
     struct DeploymentMapEntry {
         name: String,
@@ -36,12 +37,12 @@ mod ffi {
         domains: Vec<String>,
     }
     extern "Rust" {
-        pub fn package_info(filename: &CxxString) -> PackageInfo;
+        pub fn package_info(package_v2: bool, filename: &CxxString) -> PackageInfo;
     }
 }
 
-pub fn package_info(filename: &CxxString) -> ffi::PackageInfo {
-    let s = package::PackageInfo::from_text_strict("", &filename.to_string());
+pub fn package_info(package_v2: bool, filename: &CxxString) -> ffi::PackageInfo {
+    let s = package::PackageInfo::from_text_strict(package_v2, "", &filename.to_string());
     match s {
         Ok(info) => {
             let convert = |v: Option<&package::NameSet>| {
@@ -56,6 +57,7 @@ pub fn package_info(filename: &CxxString) -> ffi::PackageInfo {
                         uses: convert(package.uses.as_ref()),
                         includes: convert(package.includes.as_ref()),
                         soft_includes: convert(package.soft_includes.as_ref()),
+                        include_paths: convert(package.include_paths.as_ref()),
                     };
                     ffi::PackageMapEntry {
                         name: name.get_ref().to_string(),

hphp/hack/src/package/ocaml_ffi/package_ocaml_ffi.rs

@@ -10,11 +10,12 @@ use oxidized::package_info_impl::Errors;
 
 ocaml_ffi! {
     fn extract_packages_from_text_ffi(
+        package_v2: bool,
         strict: bool,
         root: String,
         filename: String,
     ) -> Result<Vec<Package>, Errors> {
-        let info = match package::PackageInfo::from_text(strict, &root, &filename) {
+        let info = match package::PackageInfo::from_text(package_v2, strict, &root, &filename) {
             Ok(info) => info,
             // TODO(T148525961): Send a proper error when packages.toml fails to parse
             Err(_) => return Ok(vec![]),

hphp/hack/src/package/packageConfig.ml

@@ -10,7 +10,7 @@ open Hh_prelude
 type errors = (Pos.t * string * (Pos.t * string) list) list
 
 external extract_packages_from_text :
-  bool -> string -> string -> string -> (Package.t list, errors) result
+  bool -> bool -> string -> string -> string -> (Package.t list, errors) result
   = "extract_packages_from_text_ffi"
 
 let pkgs_config_path_relative_to_repo_root = "PACKAGES.toml"
@@ -21,10 +21,10 @@ let repo_config_path =
 let log_debug (msg : ('a, unit, string, string, string, unit) format6) : 'a =
   Hh_logger.debug ~category:"packages" ?exn:None msg
 
-let parse (strict : bool) (path : string) =
+let parse (package_v2 : bool) (strict : bool) (path : string) =
   let contents = Sys_utils.cat path in
   let root = Relative_path.(path_of_prefix Root) in
-  match extract_packages_from_text strict root path contents with
+  match extract_packages_from_text package_v2 strict root path contents with
   | Error errors ->
     let error_list =
       List.map errors ~f:(fun (pos, msg, reasons) ->
@@ -37,7 +37,8 @@ let parse (strict : bool) (path : string) =
     (Errors.from_error_list error_list, PackageInfo.empty)
   | Ok packages -> (Errors.empty, PackageInfo.from_packages packages)
 
-let load_and_parse ?(strict = true) ?(pkgs_config_abs_path = None) () :
+let load_and_parse
+    ~(package_v2 : bool) ?(strict = true) ?(pkgs_config_abs_path = None) () :
     Errors.t * PackageInfo.t =
   let pkgs_config_abs_path =
     match pkgs_config_abs_path with
@@ -47,6 +48,6 @@ let load_and_parse ?(strict = true) ?(pkgs_config_abs_path = None) () :
   if not @@ Sys.file_exists pkgs_config_abs_path then
     (Errors.empty, PackageInfo.empty)
   else
-    let result = parse strict pkgs_config_abs_path in
+    let result = parse package_v2 strict pkgs_config_abs_path in
     log_debug "Parsed %s" pkgs_config_abs_path;
     result

hphp/hack/src/package/packageConfig.mli

@@ -11,6 +11,7 @@
   * does not exist in the filesystem.
   *)
 val load_and_parse :
+  package_v2:bool ->
   ?strict:bool ->
   ?pkgs_config_abs_path:string option ->
   unit ->

hphp/hack/src/package/package_info.rs

@@ -24,7 +24,12 @@ pub struct PackageInfo {
 }
 
 impl PackageInfo {
-    pub fn from_text(strict: bool, root: &str, filename: &str) -> Result<PackageInfo> {
+    pub fn from_text(
+        package_v2: bool,
+        strict: bool,
+        root: &str,
+        filename: &str,
+    ) -> Result<PackageInfo> {
         let mut errors = vec![];
         let root = if !root.is_empty() {
             Path::new(root)
@@ -87,7 +92,7 @@ impl PackageInfo {
             }
         }
 
-        config.check_config(&mut errors);
+        config.check_config(package_v2, &mut errors);
 
         Ok(Self {
             packages: config.packages,
@@ -97,8 +102,8 @@ impl PackageInfo {
         })
     }
 
-    pub fn from_text_strict(root: &str, filename: &str) -> Result<PackageInfo> {
-        PackageInfo::from_text(true, root, filename)
+    pub fn from_text_strict(package_v2: bool, root: &str, filename: &str) -> Result<PackageInfo> {
+        PackageInfo::from_text(package_v2, true, root, filename)
     }
 
     pub fn packages(&self) -> &PackageMap {
@@ -148,7 +153,7 @@ mod test {
     #[test]
     fn test_parsing_basic_file() {
         let test_path = SRCDIR.as_path().join("tests/package-1.toml");
-        let info = PackageInfo::from_text(true, "", test_path.to_str().unwrap()).unwrap();
+        let info = PackageInfo::from_text(false, true, "", test_path.to_str().unwrap()).unwrap();
         assert!(info.errors.is_empty());
 
         let foo = &info.packages()["foo"];
@@ -193,7 +198,7 @@ mod test {
     #[test]
     fn test_multiline_uses() {
         let test_path = SRCDIR.as_path().join("tests/package-2.toml");
-        let info = PackageInfo::from_text(true, "", test_path.to_str().unwrap()).unwrap();
+        let info = PackageInfo::from_text(false, true, "", test_path.to_str().unwrap()).unwrap();
         assert!(info.errors.is_empty());
 
         let foo = &info.packages()["foo"];
@@ -211,7 +216,7 @@ mod test {
     #[test]
     fn test_config_errors1() {
         let test_path = SRCDIR.as_path().join("tests/package-3.toml");
-        let info = PackageInfo::from_text(true, "", test_path.to_str().unwrap()).unwrap();
+        let info = PackageInfo::from_text(false, true, "", test_path.to_str().unwrap()).unwrap();
         assert_eq!(info.errors.len(), 3);
         assert_eq!(info.errors[0].msg(), "Undefined package: baz");
         assert_eq!(info.errors[1].msg(), "Undefined package: baz");
@@ -224,7 +229,7 @@ mod test {
     #[test]
     fn test_config_errors2() {
         let test_path = SRCDIR.as_path().join("tests/package-4.toml");
-        let info = PackageInfo::from_text(true, "", test_path.to_str().unwrap()).unwrap();
+        let info = PackageInfo::from_text(false, true, "", test_path.to_str().unwrap()).unwrap();
         let errors = info
             .errors
             .iter()
@@ -244,7 +249,7 @@ mod test {
     #[test]
     fn test_soft() {
         let test_path = SRCDIR.as_path().join("tests/package-5.toml");
-        let info = PackageInfo::from_text(true, "", test_path.to_str().unwrap()).unwrap();
+        let info = PackageInfo::from_text(false, true, "", test_path.to_str().unwrap()).unwrap();
         let c = &info.packages()["c"];
         let errors = info
             .errors
@@ -272,7 +277,7 @@ mod test {
     #[test]
     fn test_include_paths1() {
         let test_path = SRCDIR.as_path().join("tests/package-6.toml");
-        let info = PackageInfo::from_text(true, "", test_path.to_str().unwrap()).unwrap();
+        let info = PackageInfo::from_text(true, true, "", test_path.to_str().unwrap()).unwrap();
         let included_dirs = info.packages()["foo"].include_paths.as_ref().unwrap();
         assert_eq!(included_dirs.len(), 2);
         assert!(
@@ -296,7 +301,7 @@ mod test {
     #[test]
     fn test_include_paths_error() {
         let test_path = SRCDIR.as_path().join("tests/package-6.toml");
-        let info = PackageInfo::from_text(true, "", test_path.to_str().unwrap()).unwrap();
+        let info = PackageInfo::from_text(true, true, "", test_path.to_str().unwrap()).unwrap();
 
         let errors = info.errors.iter().map(|e| e.msg()).collect::<Vec<_>>();
 
@@ -327,7 +332,7 @@ mod test {
     #[test]
     fn test_include_paths_non_strict() {
         let test_path = SRCDIR.as_path().join("tests/package-6.toml");
-        let info = PackageInfo::from_text(false, "", test_path.to_str().unwrap()).unwrap();
+        let info = PackageInfo::from_text(true, false, "", test_path.to_str().unwrap()).unwrap();
         let errors = info.errors.iter().map(|e| e.msg()).collect::<Vec<_>>();
         assert!(errors.len() == 3);
         // with non-strict PackageInfo parsing only "malformed path" errors should be generated
@@ -338,11 +343,9 @@ mod test {
 
     #[test]
     fn test_include_paths_error_2() {
-        println!(" !!! ");
         let test_path = SRCDIR.as_path().join("tests/package-7.toml");
-        let info = PackageInfo::from_text(false, "", test_path.to_str().unwrap()).unwrap();
+        let info = PackageInfo::from_text(true, false, "", test_path.to_str().unwrap()).unwrap();
         let errors = info.errors.iter().map(|e| e.msg()).collect::<Vec<_>>();
-        println!(" *** {:?}", errors);
         let expected = [
             String::from(
                 "include_path //doesnotexist/./bar/ is malformed: paths must start with // and cannot include ./ or ../, directories must end with /",
@@ -354,4 +357,26 @@ mod test {
         assert!(errors[0] == expected[0]);
         assert!(errors[1] == expected[1]);
     }
+
+    #[test]
+    fn test_uses_in_package_v2() {
+        let test_path = SRCDIR.as_path().join("tests/package-1.toml");
+        let info = PackageInfo::from_text(true, false, "", test_path.to_str().unwrap()).unwrap();
+        let errors = info.errors.iter().map(|e| e.msg()).collect::<Vec<_>>();
+        let expected = [String::from(
+            "The `uses` field is not supported by PackageV2 in package foo",
+        )];
+        assert!(errors[0] == expected[0]);
+    }
+
+    #[test]
+    fn test_include_path_in_package_v1() {
+        let test_path = SRCDIR.as_path().join("tests/package-6.toml");
+        let info = PackageInfo::from_text(false, false, "", test_path.to_str().unwrap()).unwrap();
+        let errors = info.errors.iter().map(|e| e.msg()).collect::<Vec<_>>();
+        let expected = [String::from(
+            "The `include_paths` field is not supported by PackageV1 in package bar",
+        )];
+        assert!(errors[4] == expected[0]);
+    }
 }

hphp/hack/src/package/tests/package-6.toml

@@ -1,14 +1,12 @@
 [packages]
 
 [packages.foo]
-uses = ["x.*"]
 include_paths=[
     "//doesnotexist.php",
     "//doesnotexist/",
 ] # well-formed but nonexistent paths
 
 [packages.bar]
-uses = ["y.*"]
 include_paths=[
     "bar",
     "//bar",