Add Bazel-style visibility() limiting for bzl loads

app/buck2_common/src/package_listing/dice.rs

@@ -26,6 +26,7 @@ use pagable::Pagable;
 use pagable::pagable_typetag;
 use smallvec::SmallVec;
 
+use crate::file_ops::dice::DiceFileComputations;
 use crate::package_listing::interpreter::InterpreterPackageListingResolver;
 use crate::package_listing::listing::PackageListing;
 use crate::package_listing::resolver::PackageListingResolver;
@@ -84,8 +85,137 @@ impl Key for PackageListingKey {
     }
 }
 
+/// Whether a directory contains a buildfile. Depends only on the existence of
+/// each buildfile candidate, not on the directory listing, so adding or removing
+/// other files in the directory does not invalidate it.
+#[derive(
+    Clone,
+    Copy,
+    Dupe,
+    derive_more::Display,
+    Debug,
+    Eq,
+    Hash,
+    PartialEq,
+    Allocative,
+    Pagable
+)]
+#[pagable_typetag(dice::DiceKeyDyn)]
+struct HasBuildfileKey(CellPath);
+
+#[async_trait]
+impl Key for HasBuildfileKey {
+    type Value = buck2_error::Result<bool>;
+
+    async fn compute(
+        &self,
+        ctx: &mut DiceComputations,
+        _cancellations: &CancellationContext,
+    ) -> Self::Value {
+        for candidate in DiceFileComputations::buildfiles(ctx, self.0.cell())
+            .await?
+            .iter()
+        {
+            if DiceFileComputations::exists(ctx, self.0.join(candidate)).await? {
+                return Ok(true);
+            }
+        }
+        Ok(false)
+    }
+
+    fn equality_behavior() -> EqualityBehavior<Self::Value> {
+        EqualityBehavior::Compare(|x, y| match (x, y) {
+            (Ok(x), Ok(y)) => x == y,
+            _ => false,
+        })
+    }
+
+    fn validity(x: &Self::Value) -> bool {
+        x.is_ok()
+    }
+
+    fn value_serialize() -> impl ValueSerialize<Value = Self::Value> {
+        OkPagableValueSerialize::<Self::Value>::new()
+    }
+}
+
+/// The package enclosing a directory: the nearest directory at or above it
+/// that contains a buildfile, or `None` if there is none up to the cell root.
+/// Only changes when a buildfile is added or removed at or above the directory.
+#[derive(
+    Clone,
+    Copy,
+    Dupe,
+    derive_more::Display,
+    Debug,
+    Eq,
+    Hash,
+    PartialEq,
+    Allocative,
+    Pagable
+)]
+#[pagable_typetag(dice::DiceKeyDyn)]
+struct EnclosingPackageKey(CellPath);
+
+#[async_trait]
+impl Key for EnclosingPackageKey {
+    type Value = buck2_error::Result<Option<PackageLabel>>;
+
+    async fn compute(
+        &self,
+        ctx: &mut DiceComputations,
+        _cancellations: &CancellationContext,
+    ) -> Self::Value {
+        if ctx.compute(&HasBuildfileKey(self.0)).await?.dupe()? {
+            return Ok(Some(PackageLabel::from_cell_path(self.0)?));
+        }
+        match self.0.parent() {
+            Some(parent) => enclosing_package_of_dir(ctx, parent).await,
+            None => Ok(None),
+        }
+    }
+
+    fn equality_behavior() -> EqualityBehavior<Self::Value> {
+        EqualityBehavior::Compare(|x, y| match (x, y) {
+            (Ok(x), Ok(y)) => x == y,
+            _ => false,
+        })
+    }
+
+    fn validity(x: &Self::Value) -> bool {
+        x.is_ok()
+    }
+
+    fn value_serialize() -> impl ValueSerialize<Value = Self::Value> {
+        OkPagableValueSerialize::<Self::Value>::new()
+    }
+}
+
 pub struct DicePackageListingResolver<'compute, 'dice>(pub &'compute mut DiceComputations<'dice>);
 
+impl DicePackageListingResolver<'_, '_> {
+    /// The package of the file at `path`: the nearest directory at or above it
+    /// that contains a buildfile. Unlike `get_enclosing_package`, this returns
+    /// `None` rather than an error when there is none.
+    pub async fn find_enclosing_package(
+        &mut self,
+        path: CellPath,
+    ) -> buck2_error::Result<Option<PackageLabel>> {
+        match path.parent() {
+            Some(dir) => enclosing_package_of_dir(self.0, dir).await,
+            None => Ok(None),
+        }
+    }
+}
+
+/// The package enclosing the directory `dir`, or `None` if there is none.
+pub(crate) async fn enclosing_package_of_dir(
+    ctx: &mut DiceComputations<'_>,
+    dir: CellPath,
+) -> buck2_error::Result<Option<PackageLabel>> {
+    ctx.compute(&EnclosingPackageKey(dir)).await?.dupe()
+}
+
 #[async_trait]
 impl PackageListingResolver for DicePackageListingResolver<'_, '_> {
     async fn resolve(&mut self, package: PackageLabel) -> buck2_error::Result<PackageListing> {

app/buck2_common/src/package_listing/interpreter.rs

@@ -26,6 +26,7 @@ use crate::find_buildfile::find_buildfile;
 use crate::ignores::file_ignores::FileIgnoreReason;
 use crate::io::DirectoryDoesNotExistSuggestion;
 use crate::io::ReadDirError;
+use crate::package_listing::dice::enclosing_package_of_dir;
 use crate::package_listing::listing::PackageListing;
 use crate::package_listing::resolver::PackageListingResolver;
 
@@ -43,15 +44,18 @@ impl PackageListingResolver for InterpreterPackageListingResolver<'_, '_> {
     }
 
     async fn get_enclosing_package(&mut self, path: CellPath) -> buck2_error::Result<PackageLabel> {
-        let buildfile_candidates = DiceFileComputations::buildfiles(self.ctx, path.cell()).await?;
-        if let Some(path) = path.parent() {
-            for path in path.ancestors() {
-                let listing = DiceFileComputations::read_dir(self.ctx, path).await?;
-                if find_buildfile(buildfile_candidates, &listing.included).is_some() {
-                    return PackageLabel::from_cell_path(path);
-                }
+        if let Some(dir) = path.parent() {
+            if !DiceFileComputations::exists(self.ctx, dir).await? {
+                // Only for the error, which says what is wrong and suggests
+                // corrections: depending on the listing otherwise would rerun
+                // this on any change to the directory.
+                DiceFileComputations::read_dir(self.ctx, dir).await?;
+            }
+            if let Some(package) = enclosing_package_of_dir(self.ctx, dir).await? {
+                return Ok(package);
             }
         }
+        let buildfile_candidates = DiceFileComputations::buildfiles(self.ctx, path.cell()).await?;
         Err(PackageListingError::NoContainingPackage(path, buildfile_candidates.to_vec()).into())
     }
 
@@ -60,26 +64,30 @@ impl PackageListingResolver for InterpreterPackageListingResolver<'_, '_> {
         path: CellPath,
         enclosing_path: CellPath,
     ) -> buck2_error::Result<Vec<PackageLabel>> {
-        let buildfile_candidates = DiceFileComputations::buildfiles(self.ctx, path.cell()).await?;
-        if let Some(path) = path.parent() {
-            let mut packages = Vec::new();
-            for path in path.ancestors() {
-                if !path.starts_with(enclosing_path) {
-                    // stop when we are no longer within the enclosing path
-                    break;
-                }
-                let listing = DiceFileComputations::read_dir(self.ctx, path).await?;
-                if find_buildfile(buildfile_candidates, &listing.included).is_some() {
-                    packages.push(PackageLabel::from_cell_path(path)?);
-                }
-            }
-            Ok(packages)
-        } else {
-            Err(
-                PackageListingError::NoContainingPackage(path, buildfile_candidates.to_vec())
-                    .into(),
+        let Some(mut dir) = path.parent() else {
+            let buildfile_candidates =
+                DiceFileComputations::buildfiles(self.ctx, path.cell()).await?;
+            return Err(PackageListingError::NoContainingPackage(
+                path,
+                buildfile_candidates.to_vec(),
             )
+            .into());
+        };
+        // Hop from package to package rather than reading every directory, so
+        // this only depends on where the buildfiles are.
+        let mut packages = Vec::new();
+        while let Some(package) = enclosing_package_of_dir(self.ctx, dir).await? {
+            if !package.as_cell_path().starts_with(enclosing_path) {
+                // stop when we are no longer within the enclosing path
+                break;
+            }
+            packages.push(package);
+            match package.as_cell_path().parent() {
+                Some(parent) => dir = parent,
+                None => break,
+            }
         }
+        Ok(packages)
     }
 }
 

app/buck2_core/src/pattern/pattern.rs

@@ -276,6 +276,19 @@ impl ParsedPattern<TargetPatternExtra> {
             }
         }
     }
+
+    /// Check if a [`ParsedPattern`] matches a package, identified by the
+    /// cell path of its directory. Used for load visibility, where the entity
+    /// being matched is the directory of a loading `BUCK`/`.bzl` file rather
+    /// than a target. A `Target` pattern (`//pkg:name`) names a single target,
+    /// not a package, so it never matches.
+    pub fn matches_package(&self, package: CellPath) -> bool {
+        match self {
+            ParsedPattern::Target(..) => false,
+            ParsedPattern::Package(pkg) => pkg.as_cell_path() == package,
+            ParsedPattern::Recursive(cell_path) => package.starts_with(*cell_path),
+        }
+    }
 }
 
 impl ParsedPattern<ProvidersPatternExtra> {

app/buck2_interpreter/src/parse_import.rs

@@ -32,6 +32,10 @@ enum ImportParseError {
     EmptyFileName(String),
     #[error("Unexpected relative import spec. Got `{0}`")]
     ProhibitedRelativeImport(String),
+    #[error(
+        "Cell-qualified load path `{import}` must begin with `@` to reference another cell (did you mean `{suggestion}`?). A bare cell alias like `cell//...` is not accepted in `load()`."
+    )]
+    CellAliasMissingAtSymbol { import: String, suggestion: String },
     #[error(
         "Unable to parse import spec. Expected format `(@<cell>)//package/name:filename.bzl` or `:filename.bzl`, but got a path. Got `{0}`"
     )]
@@ -62,6 +66,12 @@ fn has_cell_alias_prefix(path: &str, allow_missing_at_symbol: bool) -> bool {
     alias.is_empty() || alias.starts_with('@') || allow_missing_at_symbol
 }
 
+/// `cell//...` with a non-empty alias and no leading `@`.
+fn has_bare_cell_alias_prefix(path: &str) -> bool {
+    path.split_once("//")
+        .is_some_and(|(alias, _)| !alias.is_empty() && !alias.starts_with('@'))
+}
+
 pub fn parse_import(
     cell_resolver: &CellAliasResolver,
     relative_import_option: RelativeImports,
@@ -93,6 +103,15 @@ pub fn parse_import_with_config(
 
             if has_cell_alias_prefix(import, opts.allow_missing_at_symbol) {
                 Ok(parse_cell_path(import, cell_resolver)?)
+            } else if has_bare_cell_alias_prefix(import) {
+                // `cell//pkg/file` looks cell-qualified, but the alias lacks the
+                // required leading `@`. Give a targeted hint rather than treating
+                // it as a relative import that fails later with an opaque error.
+                Err(ImportParseError::CellAliasMissingAtSymbol {
+                    import: import.to_owned(),
+                    suggestion: format!("@{import}"),
+                }
+                .into())
             } else if let RelativeImports::Allow {
                 current_dir_with_allowed_relative,
             } = opts.relative_import_option
@@ -419,6 +438,38 @@ mod tests {
         Ok(())
     }
 
+    #[test]
+    fn cell_alias_without_at_symbol_is_rejected_with_hint() -> buck2_error::Result<()> {
+        // A bare `cell//...` (no leading `@`, no colon) must not be silently
+        // treated as a relative import and fail later with an opaque
+        // forward-relative-path error. It should report the missing `@`.
+        let import = "other//pkg/lib.lock";
+        let res = parse_import(
+            &resolver(),
+            RelativeImports::Allow {
+                current_dir_with_allowed_relative: &CellPathWithAllowedRelativeDir::new(
+                    CellPath::testing_new("root//pkg"),
+                    None,
+                ),
+            },
+            import,
+        );
+        match res {
+            Ok(res) => panic!("Expected parse failure for {import}, got result {res}"),
+            Err(e) => {
+                assert_eq!(
+                    format!("{e:#}"),
+                    ImportParseError::CellAliasMissingAtSymbol {
+                        import: import.to_owned(),
+                        suggestion: format!("@{import}"),
+                    }
+                    .to_string()
+                );
+            }
+        };
+        Ok(())
+    }
+
     #[test]
     fn fails_relative_import_if_disallowed() -> buck2_error::Result<()> {
         let imported_file = ":bar.bzl";

app/buck2_interpreter_for_build/src/interpreter/build_context.rs

@@ -47,6 +47,8 @@ enum BuildContextError {
         "`{0}()` can only be called from a package file context; current file context is {1:?}"
     )]
     NotPackageFile(String, StarlarkFileType),
+    #[error("`{0}()` can only be called from a `.bzl` file; current file context is {1:?}")]
+    NotBzlFile(String, StarlarkFileType),
     #[error("Expecting package file context, but current file context is {0:?}")]
     NotPackageFileNoFunction(StarlarkFileType),
     #[error("Package can only be fetched from a build file")]
@@ -142,6 +144,13 @@ impl PerFileTypeContext {
             x => Err(BuildContextError::NotPackageFileNoFunction(x.file_type()).into()),
         }
     }
+
+    pub(crate) fn require_bzl(&self, function_name: &str) -> buck2_error::Result<&BzlEvalCtx> {
+        match self {
+            PerFileTypeContext::Bzl(ctx) => Ok(ctx),
+            x => Err(BuildContextError::NotBzlFile(function_name.to_owned(), x.file_type()).into()),
+        }
+    }
 }
 
 /// Buck-specific information exposed to the starlark environment via the context's extra field.