Override visibility only if current package's visibility is set

Summary: The visibility of a PACKAGE should use the parent's visibility if it does not call package() explicitly.

Reviewed By: stepancheg

Differential Revision: D62501751

fbshipit-source-id: 2b20bda5c772d9b7dadf6486051dcca38cc1f425

Author: Rajneesh Lakkundi

app/buck2_interpreter_for_build/src/super_package/eval_ctx.rs

@@ -83,19 +83,31 @@ impl PackageFileEvalCtx {
         let merged_package_values =
             SuperPackageValuesImpl::merge(self.parent.package_values(), package_values)?;
 
-        let PackageFileVisibilityFields {
-            visibility,
-            within_view,
-            inherit,
-        } = self.visibility.into_inner().unwrap_or_default();
-
-        let (visibility, within_view) = if inherit {
-            (
-                self.parent.visibility().extend_with(&visibility),
-                self.parent.within_view().extend_with(&within_view),
-            )
-        } else {
-            (visibility, within_view)
+        let (visibility, within_view) = match self.visibility.into_inner() {
+            Some(package_visibility) => {
+                if package_visibility.inherit {
+                    (
+                        self.parent
+                            .visibility()
+                            .extend_with(&package_visibility.visibility),
+                        self.parent
+                            .within_view()
+                            .extend_with(&package_visibility.within_view),
+                    )
+                } else {
+                    (
+                        package_visibility.visibility,
+                        package_visibility.within_view,
+                    )
+                }
+            }
+            None => {
+                // If the package file does not specify any visibility, default to the parent visibility.
+                (
+                    self.parent.visibility().to_owned(),
+                    self.parent.within_view().to_owned(),
+                )
+            }
         };
 
         Ok(SuperPackage::new(

tests/e2e/interpreter/test_package_file_visibility.py

@@ -9,17 +9,11 @@
 
 
 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
 
 
 @buck_test(inplace=False)
 async def test_no_package_call_does_not_reset_visibility(buck: Buck) -> None:
     # Test that PACKAGE file without package() call does not reset visibility inherited from parent PACKAGE file.
 
-    # TODO(rajneesh): This test is currently broken. The nested_package:bottom target
-    # should not reset visibility if the PACKAGE file does not call the package() function.
-    await expect_failure(
-        buck.build("root//b:top"),
-        stderr_regex=".*`root//a/nested_package:bottom` is not visible to `root//b:top`.*",
-    )
+    await buck.build("root//b:top")