Require an implicit package member directory to be a valid identifier

Summary:
# Context

Each direct child directory `D` of an implicit package family's `path` denotes a
synthesized member package named `F.D`. Any directory name was accepted, which
means a name that cannot work as a package name still produced one.

Dotted names are the case that actually breaks. A directory `proto.v1` yields the
member name `prototypes.proto.v1`, and resolving a name back to a family and
member splits on `.`, so the name does not round-trip cleanly.

# Solution

Require a member directory name to be a valid Hack identifier. A directory that
is not one denotes no member package, and a file inside it is reported with a new
`Naming[2136]`.

The predicate comes from the `hack_name` crate added earlier in this stack, so
the check shares the lexer's definition of a name rather than carrying a copy
that can drift.

Reviewed By: madgen

Differential Revision: D114386952

fbshipit-source-id: d94136b79671d51f2ecf27291a7950f522c1b593

Author: dlreeves

hphp/hack/src/Cargo.lock

@@ -91,6 +91,7 @@ members = [
     "utils/escaper",
     "utils/ffi_cbindgen",
     "utils/find_utils",
+    "utils/hack_name/cargo",
     "utils/hash",
     "utils/hh24_types",
     "utils/html_entities",

hphp/hack/src/Cargo.toml

@@ -174,6 +174,7 @@ module Naming = struct
     | NamedVariadicTypeDisallowed [@value 2133]
     | VariadicNamedParameterDisallowed [@value 2134]
     | ImplicitPackageFileDirectlyUnderPath [@value 2135]
+    | ImplicitPackageInvalidMemberDir [@value 2136]
   (* Add new Naming codes here! Comment out when deprecating. *)
   [@@deriving enum, show { with_path = false }]
 

hphp/hack/src/diagnostics/error_codes.ml

@@ -20,6 +20,7 @@ bstr = { version = "1.13.0", features = ["serde", "std", "unicode"] }
 bumpalo = { version = "3.20.3", features = ["allocator_api", "collections"] }
 eq_modulo_pos = { path = "../utils/eq_modulo_pos" }
 file_info = { path = "../deps/rust/file_info" }
+hack_name = { path = "../utils/hack_name/cargo" }
 hash = { path = "../utils/hash" }
 hh24_types = { path = "../utils/hh24_types" }
 hh_autoimport_rust = { path = "../parser/cargo/hh_autoimport" }

hphp/hack/src/oxidized/Cargo.toml

@@ -3,7 +3,7 @@
 // This source code is licensed under the MIT license found in the
 // LICENSE file in the "hack" directory of this source tree.
 //
-// @generated SignedSource<<aa25f2a5e885275c1761709806ae1743>>
+// @generated SignedSource<<e4f2726af84f2f8b5e71924505052188>>
 //
 // To regenerate this file, run:
 //   buck run @fbcode//mode/dev-nosan-lg fbcode//hphp/hack/src:oxidized_regen
@@ -148,6 +148,7 @@ pub enum Naming {
     NamedVariadicTypeDisallowed = 2133,
     VariadicNamedParameterDisallowed = 2134,
     ImplicitPackageFileDirectlyUnderPath = 2135,
+    ImplicitPackageInvalidMemberDir = 2136,
 }
 impl TrivialDrop for Naming {}
 arena_deserializer::impl_deserialize_in_arena!(Naming);

hphp/hack/src/oxidized/gen/error_codes.rs

@@ -174,6 +174,38 @@ impl Naming {
         )
     }
 
+    /// A file under an implicit package family sits in a directory whose name
+    /// is not a valid Hack identifier, so that directory can name no member
+    /// package.
+    ///
+    /// `family_pos` points at the family's declaration in PACKAGES.toml.
+    pub fn implicit_package_invalid_member_dir(
+        pos: Pos,
+        family: &str,
+        family_pos: Pos,
+        member_dir: &str,
+    ) -> Diagnostic {
+        UserDiagnostic::new(
+            Severity::Err,
+            Self::ImplicitPackageInvalidMemberDir as isize,
+            Message(
+                pos,
+                format!(
+                    "The directory `{}` cannot name a member of the implicit package family `{}`; an implicit package member directory must be a valid identifier",
+                    member_dir, family
+                )
+                .into(),
+            ),
+            vec![Message(
+                family_pos,
+                format!("The implicit package family `{}` is declared here", family).into(),
+            )],
+            Explanation::Empty,
+            vec![],
+            vec![],
+        )
+    }
+
     pub fn bad_builtin_type(p: Pos, name: &str, correct_name: &str) -> Diagnostic {
         UserDiagnostic::new(
             Severity::Err,

hphp/hack/src/oxidized/manual/diagnostics_impl.rs

@@ -168,6 +168,19 @@ static MULTIFILE_PREFIX: LazyLock<regex::Regex> = LazyLock::new(|| {
     regex::Regex::new(r"^.*--").expect("should compile: the pattern is a literal")
 });
 
+/// Where a file sits relative to the implicit package families. The illegal
+/// placements carry the family's positioned name, so the caller can point at its
+/// declaration in PACKAGES.toml.
+pub enum ImplicitFamilyPlacement<'a> {
+    /// Belongs to a member package, or is not under any family at all.
+    Valid,
+    /// Lies directly under a family `path` with no member directory in between.
+    DirectlyUnderFamily(&'a PosId),
+    /// Lies under a directory whose name is not a valid Hack identifier, so that
+    /// directory names no member package. Carries the offending name.
+    InvalidMemberDir(&'a PosId, String),
+}
+
 /// The path rewriting every package lookup applies before prefix-matching. All
 /// lookups must go through this, so they prefix-match the same string.
 fn normalize(support_multifile_tests: bool, path: &str) -> Cow<'_, str> {
@@ -209,25 +222,32 @@ impl PackageInfo {
         }
     }
 
-    /// The implicit family (its positioned name) if `path` lies directly under
-    /// that family's `path`, with no member directory in between -- e.g.
-    /// `//prototypes/loose.php` under a family declared at `//prototypes/`.
+    /// Where `path` sits relative to the implicit package families.
     ///
-    /// `None` means the file is legally placed: either it belongs to a member,
-    /// or it is under no family at all.
-    pub fn file_directly_under_implicit_family(
+    /// - `Valid` -- belongs to a member package, or lies under no family at all.
+    /// - `DirectlyUnderFamily` -- lies directly under a family `path` with no
+    ///   member directory in between, e.g. `prototypes/loose.php` under a family
+    ///   declared at `prototypes/`.
+    /// - `InvalidMemberDir` -- the member directory is not a valid Hack
+    ///   identifier, so it names no member package.
+    pub fn check_implicit_family_placement(
         &self,
         support_multifile_tests: bool,
         path: &str,
-    ) -> Option<&PosId> {
+    ) -> ImplicitFamilyPlacement<'_> {
         let path = normalize(support_multifile_tests, path);
-        let (package, remainder) = self.match_include_path(&path)?;
+        let Some((package, remainder)) = self.match_include_path(&path) else {
+            return ImplicitFamilyPlacement::Valid;
+        };
         if !package.is_implicit {
-            return None;
+            return ImplicitFamilyPlacement::Valid;
         }
         match remainder.split_once('/') {
-            Some((dir, _)) if !dir.is_empty() => None,
-            _ => Some(&package.name),
+            Some((dir, _)) if hack_name::is_valid_identifier(dir) => ImplicitFamilyPlacement::Valid,
+            Some((dir, _)) if !dir.is_empty() => {
+                ImplicitFamilyPlacement::InvalidMemberDir(&package.name, dir.to_owned())
+            }
+            _ => ImplicitFamilyPlacement::DirectlyUnderFamily(&package.name),
         }
     }
 }
@@ -292,6 +312,57 @@ mod test {
 
         // A file outside the family path belongs to no package.
         assert!(info.get_package_for_file(false, "other/x.php").is_none());
+
+        // Resolution does NOT reject a badly-named member directory; it
+        // synthesizes a member like any other.
+        for bad in [
+            "www/prototypes/proto.v1/a.php",
+            "www/prototypes/1bad/a.php",
+            "www/prototypes/:xhp/a.php",
+        ] {
+            assert!(
+                info.get_package_for_file(false, bad).is_some(),
+                "{bad} should still resolve; rejecting it is the placement check's job"
+            );
+        }
+    }
+
+    // Identifier validity is decided only by the placement classifier.
+    #[test]
+    fn placement_rejects_badly_named_member_dirs() {
+        let family = Package {
+            name: pos_id("prototypes"),
+            includes: vec![],
+            soft_includes: vec![],
+            include_paths: vec![pos_id("www/prototypes/")],
+            enable_strict_isolation: true,
+            is_implicit: true,
+        };
+        let info = PackageInfo {
+            existing_packages: Default::default(),
+            include_path_to_package_map: vec![("www/prototypes/".to_string(), family)],
+        };
+
+        for (path, expected_dir) in [
+            ("www/prototypes/proto.v1/a.php", "proto.v1"),
+            ("www/prototypes/1bad/a.php", "1bad"),
+            ("www/prototypes/:xhp/a.php", ":xhp"),
+        ] {
+            match info.check_implicit_family_placement(false, path) {
+                ImplicitFamilyPlacement::InvalidMemberDir(_, dir) => {
+                    assert_eq!(dir, expected_dir, "wrong directory reported for {path}")
+                }
+                _ => panic!("{path} should be reported as a badly-named member directory"),
+            }
+        }
+
+        // A validly-named directory, and a file outside any family, are fine.
+        for ok in ["www/prototypes/alpha/a.php", "other/x.php"] {
+            assert!(matches!(
+                info.check_implicit_family_placement(false, ok),
+                ImplicitFamilyPlacement::Valid
+            ));
+        }
     }
 
     // A container path with directories in it must be dropped whole; a

hphp/hack/src/oxidized/manual/package_info_impl.rs

@@ -52,12 +52,14 @@ let synthesize_member (family : Package.t) (member_dir : string) : Package.t =
     Package.is_implicit = true;
   }
 
-(* Splits a (possibly synthesized) name [F.D] into family [F] and member [D],
- * splitting on the *first* [.]. The member [D] is a single directory name that
- * may itself contain [.] (e.g. [proto.v1]), so we keep the remainder after the
- * first [.] as the member. Family names are forbidden from containing [.] (see
- * the family-name validation in config.rs), so this split is unambiguous.
- * Returns None unless both the family and the member are non-empty. *)
+(* Splits a (possibly synthesized) name [F.D] into family [F] and member [D].
+ * In a well-formed repo a member directory is a valid Hack identifier and so
+ * contains no [.], and family names are forbidden from containing [.] (see the
+ * family-name validation in config.rs), so [F.D] contains exactly one [.].
+ * Resolution does not re-verify that: a directory that is not a valid identifier
+ * is reported by the lowerer's placement check, and this code is only meaningful
+ * for a repo that type-checks clean. Returns None unless both sides are
+ * non-empty. *)
 let split_member_name (pkg : string) : (string * string) option =
   match String.lsplit2 pkg ~on:'.' with
   | Some (family, member)

hphp/hack/src/packages/package_info.ml

@@ -6266,25 +6266,30 @@ fn get_current_package<'a>(env: &mut Env<'a>, node: S<'a>) -> Option<PackageMemb
 /// subdirectory; one placed directly under the family path belongs to no member
 /// package, so it is illegal.
 fn check_implicit_package_placement(env: &mut Env<'_>) {
+    use oxidized::package_info_impl::ImplicitFamilyPlacement;
+
     let parser_options = env.parser_options;
     let file_path = env.source_text().file_path_rc();
-    let Some(family) = parser_options
-        .package_info
-        .file_directly_under_implicit_family(
-            parser_options.package_support_multifile_tests,
-            file_path.path_str(),
-        )
-    else {
-        return;
-    };
-    let (family_pos, family_name) = (family.0.clone(), family.1.clone());
-    // Zero-width, at the start of the file: nothing the file contains is at
-    // fault, and a `.hack` file has no `<?hh` pragma to point at.
-    let file_top_pos = Pos::from_lnum_bol_offset(file_path, (1, 0, 0), (1, 0, 0));
-    raise_hh_error(
-        env,
-        Naming::implicit_package_file_directly_under_path(file_top_pos, &family_name, family_pos),
+    let placement = parser_options.package_info.check_implicit_family_placement(
+        parser_options.package_support_multifile_tests,
+        file_path.path_str(),
     );
+    // Both errors are anchored at a zero-width position at the start of the
+    // file: a `.hack` file has no `<?hh` pragma to point at.
+    let error = match placement {
+        ImplicitFamilyPlacement::Valid => return,
+        ImplicitFamilyPlacement::DirectlyUnderFamily(family) => {
+            let (family_pos, family_name) = (family.0.clone(), family.1.clone());
+            let pos = Pos::from_lnum_bol_offset(file_path, (1, 0, 0), (1, 0, 0));
+            Naming::implicit_package_file_directly_under_path(pos, &family_name, family_pos)
+        }
+        ImplicitFamilyPlacement::InvalidMemberDir(family, member_dir) => {
+            let (family_pos, family_name) = (family.0.clone(), family.1.clone());
+            let pos = Pos::from_lnum_bol_offset(file_path, (1, 0, 0), (1, 0, 0));
+            Naming::implicit_package_invalid_member_dir(pos, &family_name, family_pos, &member_dir)
+        }
+    };
+    raise_hh_error(env, error);
 }
 
 fn p_def<'a>(node: S<'a>, env: &mut Env<'a>) -> Result<Vec<ast::Def>> {

hphp/hack/src/parser/lowerer/lowerer.rs

@@ -0,0 +1,15 @@
+//// prototypes/loose.php
+<?hh
+// Directly under the family path: reported by check_directly_under_family.
+const int LOOSE_C = 1;
+
+//// prototypes/1bad/b.php
+<?hh
+// Under a directory that cannot name a member: reported by
+// check_invalid_member_dir.
+const int BAD_C = 1;
+
+//// prototypes/ok/o.php
+<?hh
+// Correctly placed, so neither check fires for it.
+const int OK_C = 1;