From 17c61438a61fbbf15ac09ce971317afe35b3da02 Mon Sep 17 00:00:00 2001 From: Cormac Relf Date: Tue, 21 Jul 2026 15:43:12 +1000 Subject: [PATCH 01/10] Reject bare cell aliases with a hint --- app/buck2_interpreter/src/parse_import.rs | 51 +++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/app/buck2_interpreter/src/parse_import.rs b/app/buck2_interpreter/src/parse_import.rs index 965ab55d96a1f..9ba16b4429471 100644 --- a/app/buck2_interpreter/src/parse_import.rs +++ b/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 `(@)//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"; From d1a19fd447deacf1aaa43b579cf201bad0685725 Mon Sep 17 00:00:00 2001 From: Cormac Relf Date: Tue, 21 Jul 2026 15:42:34 +1000 Subject: [PATCH 02/10] Data types for load visibility --- app/buck2_core/src/pattern/pattern.rs | 13 ++ .../src/interpreter/extra_value.rs | 1 + app/buck2_node/src/visibility.rs | 126 ++++++++++++++++++ 3 files changed, 140 insertions(+) diff --git a/app/buck2_core/src/pattern/pattern.rs b/app/buck2_core/src/pattern/pattern.rs index 555f864b9bd41..ba3e5b91cba78 100644 --- a/app/buck2_core/src/pattern/pattern.rs +++ b/app/buck2_core/src/pattern/pattern.rs @@ -276,6 +276,19 @@ impl ParsedPattern { } } } + + /// 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 { diff --git a/app/buck2_interpreter_for_build/src/interpreter/extra_value.rs b/app/buck2_interpreter_for_build/src/interpreter/extra_value.rs index 1da03e9dd263a..9470ba387eb4f 100644 --- a/app/buck2_interpreter_for_build/src/interpreter/extra_value.rs +++ b/app/buck2_interpreter_for_build/src/interpreter/extra_value.rs @@ -13,6 +13,7 @@ use std::cell::OnceCell; use allocative::Allocative; use buck2_error::BuckErrorOptionContext; use buck2_node::attrs::coercion_context::CoerceMemoTable; +use buck2_node::visibility::LoadVisibility; use starlark::StarlarkPagable; use starlark::StarlarkPagablePanic; use starlark::any::ProvidesStaticType; diff --git a/app/buck2_node/src/visibility.rs b/app/buck2_node/src/visibility.rs index 755c9702c9c51..586b7817722f8 100644 --- a/app/buck2_node/src/visibility.rs +++ b/app/buck2_node/src/visibility.rs @@ -13,6 +13,7 @@ use std::fmt::Display; use std::fmt::Formatter; use allocative::Allocative; +use buck2_core::cells::cell_path::CellPath; use buck2_core::package::PackageLabel; use buck2_core::pattern::package::PackagePattern; use buck2_core::pattern::pattern::ParsedPattern; @@ -452,6 +453,24 @@ impl VisibilityPatternList { } } } + + /// Whether this list matches a package (the directory of a loading file), + /// for load visibility. `target_name_glob(...)` patterns are target-oriented + /// and never match a bare package, so they are treated as non-matching (they + /// also cannot be produced by the `visibility()` builtin, which only accepts + /// strings). + pub fn matches_package(&self, package: CellPath) -> bool { + match self { + VisibilityPatternList::Public => true, + VisibilityPatternList::List(patterns) => patterns.iter().any(|pattern| match pattern { + VisibilityPattern::Parsed(p) => p.matches_package(package), + VisibilityPattern::TargetNameGlob(_) => false, + }), + VisibilityPatternList::Intersection(sub_lists) => { + sub_lists.iter().all(|s| s.matches_package(package)) + } + } + } } impl Display for VisibilityPatternList { @@ -797,6 +816,64 @@ impl Display for WithinViewSpecification { } } +/// The load visibility of a `.bzl` file: the set of packages whose `BUCK`/`.bzl` +/// files may `load()` it, as declared by the `visibility()` builtin. Unlike +/// target visibility this is matched against the *package* (directory) of the +/// loading file rather than a target label. When a `.bzl` file never calls +/// `visibility()` there is no `LoadVisibility` at all, which is treated as +/// public (matching Bazel). +#[derive(Debug, Eq, PartialEq, Hash, Clone, Dupe, Allocative, Pagable)] +pub enum LoadVisibility { + /// `visibility("private")`: loadable only from the same package (enforced + /// separately by the load-visibility check). Kept distinct from an empty + /// pattern list, which has the same meaning, so it renders as written. + Private, + /// `visibility([...])`, including `[]` and `["PUBLIC"]`. + Patterns(VisibilityPatternList), +} + +impl LoadVisibility { + pub fn new(list: VisibilityPatternList) -> LoadVisibility { + LoadVisibility::Patterns(list) + } + + /// `visibility(["PUBLIC"])` / `visibility("public")`: loadable from anywhere. + pub fn public() -> LoadVisibility { + LoadVisibility::Patterns(VisibilityPatternList::Public) + } + + /// `visibility("private")`: loadable only from the same package. + pub fn private() -> LoadVisibility { + LoadVisibility::Private + } + + /// Whether a file in `package` (its directory) may load the `.bzl` file that + /// declared this visibility. + pub fn matches_package(&self, package: CellPath) -> bool { + match self { + LoadVisibility::Private => false, + LoadVisibility::Patterns(list) => list.matches_package(package), + } + } + + pub fn to_json(&self) -> serde_json::Value { + match self { + LoadVisibility::Private => serde_json::json!("private"), + LoadVisibility::Patterns(list) => list.to_json(), + } + } +} + +/// Renders the argument as it would be passed to `visibility()`. +impl Display for LoadVisibility { + fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { + match self { + LoadVisibility::Private => write!(f, "\"private\""), + LoadVisibility::Patterns(list) => Display::fmt(list, f), + } + } +} + impl AnyMatches for VisibilitySpecification { fn any_matches( &self, @@ -933,6 +1010,55 @@ mod tests { ); } + fn pkg(s: &str) -> buck2_core::cells::cell_path::CellPath { + label(s).pkg().as_cell_path().to_owned() + } + + #[test] + fn matches_package_public_and_private() { + assert!(VisibilityPatternList::Public.matches_package(pkg("root//anywhere:x"))); + + // Empty list is private: matches nothing. + let private = VisibilityPatternList::List(ThinArcSlice::empty()); + assert!(!private.matches_package(pkg("root//anywhere:x"))); + } + + #[test] + fn matches_package_recursive_and_exact() { + let list = VisibilityPatternList::testing_parse(&["root//foo/...", "root//bar:"]); + // `//foo/...` matches the package itself and its subpackages. + assert!(list.matches_package(pkg("root//foo:x"))); + assert!(list.matches_package(pkg("root//foo/sub:x"))); + // `//bar:` matches only that exact package, not subpackages. + assert!(list.matches_package(pkg("root//bar:x"))); + assert!(!list.matches_package(pkg("root//bar/sub:x"))); + // Unrelated package does not match. + assert!(!list.matches_package(pkg("root//other:x"))); + } + + #[test] + fn matches_package_target_pattern_never_matches() { + // A `//foo:bar` target pattern names a target, not a package. + let list = VisibilityPatternList::testing_parse(&["root//foo:bar"]); + assert!(!list.matches_package(pkg("root//foo:anything"))); + } + + #[test] + fn load_visibility_public_and_private() { + assert!(LoadVisibility::public().matches_package(pkg("root//anywhere:x"))); + assert!(!LoadVisibility::private().matches_package(pkg("root//anywhere:x"))); + } + + #[test] + fn load_visibility_display() { + assert_eq!(LoadVisibility::private().to_string(), "\"private\""); + assert_eq!(LoadVisibility::public().to_string(), "[\"PUBLIC\"]"); + assert_eq!( + LoadVisibility::new(VisibilityPatternList::List(ThinArcSlice::empty())).to_string(), + "[]" + ); + } + #[test] fn extend_with_intersection_returns_internal_error() { let a = VisibilityPatternList::testing_parse(&["root//foo:"]); From b7165cd93e3390c3f480ea5d69bcdcca67ba97b8 Mon Sep 17 00:00:00 2001 From: Cormac Relf Date: Tue, 21 Jul 2026 15:42:59 +1000 Subject: [PATCH 03/10] Export load visibility from frozen starlark module --- .../src/interpreter/extra_value.rs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/app/buck2_interpreter_for_build/src/interpreter/extra_value.rs b/app/buck2_interpreter_for_build/src/interpreter/extra_value.rs index 9470ba387eb4f..e48703e4b3e63 100644 --- a/app/buck2_interpreter_for_build/src/interpreter/extra_value.rs +++ b/app/buck2_interpreter_for_build/src/interpreter/extra_value.rs @@ -47,11 +47,17 @@ pub(crate) struct InterpreterExtraValue<'v> { pub(crate) buckconfigs: BuckConfigsCache<'v>, /// Coercion results memoized while evaluating a buildfile. pub(crate) coerce_memo: CoerceMemoTable<'v>, + /// Set by the `visibility()` builtin when evaluating a `.bzl` file. Holds no + /// Starlark values, so it is ignored for tracing. + #[trace(unsafe_ignore)] + pub(crate) load_visibility: OnceCell, } #[derive(Debug, ProvidesStaticType, Allocative, StarlarkPagable)] pub(crate) struct FrozenInterpreterExtraValue<'v> { pub(crate) package_extra: Option>, + #[starlark_pagable(pagable)] + pub(crate) load_visibility: Option, } starlark::register_starlark_any_complex!(InterpreterExtraValue<'_>, frozen FrozenInterpreterExtraValue<'_>); @@ -64,9 +70,11 @@ impl<'v> Freeze<'v> for InterpreterExtraValue<'v> { package_extra, buckconfigs: _, coerce_memo: _, + load_visibility, } = self; Ok(FrozenInterpreterExtraValue { package_extra: Freeze::freeze(package_extra, freezer)?, + load_visibility: load_visibility.into_inner(), }) } } @@ -98,4 +106,12 @@ impl FrozenInterpreterExtraValue<'_> { ) .internal_error("Extra value had wrong type") } + + /// The load visibility declared by a `.bzl` file via the `visibility()` + /// builtin, or `None` if it did not call it (treated as public). + pub(crate) fn load_visibility( + module: &FrozenModule, + ) -> buck2_error::Result> { + Ok(Self::get(module)?.by_ref(|v| v.as_ref().value.load_visibility.clone())) + } } From 2674888051da23dac413a1edfc1b819830681f83 Mon Sep 17 00:00:00 2001 From: Cormac Relf Date: Tue, 21 Jul 2026 15:43:12 +1000 Subject: [PATCH 04/10] Parse and save load visibility during eval --- .../src/interpreter/build_context.rs | 9 + .../src/interpreter/functions.rs | 1 + .../interpreter/functions/load_visibility.rs | 166 ++++++++++++++++++ .../src/interpreter/globals.rs | 2 + .../src/functions.rs | 1 + .../src/functions/load_visibility.rs | 114 ++++++++++++ 6 files changed, 293 insertions(+) create mode 100644 app/buck2_interpreter_for_build/src/interpreter/functions/load_visibility.rs create mode 100644 app/buck2_interpreter_for_build_tests/src/functions/load_visibility.rs diff --git a/app/buck2_interpreter_for_build/src/interpreter/build_context.rs b/app/buck2_interpreter_for_build/src/interpreter/build_context.rs index ff5bca74e79c1..e74977cf0f64f 100644 --- a/app/buck2_interpreter_for_build/src/interpreter/build_context.rs +++ b/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. diff --git a/app/buck2_interpreter_for_build/src/interpreter/functions.rs b/app/buck2_interpreter_for_build/src/interpreter/functions.rs index 86ca5cc344a80..964bc13aaff25 100644 --- a/app/buck2_interpreter_for_build/src/interpreter/functions.rs +++ b/app/buck2_interpreter_for_build/src/interpreter/functions.rs @@ -12,6 +12,7 @@ pub(crate) mod dedupe; pub(crate) mod host_info; pub(crate) mod internals; pub(crate) mod load_symbols; +pub(crate) mod load_visibility; pub(crate) mod path; pub(crate) mod read_config; pub(crate) mod regex; diff --git a/app/buck2_interpreter_for_build/src/interpreter/functions/load_visibility.rs b/app/buck2_interpreter_for_build/src/interpreter/functions/load_visibility.rs new file mode 100644 index 0000000000000..993fd9a5ab80a --- /dev/null +++ b/app/buck2_interpreter_for_build/src/interpreter/functions/load_visibility.rs @@ -0,0 +1,166 @@ +/* + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is dual-licensed under either the MIT license found in the + * LICENSE-MIT file in the root directory of this source tree or the Apache + * License, Version 2.0 found in the LICENSE-APACHE file in the root directory + * of this source tree. You may select, at your option, one of the + * above-listed licenses. + */ + +use buck2_core::cells::CellAliasResolver; +use buck2_core::cells::CellResolver; +use buck2_core::cells::name::CellName; +use buck2_core::pattern::pattern::ParsedPattern; +use buck2_node::visibility::LoadVisibility; +use buck2_node::visibility::VisibilityPattern; +use buck2_node::visibility::VisibilityWithinViewBuilder; +use either::Either; +use starlark::environment::GlobalsBuilder; +use starlark::eval::Evaluator; +use starlark::starlark_module; +use starlark::values::list_or_tuple::UnpackListOrTuple; +use starlark::values::none::NoneType; + +use crate::interpreter::build_context::BuildContext; +use crate::interpreter::extra_value::InterpreterExtraValue; + +#[derive(Debug, buck2_error::Error)] +#[buck2(tag = Input)] +enum LoadVisibilityError { + #[error( + "`visibility()` must be called at the top level of the `.bzl` file it applies to, not from inside a function" + )] + NotTopLevel, + #[error("`visibility()` can be called at most once per `.bzl` file")] + AtMostOnce, + #[error( + "`{0}` is a target label, but `visibility()` takes package patterns (`//pkg:` or `//pkg/...`)" + )] + TargetLabel(String), + #[error( + "`{0}` is not a package pattern; `visibility()` takes package patterns (`//pkg:` or `//pkg/...`)" + )] + NotPackagePattern(String), + #[error("`\"{0}\"` is not valid in `visibility()`; use {1}")] + WrongCase(String, &'static str), + #[error("Negative patterns are not supported in `visibility()`: `{0}`")] + NegativePattern(String), + #[error("`\"private\"` must be the only entry when passed to `visibility()`")] + PrivateNotAlone, +} + +/// The `"private"` special token, meaning "loadable only from the same package". +/// Equivalent to passing an empty list. +const PRIVATE: &str = "private"; +/// The lowercase Bazel-style spelling of `PUBLIC`. Both are accepted. +const PUBLIC_LOWER: &str = "public"; + +fn parse_load_visibility( + value: Either<&str, UnpackListOrTuple<&str>>, + cell_name: CellName, + cell_resolver: &CellResolver, + cell_alias_resolver: &CellAliasResolver, +) -> buck2_error::Result { + let patterns: Vec<&str> = match value { + Either::Left(s) => vec![s], + Either::Right(list) => list.items, + }; + + // `"private"` is a same-package-only token that is not a package pattern, so + // it is handled here rather than by the pattern parser. It is only + // meaningful on its own (`visibility("private")` / `visibility(["private"])`). + if patterns.iter().any(|s| *s == PRIVATE) { + if patterns.len() != 1 { + return Err(LoadVisibilityError::PrivateNotAlone.into()); + } + return Ok(LoadVisibility::private()); + } + + let mut builder = VisibilityWithinViewBuilder::with_capacity(patterns.len()); + for s in patterns { + if s == PUBLIC_LOWER || s == VisibilityPattern::PUBLIC { + builder.add_public(); + } else if s.eq_ignore_ascii_case(PUBLIC_LOWER) { + return Err(LoadVisibilityError::WrongCase( + s.to_owned(), + "`\"PUBLIC\"` or `\"public\"`", + ) + .into()); + } else if s.eq_ignore_ascii_case(PRIVATE) { + return Err(LoadVisibilityError::WrongCase(s.to_owned(), "`\"private\"`").into()); + } else if s.starts_with('-') { + return Err(LoadVisibilityError::NegativePattern(s.to_owned()).into()); + } else if !s.contains(':') && !s.ends_with("...") { + return Err(LoadVisibilityError::NotPackagePattern(s.to_owned()).into()); + } else { + match ParsedPattern::parse_precise(s, cell_name, cell_resolver, cell_alias_resolver)? { + ParsedPattern::Target(..) => { + return Err(LoadVisibilityError::TargetLabel(s.to_owned()).into()); + } + p => builder.add(VisibilityPattern::Parsed(p)), + } + } + } + Ok(LoadVisibility::new(builder.build_visibility().0)) +} + +#[starlark_module] +pub(crate) fn register_load_visibility(globals: &mut GlobalsBuilder) { + /// Set the load visibility of the current `.bzl` file: the set of packages + /// whose `BUCK`/`.bzl` files may `load()` it. + /// + /// The argument is a list of package specifications (`//pkg:` or + /// `//pkg/...`, as in target `visibility`), but target labels, negative + /// specifications and `target_name_glob(...)` are not accepted. The `[]` + /// around a single entry may be omitted. + /// + /// * `visibility(["PUBLIC"])` / `visibility("public")` — loadable from + /// anywhere (this is also the default when `visibility()` is not called). + /// * `visibility([])` / `visibility("private")` — loadable only from the + /// same package. + /// * `visibility(["//foo/...", "//bar:"])` — loadable from the given + /// packages (and, always, the file's own package). + /// + /// It must be called at most once, at the top level of the `.bzl` file it + /// applies to (not inside a function). Enforcement of the resulting + /// visibility can be disabled with `buck2.check_load_visibility = false`. + /// + /// ```python + /// # //mylib/internal_defs.bzl + /// visibility(["//mylib/...", "//tests/mylib/..."]) + /// + /// def helper(): ... + /// ``` + fn visibility<'v>( + #[starlark(require = pos)] value: Either<&'v str, UnpackListOrTuple<&'v str>>, + eval: &mut Evaluator, + ) -> starlark::Result { + let build_context = BuildContext::from_context(eval)?; + // Only meaningful in `.bzl` files. + build_context.additional.require_bzl("visibility")?; + + // Must be called from the top level of the `.bzl` file it applies to. At + // top level the call stack holds the module's frame plus this call's + // frame; anything deeper means we were reached through a function, + // either one in this file or one in a loaded helper (which would + // silently set the visibility of the *loading* module). + if eval.call_stack_count() != 2 { + return Err(buck2_error::Error::from(LoadVisibilityError::NotTopLevel).into()); + } + + let load_visibility = parse_load_visibility( + value, + build_context.cell_info().name().name(), + build_context.cell_info().cell_resolver(), + build_context.cell_info().cell_alias_resolver(), + )?; + + InterpreterExtraValue::get(eval.module())? + .load_visibility + .set(load_visibility) + .map_err(|_| buck2_error::Error::from(LoadVisibilityError::AtMostOnce))?; + + Ok(NoneType) + } +} diff --git a/app/buck2_interpreter_for_build/src/interpreter/globals.rs b/app/buck2_interpreter_for_build/src/interpreter/globals.rs index d333c0bf24e0a..6e2d049c6d983 100644 --- a/app/buck2_interpreter_for_build/src/interpreter/globals.rs +++ b/app/buck2_interpreter_for_build/src/interpreter/globals.rs @@ -33,6 +33,7 @@ use crate::interpreter::functions::dedupe::register_dedupe; use crate::interpreter::functions::host_info::register_host_info; use crate::interpreter::functions::internals::register_internals; use crate::interpreter::functions::load_symbols::register_load_symbols; +use crate::interpreter::functions::load_visibility::register_load_visibility; use crate::interpreter::functions::path::register_path; use crate::interpreter::functions::read_config::register_read_config; use crate::interpreter::functions::regex::register_regex; @@ -73,6 +74,7 @@ pub fn register_load_natives(builder: &mut GlobalsBuilder) { register_regex(builder); register_buck_regex(builder); register_load_symbols(builder); + register_load_visibility(builder); register_rule_function(builder); register_attrs(builder); register_plugins(builder); diff --git a/app/buck2_interpreter_for_build_tests/src/functions.rs b/app/buck2_interpreter_for_build_tests/src/functions.rs index a6d5a5adb3722..cfaab9bf90ef3 100644 --- a/app/buck2_interpreter_for_build_tests/src/functions.rs +++ b/app/buck2_interpreter_for_build_tests/src/functions.rs @@ -10,5 +10,6 @@ mod host_info; mod load_symbols; +mod load_visibility; mod read_config; mod with_sub_target; diff --git a/app/buck2_interpreter_for_build_tests/src/functions/load_visibility.rs b/app/buck2_interpreter_for_build_tests/src/functions/load_visibility.rs new file mode 100644 index 0000000000000..73811fba8d160 --- /dev/null +++ b/app/buck2_interpreter_for_build_tests/src/functions/load_visibility.rs @@ -0,0 +1,114 @@ +/* + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is dual-licensed under either the MIT license found in the + * LICENSE-MIT file in the root directory of this source tree or the Apache + * License, Version 2.0 found in the LICENSE-APACHE file in the root directory + * of this source tree. You may select, at your option, one of the + * above-listed licenses. + */ + +use buck2_core::bzl::ImportPath; +use buck2_interpreter_for_build::interpreter::testing::Tester; +use buck2_interpreter_for_build::interpreter::testing::expect_error; + +fn eval_bzl(content: &str) -> buck2_error::Result<()> { + let mut t = Tester::new()?; + t.add_import(&ImportPath::testing_new("root//pkg:defs.bzl"), content)?; + Ok(()) +} + +fn expect_bzl_error(content: &str, expected: &str) { + expect_error(eval_bzl(content), content, expected); +} + +#[test] +fn test_visibility_accepts_package_patterns() -> buck2_error::Result<()> { + eval_bzl(r#"visibility(["//foo:", "//bar/...", "root//baz:"])"#)?; + eval_bzl(r#"visibility("public")"#)?; + eval_bzl(r#"visibility(["PUBLIC"])"#)?; + eval_bzl(r#"visibility("private")"#)?; + eval_bzl(r#"visibility([])"#)?; + Ok(()) +} + +#[test] +fn test_visibility_rejects_target_label() { + expect_bzl_error( + r#"visibility(["//foo:bar"])"#, + "`//foo:bar` is a target label, but `visibility()` takes package patterns (`//pkg:` or `//pkg/...`)", + ); +} + +#[test] +fn test_visibility_rejects_missing_colon() { + expect_bzl_error( + r#"visibility(["//foo"])"#, + "`//foo` is not a package pattern; `visibility()` takes package patterns (`//pkg:` or `//pkg/...`)", + ); +} + +#[test] +fn test_visibility_rejects_wrong_case() { + expect_bzl_error( + r#"visibility("Public")"#, + "`\"Public\"` is not valid in `visibility()`; use `\"PUBLIC\"` or `\"public\"`", + ); + expect_bzl_error( + r#"visibility("PRIVATE")"#, + "`\"PRIVATE\"` is not valid in `visibility()`; use `\"private\"`", + ); +} + +#[test] +fn test_visibility_rejects_negative_pattern() { + expect_bzl_error( + r#"visibility(["//...", "-//x/..."])"#, + "Negative patterns are not supported in `visibility()`: `-//x/...`", + ); +} + +#[test] +fn test_visibility_must_be_top_level() { + expect_bzl_error( + r#" +def _set(): + visibility("private") +_set() +"#, + "`visibility()` must be called at the top level of the `.bzl` file it applies to", + ); +} + +#[test] +fn test_visibility_not_from_loaded_helper() -> buck2_error::Result<()> { + let mut t = Tester::new()?; + t.add_import( + &ImportPath::testing_new("root//helper:helper.bzl"), + r#" +def set_visibility(): + visibility("public") +"#, + )?; + let content = r#" +load("//helper:helper.bzl", "set_visibility") +set_visibility() +"#; + expect_error( + t.add_import(&ImportPath::testing_new("root//pkg:defs.bzl"), content), + content, + "`visibility()` must be called at the top level of the `.bzl` file it applies to", + ); + Ok(()) +} + +#[test] +fn test_visibility_at_most_once() { + expect_bzl_error( + r#" +visibility("public") +visibility("public") +"#, + "`visibility()` can be called at most once per `.bzl` file", + ); +} From c52bc328241b6f1b2940f9c8088fa71163b7f22e Mon Sep 17 00:00:00 2001 From: Cormac Relf Date: Tue, 21 Jul 2026 15:44:33 +1000 Subject: [PATCH 05/10] Add buckconfig buck2.check_load_visibility to disable enforcement --- .../interpreter/functions/load_visibility.rs | 44 +++++++++++++++++++ .../interpreter/global_interpreter_state.rs | 8 ++++ .../src/interpreter/testing.rs | 2 + 3 files changed, 54 insertions(+) diff --git a/app/buck2_interpreter_for_build/src/interpreter/functions/load_visibility.rs b/app/buck2_interpreter_for_build/src/interpreter/functions/load_visibility.rs index 993fd9a5ab80a..41c834b791ab9 100644 --- a/app/buck2_interpreter_for_build/src/interpreter/functions/load_visibility.rs +++ b/app/buck2_interpreter_for_build/src/interpreter/functions/load_visibility.rs @@ -8,6 +8,10 @@ * above-listed licenses. */ +use async_trait::async_trait; +use buck2_common::legacy_configs::dice::HasLegacyConfigs; +use buck2_common::legacy_configs::key::BuckconfigKeyRef; +use buck2_common::legacy_configs::view::LegacyBuckConfigView; use buck2_core::cells::CellAliasResolver; use buck2_core::cells::CellResolver; use buck2_core::cells::name::CellName; @@ -15,6 +19,7 @@ use buck2_core::pattern::pattern::ParsedPattern; use buck2_node::visibility::LoadVisibility; use buck2_node::visibility::VisibilityPattern; use buck2_node::visibility::VisibilityWithinViewBuilder; +use dice::DiceComputations; use either::Either; use starlark::environment::GlobalsBuilder; use starlark::eval::Evaluator; @@ -25,6 +30,45 @@ use starlark::values::none::NoneType; use crate::interpreter::build_context::BuildContext; use crate::interpreter::extra_value::InterpreterExtraValue; +/// Buckconfig key (`buck2.check_load_visibility`) gating enforcement of load +/// visibility declared via the `visibility()` builtin in `.bzl` files. Defaults +/// to `true`: only files that call `visibility()` are restricted, so existing +/// code is unaffected. Set it to `false` to disable enforcement. +const CHECK_LOAD_VISIBILITY: BuckconfigKeyRef = BuckconfigKeyRef { + section: "buck2", + property: "check_load_visibility", +}; + +pub(crate) trait GetCheckLoadVisibility { + /// Whether to enforce load visibility declared via `visibility()`. + fn get_check_load_visibility(&mut self) -> buck2_error::Result; +} + +impl GetCheckLoadVisibility for T +where + T: LegacyBuckConfigView, +{ + fn get_check_load_visibility(&mut self) -> buck2_error::Result { + Ok(self.parse::(CHECK_LOAD_VISIBILITY)?.unwrap_or(true)) + } +} + +#[async_trait] +pub(crate) trait HasCheckLoadVisibility { + /// Reads `buck2.check_load_visibility` from the root buckconfig. + async fn get_check_load_visibility(&mut self) -> buck2_error::Result; +} + +#[async_trait] +impl HasCheckLoadVisibility for DiceComputations<'_> { + async fn get_check_load_visibility(&mut self) -> buck2_error::Result { + self.get_legacy_root_config_on_dice() + .await? + .view(self) + .get_check_load_visibility() + } +} + #[derive(Debug, buck2_error::Error)] #[buck2(tag = Input)] enum LoadVisibilityError { diff --git a/app/buck2_interpreter_for_build/src/interpreter/global_interpreter_state.rs b/app/buck2_interpreter_for_build/src/interpreter/global_interpreter_state.rs index 14252f9c0d2b4..f667b34351e10 100644 --- a/app/buck2_interpreter_for_build/src/interpreter/global_interpreter_state.rs +++ b/app/buck2_interpreter_for_build/src/interpreter/global_interpreter_state.rs @@ -30,6 +30,7 @@ use starlark::environment::Globals; use crate::interpreter::configuror::BuildInterpreterConfiguror; use crate::interpreter::context::HasInterpreterContext; +use crate::interpreter::functions::load_visibility::HasCheckLoadVisibility; use crate::interpreter::globals::base_globals; pagable::static_str!(GLOBAL_ENV_HEAP_NAME = concat!(module_path!(), "::global_env")); @@ -47,6 +48,9 @@ pub struct GlobalInterpreterState { /// Interpreter Configurer pub configuror: Arc, + /// Whether to enforce load visibility declared via `visibility()`. + pub check_load_visibility: bool, + /// Check types in Starlark (or just parse and ignore). pub disable_starlark_types: bool, @@ -58,6 +62,7 @@ impl GlobalInterpreterState { pub fn new( cell_resolver: CellResolver, interpreter_configuror: Arc, + check_load_visibility: bool, disable_starlark_types: bool, unstable_typecheck: bool, ) -> buck2_error::Result { @@ -75,6 +80,7 @@ impl GlobalInterpreterState { cell_resolver, global_env, configuror: interpreter_configuror, + check_load_visibility, disable_starlark_types, unstable_typecheck, }) @@ -126,12 +132,14 @@ impl<'d> HasGlobalInterpreterState<'d> for DiceComputations<'d> { ) -> Self::Value { let interpreter_configuror = ctx.get_interpreter_configuror().await?.dupe(); let cell_resolver = ctx.get_cell_resolver().await?.dupe(); + let check_load_visibility = ctx.get_check_load_visibility().await?; let disable_starlark_types = ctx.get_disable_starlark_types().await?; let unstable_typecheck = ctx.get_unstable_typecheck().await?; Ok(Arc::new(GlobalInterpreterState::new( cell_resolver, interpreter_configuror, + check_load_visibility, disable_starlark_types, unstable_typecheck, )?)) diff --git a/app/buck2_interpreter_for_build/src/interpreter/testing.rs b/app/buck2_interpreter_for_build/src/interpreter/testing.rs index f48855b93201d..6fbfb97f891a5 100644 --- a/app/buck2_interpreter_for_build/src/interpreter/testing.rs +++ b/app/buck2_interpreter_for_build/src/interpreter/testing.rs @@ -53,6 +53,7 @@ use crate::interpreter::cell_info::InterpreterCellInfo; use crate::interpreter::configuror::AdditionalGlobalsFn; use crate::interpreter::configuror::AdditionalGlobalsFnDyn; use crate::interpreter::configuror::BuildInterpreterConfiguror; +use crate::interpreter::functions::load_visibility::GetCheckLoadVisibility; #[derive(Allocative, PagablePanic)] // test only struct FnWrapper(#[allocative(skip)] Box); @@ -231,6 +232,7 @@ impl Tester { }, ))))), )?, + (&self.root_config).get_check_load_visibility()?, false, true, )?), From d46aaa9c5f37fefd28a82be023f7af668b3d6dd0 Mon Sep 17 00:00:00 2001 From: Cormac Relf Date: Tue, 21 Jul 2026 15:46:34 +1000 Subject: [PATCH 06/10] Enforce load visibility --- .../interpreter/dice_calculation_delegate.rs | 68 +++++++++++++++++++ .../interpreter/functions/load_visibility.rs | 14 +++- 2 files changed, 81 insertions(+), 1 deletion(-) diff --git a/app/buck2_interpreter_for_build/src/interpreter/dice_calculation_delegate.rs b/app/buck2_interpreter_for_build/src/interpreter/dice_calculation_delegate.rs index 11cc790c57e55..2715670b78adb 100644 --- a/app/buck2_interpreter_for_build/src/interpreter/dice_calculation_delegate.rs +++ b/app/buck2_interpreter_for_build/src/interpreter/dice_calculation_delegate.rs @@ -64,6 +64,8 @@ use starlark::values::FrozenHeapName; use crate::interpreter::buckconfig::ConfigsOnDiceViewForStarlark; use crate::interpreter::cell_info::InterpreterCellInfo; use crate::interpreter::check_starlark_stack_size::check_starlark_stack_size; +use crate::interpreter::extra_value::FrozenInterpreterExtraValue; +use crate::interpreter::functions::load_visibility::LoadVisibilityError; use crate::interpreter::global_interpreter_state::HasGlobalInterpreterState; use crate::interpreter::interpreter_for_dir::InterpreterForDir; use crate::interpreter::interpreter_for_dir::ParseData; @@ -279,9 +281,75 @@ impl<'c, 'd: 'c> DiceCalculationDelegate<'c, 'd> { ) -> buck2_error::Result<(AstModule, ModuleDeps)> { let ParseData(ast, imports) = self.parse_file(starlark_file).await??; let deps = Self::eval_deps(self.ctx, &imports).await?; + self.check_load_visibility(starlark_file, &imports, &deps) + .await?; Ok((ast, deps)) } + /// Enforce load visibility declared via the `visibility()` builtin: a `.bzl` + /// file with a non-public `visibility()` may only be `load()`ed from files in + /// packages that its visibility list matches (its own package is always + /// allowed). Files that never call `visibility()` are public. Can be + /// disabled with `buck2.check_load_visibility = false`. + async fn check_load_visibility( + &mut self, + starlark_file: StarlarkPath<'_>, + imports: &[(Option, OwnedStarlarkModulePath)], + deps: &ModuleDeps, + ) -> buck2_error::Result<()> { + if !self + .ctx + .get_global_interpreter_state() + .await? + .check_load_visibility + { + return Ok(()); + } + + // The package (directory) of the loading file. The cell root has no + // parent; nothing there can be restricted, so allow it. + let loader_path = starlark_file.path().into_owned(); + let Some(loader_dir) = loader_path.parent() else { + return Ok(()); + }; + + // `imports` and `deps` are in the same order (`eval_deps` maps over + // `imports`), so zipping pairs each load with its resolved module. + for ((span, _), module) in imports.iter().zip(deps.0.iter()) { + let StarlarkModulePath::LoadFile(imp) = module.path() else { + // Only `.bzl` files declare load visibility. + continue; + }; + // A file's own package may always load it. + if loader_dir == imp.path_parent() { + continue; + } + let Some(visibility) = FrozenInterpreterExtraValue::load_visibility(module.env())? + else { + // No `visibility()` call: public by default. + continue; + }; + if !visibility.matches_package(loader_dir) { + return Err(buck2_error::Error::from(LoadVisibilityError::NotVisible { + loader: loader_path.clone(), + loaded: imp.path().clone(), + visibility: visibility.to_string(), + pattern: format!("{loader_dir}:"), + })) + .with_buck_error_context(|| { + format!( + "From load at {}", + span.as_ref().map_or("implicit location".to_owned(), |s| s + .resolve() + .begin_file_line() + .to_string()) + ) + }); + } + } + Ok(()) + } + pub fn prepare_eval_with_content( &self, starlark_file: StarlarkPath<'_>, diff --git a/app/buck2_interpreter_for_build/src/interpreter/functions/load_visibility.rs b/app/buck2_interpreter_for_build/src/interpreter/functions/load_visibility.rs index 41c834b791ab9..f5f867b6a4be2 100644 --- a/app/buck2_interpreter_for_build/src/interpreter/functions/load_visibility.rs +++ b/app/buck2_interpreter_for_build/src/interpreter/functions/load_visibility.rs @@ -14,6 +14,7 @@ use buck2_common::legacy_configs::key::BuckconfigKeyRef; use buck2_common::legacy_configs::view::LegacyBuckConfigView; use buck2_core::cells::CellAliasResolver; use buck2_core::cells::CellResolver; +use buck2_core::cells::cell_path::CellPath; use buck2_core::cells::name::CellName; use buck2_core::pattern::pattern::ParsedPattern; use buck2_node::visibility::LoadVisibility; @@ -71,7 +72,7 @@ impl HasCheckLoadVisibility for DiceComputations<'_> { #[derive(Debug, buck2_error::Error)] #[buck2(tag = Input)] -enum LoadVisibilityError { +pub(crate) enum LoadVisibilityError { #[error( "`visibility()` must be called at the top level of the `.bzl` file it applies to, not from inside a function" )] @@ -92,6 +93,17 @@ enum LoadVisibilityError { NegativePattern(String), #[error("`\"private\"` must be the only entry when passed to `visibility()`")] PrivateNotAlone, + #[error( + "`{loader}` cannot load `{loaded}`: it is not visible to this package. `{loaded}` declares `visibility({visibility})`; to allow this load, its `visibility()` must include `\"{pattern}\"`." + )] + NotVisible { + loader: CellPath, + loaded: CellPath, + visibility: String, + /// The package pattern of the loader, as it would be written in the + /// loaded file's `visibility()`. + pattern: String, + }, } /// The `"private"` special token, meaning "loadable only from the same package". From c242450a9595911bc1c013ea7633c95a8394b9a6 Mon Sep 17 00:00:00 2001 From: Cormac Relf Date: Tue, 21 Jul 2026 15:51:54 +1000 Subject: [PATCH 07/10] Add tests for load visibility --- .../core/interpreter/test_load_visibility.py | 95 +++++++++++++++++++ .../test_load_visibility_data/.buckconfig | 13 +++ .../test_load_visibility_data/.buckroot | 0 .../app/sub/TARGETS.fixture | 8 ++ .../bad_usage/.buckconfig | 8 ++ .../bad_usage/.buckroot | 0 .../bad_usage/in_function/TARGETS.fixture | 3 + .../bad_usage/in_function/defs.bzl | 7 ++ .../bad_usage/missing_colon/TARGETS.fixture | 3 + .../bad_usage/missing_colon/defs.bzl | 4 + .../bad_usage/negative/TARGETS.fixture | 3 + .../bad_usage/negative/defs.bzl | 4 + .../bad_usage/prelude.bzl | 0 .../bad_usage/target_label/TARGETS.fixture | 3 + .../bad_usage/target_label/defs.bzl | 4 + .../bad_usage/wrong_case/TARGETS.fixture | 3 + .../bad_usage/wrong_case/defs.bzl | 4 + .../denied/.buckconfig | 8 ++ .../denied/.buckroot | 0 .../denied/lib/TARGETS.fixture | 4 + .../denied/lib/priv.bzl | 4 + .../denied/other/TARGETS.fixture | 5 + .../denied/prelude.bzl | 0 .../indirect/.buckconfig | 8 ++ .../indirect/.buckroot | 0 .../indirect/TARGETS.fixture | 3 + .../indirect/defs.bzl | 6 ++ .../indirect/helper.bzl | 4 + .../indirect/prelude.bzl | 0 .../lib/TARGETS.fixture | 7 ++ .../test_load_visibility_data/lib/default.bzl | 2 + .../test_load_visibility_data/lib/priv.bzl | 4 + .../test_load_visibility_data/lib/pub.bzl | 4 + .../test_load_visibility_data/lib/vis.bzl | 4 + .../test_load_visibility_data/prelude.bzl | 0 .../twice/.buckconfig | 8 ++ .../test_load_visibility_data/twice/.buckroot | 0 .../twice/TARGETS.fixture | 3 + .../test_load_visibility_data/twice/defs.bzl | 5 + .../twice/prelude.bzl | 0 40 files changed, 241 insertions(+) create mode 100644 tests/core/interpreter/test_load_visibility.py create mode 100644 tests/core/interpreter/test_load_visibility_data/.buckconfig create mode 100644 tests/core/interpreter/test_load_visibility_data/.buckroot create mode 100644 tests/core/interpreter/test_load_visibility_data/app/sub/TARGETS.fixture create mode 100644 tests/core/interpreter/test_load_visibility_data/bad_usage/.buckconfig create mode 100644 tests/core/interpreter/test_load_visibility_data/bad_usage/.buckroot create mode 100644 tests/core/interpreter/test_load_visibility_data/bad_usage/in_function/TARGETS.fixture create mode 100644 tests/core/interpreter/test_load_visibility_data/bad_usage/in_function/defs.bzl create mode 100644 tests/core/interpreter/test_load_visibility_data/bad_usage/missing_colon/TARGETS.fixture create mode 100644 tests/core/interpreter/test_load_visibility_data/bad_usage/missing_colon/defs.bzl create mode 100644 tests/core/interpreter/test_load_visibility_data/bad_usage/negative/TARGETS.fixture create mode 100644 tests/core/interpreter/test_load_visibility_data/bad_usage/negative/defs.bzl create mode 100644 tests/core/interpreter/test_load_visibility_data/bad_usage/prelude.bzl create mode 100644 tests/core/interpreter/test_load_visibility_data/bad_usage/target_label/TARGETS.fixture create mode 100644 tests/core/interpreter/test_load_visibility_data/bad_usage/target_label/defs.bzl create mode 100644 tests/core/interpreter/test_load_visibility_data/bad_usage/wrong_case/TARGETS.fixture create mode 100644 tests/core/interpreter/test_load_visibility_data/bad_usage/wrong_case/defs.bzl create mode 100644 tests/core/interpreter/test_load_visibility_data/denied/.buckconfig create mode 100644 tests/core/interpreter/test_load_visibility_data/denied/.buckroot create mode 100644 tests/core/interpreter/test_load_visibility_data/denied/lib/TARGETS.fixture create mode 100644 tests/core/interpreter/test_load_visibility_data/denied/lib/priv.bzl create mode 100644 tests/core/interpreter/test_load_visibility_data/denied/other/TARGETS.fixture create mode 100644 tests/core/interpreter/test_load_visibility_data/denied/prelude.bzl create mode 100644 tests/core/interpreter/test_load_visibility_data/indirect/.buckconfig create mode 100644 tests/core/interpreter/test_load_visibility_data/indirect/.buckroot create mode 100644 tests/core/interpreter/test_load_visibility_data/indirect/TARGETS.fixture create mode 100644 tests/core/interpreter/test_load_visibility_data/indirect/defs.bzl create mode 100644 tests/core/interpreter/test_load_visibility_data/indirect/helper.bzl create mode 100644 tests/core/interpreter/test_load_visibility_data/indirect/prelude.bzl create mode 100644 tests/core/interpreter/test_load_visibility_data/lib/TARGETS.fixture create mode 100644 tests/core/interpreter/test_load_visibility_data/lib/default.bzl create mode 100644 tests/core/interpreter/test_load_visibility_data/lib/priv.bzl create mode 100644 tests/core/interpreter/test_load_visibility_data/lib/pub.bzl create mode 100644 tests/core/interpreter/test_load_visibility_data/lib/vis.bzl create mode 100644 tests/core/interpreter/test_load_visibility_data/prelude.bzl create mode 100644 tests/core/interpreter/test_load_visibility_data/twice/.buckconfig create mode 100644 tests/core/interpreter/test_load_visibility_data/twice/.buckroot create mode 100644 tests/core/interpreter/test_load_visibility_data/twice/TARGETS.fixture create mode 100644 tests/core/interpreter/test_load_visibility_data/twice/defs.bzl create mode 100644 tests/core/interpreter/test_load_visibility_data/twice/prelude.bzl diff --git a/tests/core/interpreter/test_load_visibility.py b/tests/core/interpreter/test_load_visibility.py new file mode 100644 index 0000000000000..5795df5b8f1ea --- /dev/null +++ b/tests/core/interpreter/test_load_visibility.py @@ -0,0 +1,95 @@ +# Copyright (c) Meta Platforms, Inc. and affiliates. +# +# This source code is dual-licensed under either the MIT license found in the +# LICENSE-MIT file in the root directory of this source tree or the Apache +# License, Version 2.0 found in the LICENSE-APACHE file in the root directory +# of this source tree. You may select, at your option, one of the +# above-listed licenses. + +# pyre-strict + + +from buck2.tests.e2e_util.api.buck import Buck +from buck2.tests.e2e_util.asserts import expect_failure +from buck2.tests.e2e_util.buck_workspace import buck_test, BuckTestCase + + +class LoadVisibilityTest(BuckTestCase): + @buck_test() + async def test_load_visibility_allowed(self, buck: Buck) -> None: + """Loads that satisfy the declared `visibility()` are + permitted: `//app/sub` is within `//app/...`, public/default `.bzl` files + load from anywhere, and same-package loads (including of a private `.bzl`) + always work.""" + await buck.targets("//...") + + @buck_test(data_dir="denied") + async def test_load_visibility_denied(self, buck: Buck) -> None: + """Loading a private (`visibility("private")`) `.bzl` + from another package is rejected. The error shows the declared visibility + as written and the pattern that would allow the load.""" + failure = await expect_failure( + buck.targets("//..."), + stderr_regex="not visible to this package", + ) + self.assertIn('declares `visibility("private")`', failure.stderr) + self.assertIn( + 'its `visibility()` must include `"root//other:"`', failure.stderr + ) + + @buck_test(data_dir="denied") + async def test_load_visibility_gate_off(self, buck: Buck) -> None: + """The same cross-package private load is allowed when enforcement is + disabled with `buck2.check_load_visibility=false`.""" + await buck.targets("//...", "-c", "buck2.check_load_visibility=false") + + @buck_test(data_dir="twice") + async def test_load_visibility_at_most_once(self, buck: Buck) -> None: + """`visibility()` may be called at most once per file. This is enforced at + evaluation time, independent of the enforcement buckconfig.""" + await expect_failure( + buck.targets("//..."), + stderr_regex="at most once", + ) + + @buck_test(data_dir="indirect") + async def test_load_visibility_must_be_direct(self, buck: Buck) -> None: + """`visibility()` must be called directly in the `.bzl` file it applies to, + not delegated to a loaded helper function.""" + await expect_failure( + buck.targets("//..."), + stderr_regex="must be called at the top level of the `.bzl` file", + ) + + @buck_test(data_dir="bad_usage") + async def test_load_visibility_in_function(self, buck: Buck) -> None: + """`visibility()` must be called at the top level, not from a function + defined in the same file.""" + await expect_failure( + buck.targets("//in_function:"), + stderr_regex="must be called at the top level of the `.bzl` file", + ) + + @buck_test(data_dir="bad_usage") + async def test_load_visibility_bad_patterns(self, buck: Buck) -> None: + """Entries that are not package patterns are rejected with a hint.""" + for package, message in [ + ( + "target_label", + "`//other:foo` is a target label, but `visibility()` takes package patterns (`//pkg:` or `//pkg/...`)", + ), + ( + "missing_colon", + "`//other` is not a package pattern; `visibility()` takes package patterns (`//pkg:` or `//pkg/...`)", + ), + ( + "wrong_case", + '`"Public"` is not valid in `visibility()`; use `"PUBLIC"` or `"public"`', + ), + ( + "negative", + "Negative patterns are not supported in `visibility()`: `-//other/...`", + ), + ]: + failure = await expect_failure(buck.targets(f"//{package}:")) + self.assertIn(message, failure.stderr) diff --git a/tests/core/interpreter/test_load_visibility_data/.buckconfig b/tests/core/interpreter/test_load_visibility_data/.buckconfig new file mode 100644 index 0000000000000..fb3fea603014e --- /dev/null +++ b/tests/core/interpreter/test_load_visibility_data/.buckconfig @@ -0,0 +1,13 @@ +[buildfile] +name=TARGETS.fixture + +[repositories] +root = . + +[repository_aliases] +prelude = root + +[project] +# Isolated mini-repos used only by the denied / bad-usage tests (each selected +# via data_dir); ignored so the default `//...` tests skip them. +ignore = denied, twice, indirect, bad_usage diff --git a/tests/core/interpreter/test_load_visibility_data/.buckroot b/tests/core/interpreter/test_load_visibility_data/.buckroot new file mode 100644 index 0000000000000..e69de29bb2d1d diff --git a/tests/core/interpreter/test_load_visibility_data/app/sub/TARGETS.fixture b/tests/core/interpreter/test_load_visibility_data/app/sub/TARGETS.fixture new file mode 100644 index 0000000000000..249d11ae47a7e --- /dev/null +++ b/tests/core/interpreter/test_load_visibility_data/app/sub/TARGETS.fixture @@ -0,0 +1,8 @@ +# `//app/sub` is within `//app/...`, so it may load `vis.bzl`; `pub.bzl` and +# `default.bzl` are public. + +load("//lib/default.bzl", "DSYM") +load("//lib/pub.bzl", "PSYM") +load("//lib/vis.bzl", "SYM") + +_all = (SYM, PSYM, DSYM) diff --git a/tests/core/interpreter/test_load_visibility_data/bad_usage/.buckconfig b/tests/core/interpreter/test_load_visibility_data/bad_usage/.buckconfig new file mode 100644 index 0000000000000..ba0b310bac9ec --- /dev/null +++ b/tests/core/interpreter/test_load_visibility_data/bad_usage/.buckconfig @@ -0,0 +1,8 @@ +[buildfile] +name=TARGETS.fixture + +[repositories] +root = . + +[repository_aliases] +prelude = root diff --git a/tests/core/interpreter/test_load_visibility_data/bad_usage/.buckroot b/tests/core/interpreter/test_load_visibility_data/bad_usage/.buckroot new file mode 100644 index 0000000000000..e69de29bb2d1d diff --git a/tests/core/interpreter/test_load_visibility_data/bad_usage/in_function/TARGETS.fixture b/tests/core/interpreter/test_load_visibility_data/bad_usage/in_function/TARGETS.fixture new file mode 100644 index 0000000000000..f2be172794635 --- /dev/null +++ b/tests/core/interpreter/test_load_visibility_data/bad_usage/in_function/TARGETS.fixture @@ -0,0 +1,3 @@ +load(":defs.bzl", "X") + +_all = (X,) diff --git a/tests/core/interpreter/test_load_visibility_data/bad_usage/in_function/defs.bzl b/tests/core/interpreter/test_load_visibility_data/bad_usage/in_function/defs.bzl new file mode 100644 index 0000000000000..2c9202441e03e --- /dev/null +++ b/tests/core/interpreter/test_load_visibility_data/bad_usage/in_function/defs.bzl @@ -0,0 +1,7 @@ +# `visibility()` must be a top-level call, not made from inside a function. +def _set(): + visibility("private") + +_set() + +X = 1 diff --git a/tests/core/interpreter/test_load_visibility_data/bad_usage/missing_colon/TARGETS.fixture b/tests/core/interpreter/test_load_visibility_data/bad_usage/missing_colon/TARGETS.fixture new file mode 100644 index 0000000000000..f2be172794635 --- /dev/null +++ b/tests/core/interpreter/test_load_visibility_data/bad_usage/missing_colon/TARGETS.fixture @@ -0,0 +1,3 @@ +load(":defs.bzl", "X") + +_all = (X,) diff --git a/tests/core/interpreter/test_load_visibility_data/bad_usage/missing_colon/defs.bzl b/tests/core/interpreter/test_load_visibility_data/bad_usage/missing_colon/defs.bzl new file mode 100644 index 0000000000000..88cf7ca330cce --- /dev/null +++ b/tests/core/interpreter/test_load_visibility_data/bad_usage/missing_colon/defs.bzl @@ -0,0 +1,4 @@ +# A package pattern needs a trailing `:` (or `/...`). +visibility(["//other"]) + +X = 1 diff --git a/tests/core/interpreter/test_load_visibility_data/bad_usage/negative/TARGETS.fixture b/tests/core/interpreter/test_load_visibility_data/bad_usage/negative/TARGETS.fixture new file mode 100644 index 0000000000000..f2be172794635 --- /dev/null +++ b/tests/core/interpreter/test_load_visibility_data/bad_usage/negative/TARGETS.fixture @@ -0,0 +1,3 @@ +load(":defs.bzl", "X") + +_all = (X,) diff --git a/tests/core/interpreter/test_load_visibility_data/bad_usage/negative/defs.bzl b/tests/core/interpreter/test_load_visibility_data/bad_usage/negative/defs.bzl new file mode 100644 index 0000000000000..0bbaba0f45973 --- /dev/null +++ b/tests/core/interpreter/test_load_visibility_data/bad_usage/negative/defs.bzl @@ -0,0 +1,4 @@ +# Negative patterns are not supported. +visibility(["//...", "-//other/..."]) + +X = 1 diff --git a/tests/core/interpreter/test_load_visibility_data/bad_usage/prelude.bzl b/tests/core/interpreter/test_load_visibility_data/bad_usage/prelude.bzl new file mode 100644 index 0000000000000..e69de29bb2d1d diff --git a/tests/core/interpreter/test_load_visibility_data/bad_usage/target_label/TARGETS.fixture b/tests/core/interpreter/test_load_visibility_data/bad_usage/target_label/TARGETS.fixture new file mode 100644 index 0000000000000..f2be172794635 --- /dev/null +++ b/tests/core/interpreter/test_load_visibility_data/bad_usage/target_label/TARGETS.fixture @@ -0,0 +1,3 @@ +load(":defs.bzl", "X") + +_all = (X,) diff --git a/tests/core/interpreter/test_load_visibility_data/bad_usage/target_label/defs.bzl b/tests/core/interpreter/test_load_visibility_data/bad_usage/target_label/defs.bzl new file mode 100644 index 0000000000000..7563015e6bd9c --- /dev/null +++ b/tests/core/interpreter/test_load_visibility_data/bad_usage/target_label/defs.bzl @@ -0,0 +1,4 @@ +# Target labels are rejected: load visibility takes package patterns. +visibility(["//other:foo"]) + +X = 1 diff --git a/tests/core/interpreter/test_load_visibility_data/bad_usage/wrong_case/TARGETS.fixture b/tests/core/interpreter/test_load_visibility_data/bad_usage/wrong_case/TARGETS.fixture new file mode 100644 index 0000000000000..f2be172794635 --- /dev/null +++ b/tests/core/interpreter/test_load_visibility_data/bad_usage/wrong_case/TARGETS.fixture @@ -0,0 +1,3 @@ +load(":defs.bzl", "X") + +_all = (X,) diff --git a/tests/core/interpreter/test_load_visibility_data/bad_usage/wrong_case/defs.bzl b/tests/core/interpreter/test_load_visibility_data/bad_usage/wrong_case/defs.bzl new file mode 100644 index 0000000000000..ea1bb90f01886 --- /dev/null +++ b/tests/core/interpreter/test_load_visibility_data/bad_usage/wrong_case/defs.bzl @@ -0,0 +1,4 @@ +# Only `"PUBLIC"`, `"public"` and `"private"` are accepted. +visibility("Public") + +X = 1 diff --git a/tests/core/interpreter/test_load_visibility_data/denied/.buckconfig b/tests/core/interpreter/test_load_visibility_data/denied/.buckconfig new file mode 100644 index 0000000000000..ba0b310bac9ec --- /dev/null +++ b/tests/core/interpreter/test_load_visibility_data/denied/.buckconfig @@ -0,0 +1,8 @@ +[buildfile] +name=TARGETS.fixture + +[repositories] +root = . + +[repository_aliases] +prelude = root diff --git a/tests/core/interpreter/test_load_visibility_data/denied/.buckroot b/tests/core/interpreter/test_load_visibility_data/denied/.buckroot new file mode 100644 index 0000000000000..e69de29bb2d1d diff --git a/tests/core/interpreter/test_load_visibility_data/denied/lib/TARGETS.fixture b/tests/core/interpreter/test_load_visibility_data/denied/lib/TARGETS.fixture new file mode 100644 index 0000000000000..038346d184f28 --- /dev/null +++ b/tests/core/interpreter/test_load_visibility_data/denied/lib/TARGETS.fixture @@ -0,0 +1,4 @@ +# Same-package load of the private `.bzl`: allowed. +load(":priv.bzl", "SYM") + +_all = (SYM,) diff --git a/tests/core/interpreter/test_load_visibility_data/denied/lib/priv.bzl b/tests/core/interpreter/test_load_visibility_data/denied/lib/priv.bzl new file mode 100644 index 0000000000000..31c3c04e84698 --- /dev/null +++ b/tests/core/interpreter/test_load_visibility_data/denied/lib/priv.bzl @@ -0,0 +1,4 @@ +# Private: only `//lib` may load it. +visibility("private") + +SYM = 1 diff --git a/tests/core/interpreter/test_load_visibility_data/denied/other/TARGETS.fixture b/tests/core/interpreter/test_load_visibility_data/denied/other/TARGETS.fixture new file mode 100644 index 0000000000000..f4fb8dd0c5840 --- /dev/null +++ b/tests/core/interpreter/test_load_visibility_data/denied/other/TARGETS.fixture @@ -0,0 +1,5 @@ +# Cross-package load of the private `//lib/priv.bzl`: rejected unless +# `buck2.check_load_visibility` is disabled. +load("//lib/priv.bzl", "SYM") + +_all = (SYM,) diff --git a/tests/core/interpreter/test_load_visibility_data/denied/prelude.bzl b/tests/core/interpreter/test_load_visibility_data/denied/prelude.bzl new file mode 100644 index 0000000000000..e69de29bb2d1d diff --git a/tests/core/interpreter/test_load_visibility_data/indirect/.buckconfig b/tests/core/interpreter/test_load_visibility_data/indirect/.buckconfig new file mode 100644 index 0000000000000..ba0b310bac9ec --- /dev/null +++ b/tests/core/interpreter/test_load_visibility_data/indirect/.buckconfig @@ -0,0 +1,8 @@ +[buildfile] +name=TARGETS.fixture + +[repositories] +root = . + +[repository_aliases] +prelude = root diff --git a/tests/core/interpreter/test_load_visibility_data/indirect/.buckroot b/tests/core/interpreter/test_load_visibility_data/indirect/.buckroot new file mode 100644 index 0000000000000..e69de29bb2d1d diff --git a/tests/core/interpreter/test_load_visibility_data/indirect/TARGETS.fixture b/tests/core/interpreter/test_load_visibility_data/indirect/TARGETS.fixture new file mode 100644 index 0000000000000..f2be172794635 --- /dev/null +++ b/tests/core/interpreter/test_load_visibility_data/indirect/TARGETS.fixture @@ -0,0 +1,3 @@ +load(":defs.bzl", "X") + +_all = (X,) diff --git a/tests/core/interpreter/test_load_visibility_data/indirect/defs.bzl b/tests/core/interpreter/test_load_visibility_data/indirect/defs.bzl new file mode 100644 index 0000000000000..146fb48e2a2de --- /dev/null +++ b/tests/core/interpreter/test_load_visibility_data/indirect/defs.bzl @@ -0,0 +1,6 @@ +# Delegating `visibility()` to a loaded helper is rejected. +load(":helper.bzl", "set_visibility") + +set_visibility() + +X = 1 diff --git a/tests/core/interpreter/test_load_visibility_data/indirect/helper.bzl b/tests/core/interpreter/test_load_visibility_data/indirect/helper.bzl new file mode 100644 index 0000000000000..41ae9cb71d37c --- /dev/null +++ b/tests/core/interpreter/test_load_visibility_data/indirect/helper.bzl @@ -0,0 +1,4 @@ +# A helper that calls `visibility()` on behalf of its caller. This is not +# allowed: `visibility()` must be called directly in the file it applies to. +def set_visibility(): + visibility(["PUBLIC"]) diff --git a/tests/core/interpreter/test_load_visibility_data/indirect/prelude.bzl b/tests/core/interpreter/test_load_visibility_data/indirect/prelude.bzl new file mode 100644 index 0000000000000..e69de29bb2d1d diff --git a/tests/core/interpreter/test_load_visibility_data/lib/TARGETS.fixture b/tests/core/interpreter/test_load_visibility_data/lib/TARGETS.fixture new file mode 100644 index 0000000000000..259d2c8735b97 --- /dev/null +++ b/tests/core/interpreter/test_load_visibility_data/lib/TARGETS.fixture @@ -0,0 +1,7 @@ +# Same-package loads: always allowed, including the private `priv.bzl`. + +load(":default.bzl", "DSYM") +load(":priv.bzl", "PRIVSYM") +load(":vis.bzl", "SYM") + +_all = (PRIVSYM, SYM, DSYM) diff --git a/tests/core/interpreter/test_load_visibility_data/lib/default.bzl b/tests/core/interpreter/test_load_visibility_data/lib/default.bzl new file mode 100644 index 0000000000000..cb1295091b16d --- /dev/null +++ b/tests/core/interpreter/test_load_visibility_data/lib/default.bzl @@ -0,0 +1,2 @@ +# No `visibility()` call: public by default, loadable from anywhere. +DSYM = 1 diff --git a/tests/core/interpreter/test_load_visibility_data/lib/priv.bzl b/tests/core/interpreter/test_load_visibility_data/lib/priv.bzl new file mode 100644 index 0000000000000..f2caa42782dbb --- /dev/null +++ b/tests/core/interpreter/test_load_visibility_data/lib/priv.bzl @@ -0,0 +1,4 @@ +# Private: loadable only from its own package (`//lib`). +visibility([]) + +PRIVSYM = 1 diff --git a/tests/core/interpreter/test_load_visibility_data/lib/pub.bzl b/tests/core/interpreter/test_load_visibility_data/lib/pub.bzl new file mode 100644 index 0000000000000..50e677f072a57 --- /dev/null +++ b/tests/core/interpreter/test_load_visibility_data/lib/pub.bzl @@ -0,0 +1,4 @@ +# Explicitly public: loadable from anywhere. +visibility(["PUBLIC"]) + +PSYM = 1 diff --git a/tests/core/interpreter/test_load_visibility_data/lib/vis.bzl b/tests/core/interpreter/test_load_visibility_data/lib/vis.bzl new file mode 100644 index 0000000000000..8fab63b393a74 --- /dev/null +++ b/tests/core/interpreter/test_load_visibility_data/lib/vis.bzl @@ -0,0 +1,4 @@ +# Loadable from `//app/...`, `//lib/...`, and (always) its own package. +visibility(["//app/...", "//lib/..."]) + +SYM = 1 diff --git a/tests/core/interpreter/test_load_visibility_data/prelude.bzl b/tests/core/interpreter/test_load_visibility_data/prelude.bzl new file mode 100644 index 0000000000000..e69de29bb2d1d diff --git a/tests/core/interpreter/test_load_visibility_data/twice/.buckconfig b/tests/core/interpreter/test_load_visibility_data/twice/.buckconfig new file mode 100644 index 0000000000000..ba0b310bac9ec --- /dev/null +++ b/tests/core/interpreter/test_load_visibility_data/twice/.buckconfig @@ -0,0 +1,8 @@ +[buildfile] +name=TARGETS.fixture + +[repositories] +root = . + +[repository_aliases] +prelude = root diff --git a/tests/core/interpreter/test_load_visibility_data/twice/.buckroot b/tests/core/interpreter/test_load_visibility_data/twice/.buckroot new file mode 100644 index 0000000000000..e69de29bb2d1d diff --git a/tests/core/interpreter/test_load_visibility_data/twice/TARGETS.fixture b/tests/core/interpreter/test_load_visibility_data/twice/TARGETS.fixture new file mode 100644 index 0000000000000..f2be172794635 --- /dev/null +++ b/tests/core/interpreter/test_load_visibility_data/twice/TARGETS.fixture @@ -0,0 +1,3 @@ +load(":defs.bzl", "X") + +_all = (X,) diff --git a/tests/core/interpreter/test_load_visibility_data/twice/defs.bzl b/tests/core/interpreter/test_load_visibility_data/twice/defs.bzl new file mode 100644 index 0000000000000..59320238db6ff --- /dev/null +++ b/tests/core/interpreter/test_load_visibility_data/twice/defs.bzl @@ -0,0 +1,5 @@ +# Calling `visibility()` more than once is an error. +visibility(["PUBLIC"]) +visibility(["PUBLIC"]) + +X = 1 diff --git a/tests/core/interpreter/test_load_visibility_data/twice/prelude.bzl b/tests/core/interpreter/test_load_visibility_data/twice/prelude.bzl new file mode 100644 index 0000000000000..e69de29bb2d1d From 7b104d95b710d9896819e3fa36a404c286fbb098 Mon Sep 17 00:00:00 2001 From: Cormac Relf Date: Tue, 21 Jul 2026 17:48:35 +1000 Subject: [PATCH 08/10] Add bxl test for load visibility --- tests/core/interpreter/test_load_visibility.py | 14 ++++++++++++++ .../test_load_visibility_data/.buckconfig | 2 +- .../test_load_visibility_data/bxl/.buckconfig | 12 ++++++++++++ .../test_load_visibility_data/bxl/.buckroot | 0 .../test_load_visibility_data/bxl/app/allowed.bxl | 10 ++++++++++ .../test_load_visibility_data/bxl/app/denied.bxl | 11 +++++++++++ .../test_load_visibility_data/bxl/lib/priv.bzl | 4 ++++ .../test_load_visibility_data/bxl/lib/vis.bzl | 4 ++++ 8 files changed, 56 insertions(+), 1 deletion(-) create mode 100644 tests/core/interpreter/test_load_visibility_data/bxl/.buckconfig create mode 100644 tests/core/interpreter/test_load_visibility_data/bxl/.buckroot create mode 100644 tests/core/interpreter/test_load_visibility_data/bxl/app/allowed.bxl create mode 100644 tests/core/interpreter/test_load_visibility_data/bxl/app/denied.bxl create mode 100644 tests/core/interpreter/test_load_visibility_data/bxl/lib/priv.bzl create mode 100644 tests/core/interpreter/test_load_visibility_data/bxl/lib/vis.bzl diff --git a/tests/core/interpreter/test_load_visibility.py b/tests/core/interpreter/test_load_visibility.py index 5795df5b8f1ea..dc972ccf09465 100644 --- a/tests/core/interpreter/test_load_visibility.py +++ b/tests/core/interpreter/test_load_visibility.py @@ -93,3 +93,17 @@ async def test_load_visibility_bad_patterns(self, buck: Buck) -> None: ]: failure = await expect_failure(buck.targets(f"//{package}:")) self.assertIn(message, failure.stderr) + + @buck_test(data_dir="bxl") + async def test_load_visibility_bxl_allowed(self, buck: Buck) -> None: + """Load visibility of a `.bzl` is enforced for `.bxl` loaders too: a `.bxl` + within the declared visibility may load the `.bzl`.""" + await buck.bxl("//app/allowed.bxl:check") + + @buck_test(data_dir="bxl") + async def test_load_visibility_bxl_denied(self, buck: Buck) -> None: + """A `.bxl` loading a private `.bzl` from another package is rejected.""" + await expect_failure( + buck.bxl("//app/denied.bxl:check"), + stderr_regex="not visible to this package", + ) diff --git a/tests/core/interpreter/test_load_visibility_data/.buckconfig b/tests/core/interpreter/test_load_visibility_data/.buckconfig index fb3fea603014e..47219455bc0c4 100644 --- a/tests/core/interpreter/test_load_visibility_data/.buckconfig +++ b/tests/core/interpreter/test_load_visibility_data/.buckconfig @@ -10,4 +10,4 @@ prelude = root [project] # Isolated mini-repos used only by the denied / bad-usage tests (each selected # via data_dir); ignored so the default `//...` tests skip them. -ignore = denied, twice, indirect, bad_usage +ignore = denied, twice, indirect, bad_usage, bxl diff --git a/tests/core/interpreter/test_load_visibility_data/bxl/.buckconfig b/tests/core/interpreter/test_load_visibility_data/bxl/.buckconfig new file mode 100644 index 0000000000000..09556211287db --- /dev/null +++ b/tests/core/interpreter/test_load_visibility_data/bxl/.buckconfig @@ -0,0 +1,12 @@ +[cells] + root = . + nano_prelude = nano_prelude + +[cell_aliases] + prelude = nano_prelude + +[external_cells] + nano_prelude = bundled + +[buildfile] + name = TARGETS.fixture diff --git a/tests/core/interpreter/test_load_visibility_data/bxl/.buckroot b/tests/core/interpreter/test_load_visibility_data/bxl/.buckroot new file mode 100644 index 0000000000000..e69de29bb2d1d diff --git a/tests/core/interpreter/test_load_visibility_data/bxl/app/allowed.bxl b/tests/core/interpreter/test_load_visibility_data/bxl/app/allowed.bxl new file mode 100644 index 0000000000000..a548bdccd5a12 --- /dev/null +++ b/tests/core/interpreter/test_load_visibility_data/bxl/app/allowed.bxl @@ -0,0 +1,10 @@ +# `//app/allowed.bxl` is within `//app/...`, so it may load `vis.bzl`. +load("//lib/vis.bzl", "SYM2") + +def _impl(ctx: bxl.Context): + ctx.output.print(SYM2) + +check = bxl_main( + impl = _impl, + cli_args = {}, +) diff --git a/tests/core/interpreter/test_load_visibility_data/bxl/app/denied.bxl b/tests/core/interpreter/test_load_visibility_data/bxl/app/denied.bxl new file mode 100644 index 0000000000000..7a6b416070e03 --- /dev/null +++ b/tests/core/interpreter/test_load_visibility_data/bxl/app/denied.bxl @@ -0,0 +1,11 @@ +# `//app/denied.bxl` is in a different package than `//lib`, so loading the +# private `//lib/priv.bzl` is rejected when enforcement is on. +load("//lib/priv.bzl", "SYM") + +def _impl(ctx: bxl.Context): + ctx.output.print(SYM) + +check = bxl_main( + impl = _impl, + cli_args = {}, +) diff --git a/tests/core/interpreter/test_load_visibility_data/bxl/lib/priv.bzl b/tests/core/interpreter/test_load_visibility_data/bxl/lib/priv.bzl new file mode 100644 index 0000000000000..1a861898b6a94 --- /dev/null +++ b/tests/core/interpreter/test_load_visibility_data/bxl/lib/priv.bzl @@ -0,0 +1,4 @@ +# Private: only `//lib` may load it. +visibility([]) + +SYM = 1 diff --git a/tests/core/interpreter/test_load_visibility_data/bxl/lib/vis.bzl b/tests/core/interpreter/test_load_visibility_data/bxl/lib/vis.bzl new file mode 100644 index 0000000000000..6b24fa1faf230 --- /dev/null +++ b/tests/core/interpreter/test_load_visibility_data/bxl/lib/vis.bzl @@ -0,0 +1,4 @@ +# Visible to `//app/...`. +visibility(["//app/..."]) + +SYM2 = 1 From 65ba13f6327616f8c4e3a1db9d412969c8c9ba98 Mon Sep 17 00:00:00 2001 From: Cormac Relf Date: Fri, 30 Oct 2026 15:36:06 +1100 Subject: [PATCH 09/10] Find owning packages through buildfile-existence keys Add `HasBuildfileKey` and `EnclosingPackageKey`, which only change when a buildfile is added or removed, and use them for `get_enclosing_packages` (`owner()`) and the new `find_enclosing_package`, instead of reading every directory up to the package. The basic problem being solved is that checking load-visibility using package semantics rather than directory semantics requires finding the package of the importing bzl file and also the one it's importing. A naive implementation would recursively read_dir each parent directory until it found a BUCK/buildfile, but that would be bad because then every single load would have to be re-checked whenever you added or removed any file basically anywhere. This limits re-checking to the addition and deletion of buildfiles i.e. changes to the package structure. A subsequent commit will alter the semantics. For now, we swap out all the places we currently try to find the owning buildfile. This should be a perf win under fs changes. - `get_enclosing_package` (bxl, `audit file-package`, `buildfile()`) - `get_enclosing_packages` (`owner()`) I ran a benchmark with large numbers of loads nested within a package etc + package semantics (the later commit), and yeah you can make a dumb benchmark where it's twice as fast at re-evaluating stuff. That should help show we aren't introducing perf regressions. --- app/buck2_common/src/package_listing/dice.rs | 130 ++++++++++++++++++ .../src/package_listing/interpreter.rs | 58 ++++---- 2 files changed, 163 insertions(+), 25 deletions(-) diff --git a/app/buck2_common/src/package_listing/dice.rs b/app/buck2_common/src/package_listing/dice.rs index 38ccf28d4b652..4d13a3b000252 100644 --- a/app/buck2_common/src/package_listing/dice.rs +++ b/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; + + 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 { + 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 { + OkPagableValueSerialize::::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>; + + 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 { + 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 { + OkPagableValueSerialize::::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> { + 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> { + ctx.compute(&EnclosingPackageKey(dir)).await?.dupe() +} + #[async_trait] impl PackageListingResolver for DicePackageListingResolver<'_, '_> { async fn resolve(&mut self, package: PackageLabel) -> buck2_error::Result { diff --git a/app/buck2_common/src/package_listing/interpreter.rs b/app/buck2_common/src/package_listing/interpreter.rs index c7024abeec1cd..26b3d71305c3f 100644 --- a/app/buck2_common/src/package_listing/interpreter.rs +++ b/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 { - 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> { - 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) } } From 97e5833acc8c7c51d518e75512ce253b5b0ae2e9 Mon Sep 17 00:00:00 2001 From: Cormac Relf Date: Fri, 30 Oct 2026 15:03:00 +1100 Subject: [PATCH 10/10] Use package semantics for load visibility Package semantics: visibility(["//pkg:"]) is visible from pkg/defs.bzl AND pkg/sub/defs.bzl. Directory semantics: visibility(["//pkg:"]) is visible from pkg/defs.bzl BUT NOT subdirectories thereof. You would have to write //pkg/... which would include pkg/subpackage/BUCK subpackages. We choose package because: > It makes more sense to me to be package based rather than directory. If you > have a module of bzl code you can put some in a subdirectory called private/, > write visibility("private") at the top of each, and then import those from a > public interface. If directory-based, we would require you to write > visibility("//pkg/...") in that very common scenario, "private" would be much > less useful and this feature would be worse. --- .../interpreter/dice_calculation_delegate.rs | 94 ++++++++++++------- .../interpreter/functions/load_visibility.rs | 13 +++ app/buck2_node/src/visibility.rs | 28 ++++-- .../core/interpreter/test_load_visibility.py | 22 ++++- .../bxl/app/TARGETS.fixture | 0 .../bxl/app/allowed.bxl | 2 +- .../bxl/lib/TARGETS.fixture | 0 .../bxl/nopkg/denied.bxl | 11 +++ .../denied/lib/sub/TARGETS.fixture | 5 + .../lib/TARGETS.fixture | 4 +- .../lib/sub/helper.bzl | 7 ++ 11 files changed, 137 insertions(+), 49 deletions(-) create mode 100644 tests/core/interpreter/test_load_visibility_data/bxl/app/TARGETS.fixture create mode 100644 tests/core/interpreter/test_load_visibility_data/bxl/lib/TARGETS.fixture create mode 100644 tests/core/interpreter/test_load_visibility_data/bxl/nopkg/denied.bxl create mode 100644 tests/core/interpreter/test_load_visibility_data/denied/lib/sub/TARGETS.fixture create mode 100644 tests/core/interpreter/test_load_visibility_data/lib/sub/helper.bzl diff --git a/app/buck2_interpreter_for_build/src/interpreter/dice_calculation_delegate.rs b/app/buck2_interpreter_for_build/src/interpreter/dice_calculation_delegate.rs index 2715670b78adb..8f8f63aa1b7f4 100644 --- a/app/buck2_interpreter_for_build/src/interpreter/dice_calculation_delegate.rs +++ b/app/buck2_interpreter_for_build/src/interpreter/dice_calculation_delegate.rs @@ -287,10 +287,10 @@ impl<'c, 'd: 'c> DiceCalculationDelegate<'c, 'd> { } /// Enforce load visibility declared via the `visibility()` builtin: a `.bzl` - /// file with a non-public `visibility()` may only be `load()`ed from files in - /// packages that its visibility list matches (its own package is always - /// allowed). Files that never call `visibility()` are public. Can be - /// disabled with `buck2.check_load_visibility = false`. + /// file with a non-public `visibility()` may only be `load()`ed from its own + /// package or from packages that its visibility list matches. See the + /// `visibility()` builtin for what a file's package is. Can be disabled with + /// `buck2.check_load_visibility = false`. async fn check_load_visibility( &mut self, starlark_file: StarlarkPath<'_>, @@ -306,46 +306,68 @@ impl<'c, 'd: 'c> DiceCalculationDelegate<'c, 'd> { return Ok(()); } - // The package (directory) of the loading file. The cell root has no - // parent; nothing there can be restricted, so allow it. - let loader_path = starlark_file.path().into_owned(); - let Some(loader_dir) = loader_path.parent() else { - return Ok(()); - }; - // `imports` and `deps` are in the same order (`eval_deps` maps over // `imports`), so zipping pairs each load with its resolved module. + let mut restricted = Vec::new(); for ((span, _), module) in imports.iter().zip(deps.0.iter()) { + // Only `.bzl` files declare load visibility, and one that never calls + // `visibility()` is public. let StarlarkModulePath::LoadFile(imp) = module.path() else { - // Only `.bzl` files declare load visibility. continue; }; - // A file's own package may always load it. - if loader_dir == imp.path_parent() { - continue; + if let Some(visibility) = FrozenInterpreterExtraValue::load_visibility(module.env())? + && !visibility.is_public() + { + restricted.push((span, *imp.path(), visibility)); } - let Some(visibility) = FrozenInterpreterExtraValue::load_visibility(module.env())? - else { - // No `visibility()` call: public by default. - continue; - }; - if !visibility.matches_package(loader_dir) { - return Err(buck2_error::Error::from(LoadVisibilityError::NotVisible { - loader: loader_path.clone(), - loaded: imp.path().clone(), - visibility: visibility.to_string(), - pattern: format!("{loader_dir}:"), - })) - .with_buck_error_context(|| { - format!( - "From load at {}", - span.as_ref().map_or("implicit location".to_owned(), |s| s - .resolve() - .begin_file_line() - .to_string()) - ) - }); + } + if restricted.is_empty() { + return Ok(()); + } + + let loader = *starlark_file.path(); + let loader_package = match starlark_file { + StarlarkPath::BuildFile(path) => Some(path.package()), + _ => { + DicePackageListingResolver(self.ctx) + .find_enclosing_package(loader) + .await? } + }; + + for (span, loaded, visibility) in restricted { + let error = match loader_package { + Some(loader_package) => { + if visibility.matches_package(loader_package.to_cell_path()) + || DicePackageListingResolver(self.ctx) + .find_enclosing_package(loaded) + .await? + == Some(loader_package) + { + continue; + } + LoadVisibilityError::NotVisible { + loader, + loaded, + visibility: visibility.to_string(), + pattern: format!("{loader_package}:"), + } + } + None => LoadVisibilityError::NotVisibleNoPackage { + loader, + loaded, + visibility: visibility.to_string(), + }, + }; + return Err(buck2_error::Error::from(error)).with_buck_error_context(|| { + format!( + "From load at {}", + span.as_ref().map_or("implicit location".to_owned(), |s| s + .resolve() + .begin_file_line() + .to_string()) + ) + }); } Ok(()) } diff --git a/app/buck2_interpreter_for_build/src/interpreter/functions/load_visibility.rs b/app/buck2_interpreter_for_build/src/interpreter/functions/load_visibility.rs index f5f867b6a4be2..a203882d36d5f 100644 --- a/app/buck2_interpreter_for_build/src/interpreter/functions/load_visibility.rs +++ b/app/buck2_interpreter_for_build/src/interpreter/functions/load_visibility.rs @@ -104,6 +104,14 @@ pub(crate) enum LoadVisibilityError { /// loaded file's `visibility()`. pattern: String, }, + #[error( + "`{loader}` cannot load `{loaded}`: `{loaded}` declares `visibility({visibility})`, but `{loader}` is not in any package (there is no buildfile in its directory or any parent directory). Add a buildfile so it belongs to a package that `{loaded}`'s `visibility()` can name." + )] + NotVisibleNoPackage { + loader: CellPath, + loaded: CellPath, + visibility: String, + }, } /// The `"private"` special token, meaning "loadable only from the same package". @@ -178,6 +186,11 @@ pub(crate) fn register_load_visibility(globals: &mut GlobalsBuilder) { /// * `visibility(["//foo/...", "//bar:"])` — loadable from the given /// packages (and, always, the file's own package). /// + /// As in Bazel, the package of a file is the nearest directory at or above + /// it that contains a buildfile, so `//foo/sub/defs.bzl` is in package + /// `//foo:` unless `foo/sub` has its own buildfile. A file outside of any + /// package can only load public `.bzl` files. + /// /// It must be called at most once, at the top level of the `.bzl` file it /// applies to (not inside a function). Enforcement of the resulting /// visibility can be disabled with `buck2.check_load_visibility = false`. diff --git a/app/buck2_node/src/visibility.rs b/app/buck2_node/src/visibility.rs index 586b7817722f8..3f4065cf007b8 100644 --- a/app/buck2_node/src/visibility.rs +++ b/app/buck2_node/src/visibility.rs @@ -454,10 +454,10 @@ impl VisibilityPatternList { } } - /// Whether this list matches a package (the directory of a loading file), - /// for load visibility. `target_name_glob(...)` patterns are target-oriented - /// and never match a bare package, so they are treated as non-matching (they - /// also cannot be produced by the `visibility()` builtin, which only accepts + /// Whether this list matches the package of a loading file, for load + /// visibility. `target_name_glob(...)` patterns are target-oriented and never + /// match a bare package, so they are treated as non-matching (they also + /// cannot be produced by the `visibility()` builtin, which only accepts /// strings). pub fn matches_package(&self, package: CellPath) -> bool { match self { @@ -818,10 +818,10 @@ impl Display for WithinViewSpecification { /// The load visibility of a `.bzl` file: the set of packages whose `BUCK`/`.bzl` /// files may `load()` it, as declared by the `visibility()` builtin. Unlike -/// target visibility this is matched against the *package* (directory) of the -/// loading file rather than a target label. When a `.bzl` file never calls -/// `visibility()` there is no `LoadVisibility` at all, which is treated as -/// public (matching Bazel). +/// target visibility this is matched against the *package* of the loading file +/// rather than a target label. When a `.bzl` file never calls `visibility()` +/// there is no `LoadVisibility` at all, which is treated as public (matching +/// Bazel). #[derive(Debug, Eq, PartialEq, Hash, Clone, Dupe, Allocative, Pagable)] pub enum LoadVisibility { /// `visibility("private")`: loadable only from the same package (enforced @@ -847,8 +847,16 @@ impl LoadVisibility { LoadVisibility::Private } - /// Whether a file in `package` (its directory) may load the `.bzl` file that - /// declared this visibility. + /// Whether this is `visibility(["PUBLIC"])` / `visibility("public")`. + pub fn is_public(&self) -> bool { + matches!( + self, + LoadVisibility::Patterns(VisibilityPatternList::Public) + ) + } + + /// Whether a file in `package` may load the `.bzl` file that declared this + /// visibility. pub fn matches_package(&self, package: CellPath) -> bool { match self { LoadVisibility::Private => false, diff --git a/tests/core/interpreter/test_load_visibility.py b/tests/core/interpreter/test_load_visibility.py index dc972ccf09465..0994660854f87 100644 --- a/tests/core/interpreter/test_load_visibility.py +++ b/tests/core/interpreter/test_load_visibility.py @@ -29,7 +29,7 @@ async def test_load_visibility_denied(self, buck: Buck) -> None: from another package is rejected. The error shows the declared visibility as written and the pattern that would allow the load.""" failure = await expect_failure( - buck.targets("//..."), + buck.targets("//other:"), stderr_regex="not visible to this package", ) self.assertIn('declares `visibility("private")`', failure.stderr) @@ -37,6 +37,18 @@ async def test_load_visibility_denied(self, buck: Buck) -> None: 'its `visibility()` must include `"root//other:"`', failure.stderr ) + @buck_test(data_dir="denied") + async def test_load_visibility_subpackage_denied(self, buck: Buck) -> None: + """A subdirectory with its own buildfile is a different package, so it + may not load its parent package's private `.bzl`.""" + failure = await expect_failure( + buck.targets("//lib/sub:"), + stderr_regex="not visible to this package", + ) + self.assertIn( + 'its `visibility()` must include `"root//lib/sub:"`', failure.stderr + ) + @buck_test(data_dir="denied") async def test_load_visibility_gate_off(self, buck: Buck) -> None: """The same cross-package private load is allowed when enforcement is @@ -107,3 +119,11 @@ async def test_load_visibility_bxl_denied(self, buck: Buck) -> None: buck.bxl("//app/denied.bxl:check"), stderr_regex="not visible to this package", ) + + @buck_test(data_dir="bxl") + async def test_load_visibility_no_package_denied(self, buck: Buck) -> None: + """A `.bxl` outside of any package may only load public `.bzl` files.""" + await expect_failure( + buck.bxl("//nopkg/denied.bxl:check"), + stderr_regex="is not in any package", + ) diff --git a/tests/core/interpreter/test_load_visibility_data/bxl/app/TARGETS.fixture b/tests/core/interpreter/test_load_visibility_data/bxl/app/TARGETS.fixture new file mode 100644 index 0000000000000..e69de29bb2d1d diff --git a/tests/core/interpreter/test_load_visibility_data/bxl/app/allowed.bxl b/tests/core/interpreter/test_load_visibility_data/bxl/app/allowed.bxl index a548bdccd5a12..60dd7c9c380ae 100644 --- a/tests/core/interpreter/test_load_visibility_data/bxl/app/allowed.bxl +++ b/tests/core/interpreter/test_load_visibility_data/bxl/app/allowed.bxl @@ -1,4 +1,4 @@ -# `//app/allowed.bxl` is within `//app/...`, so it may load `vis.bzl`. +# `//app/allowed.bxl` is in package `//app`, within `//app/...`, so it may load `vis.bzl`. load("//lib/vis.bzl", "SYM2") def _impl(ctx: bxl.Context): diff --git a/tests/core/interpreter/test_load_visibility_data/bxl/lib/TARGETS.fixture b/tests/core/interpreter/test_load_visibility_data/bxl/lib/TARGETS.fixture new file mode 100644 index 0000000000000..e69de29bb2d1d diff --git a/tests/core/interpreter/test_load_visibility_data/bxl/nopkg/denied.bxl b/tests/core/interpreter/test_load_visibility_data/bxl/nopkg/denied.bxl new file mode 100644 index 0000000000000..12788935459c3 --- /dev/null +++ b/tests/core/interpreter/test_load_visibility_data/bxl/nopkg/denied.bxl @@ -0,0 +1,11 @@ +# There is no buildfile in `nopkg` or any parent, so this file is not in any +# package and may only load public `.bzl` files. +load("//lib/vis.bzl", "SYM2") + +def _impl(ctx: bxl.Context): + ctx.output.print(SYM2) + +check = bxl_main( + impl = _impl, + cli_args = {}, +) diff --git a/tests/core/interpreter/test_load_visibility_data/denied/lib/sub/TARGETS.fixture b/tests/core/interpreter/test_load_visibility_data/denied/lib/sub/TARGETS.fixture new file mode 100644 index 0000000000000..d328bccc2139e --- /dev/null +++ b/tests/core/interpreter/test_load_visibility_data/denied/lib/sub/TARGETS.fixture @@ -0,0 +1,5 @@ +# `//lib/sub` has its own buildfile, so it is a different package from +# `//lib` and may not load the private `//lib/priv.bzl`. +load("//lib:priv.bzl", "SYM") + +_all = (SYM,) diff --git a/tests/core/interpreter/test_load_visibility_data/lib/TARGETS.fixture b/tests/core/interpreter/test_load_visibility_data/lib/TARGETS.fixture index 259d2c8735b97..242feb7b58b98 100644 --- a/tests/core/interpreter/test_load_visibility_data/lib/TARGETS.fixture +++ b/tests/core/interpreter/test_load_visibility_data/lib/TARGETS.fixture @@ -1,7 +1,9 @@ # Same-package loads: always allowed, including the private `priv.bzl`. +# `lib/sub` has no buildfile, so this is also a same-package load. +load("//lib/sub:helper.bzl", "HSYM") load(":default.bzl", "DSYM") load(":priv.bzl", "PRIVSYM") load(":vis.bzl", "SYM") -_all = (PRIVSYM, SYM, DSYM) +_all = (PRIVSYM, SYM, DSYM, HSYM) diff --git a/tests/core/interpreter/test_load_visibility_data/lib/sub/helper.bzl b/tests/core/interpreter/test_load_visibility_data/lib/sub/helper.bzl new file mode 100644 index 0000000000000..20f52a1c57ca0 --- /dev/null +++ b/tests/core/interpreter/test_load_visibility_data/lib/sub/helper.bzl @@ -0,0 +1,7 @@ +# Private, and in package `//lib` (there is no buildfile in `lib/sub`), so +# `//lib` may load it, and it may load `//lib`'s private `priv.bzl`. +load("//lib:priv.bzl", "PRIVSYM") + +visibility("private") + +HSYM = PRIVSYM