Support list of private constants (#19)

* restore tests from #5

* update validator to support private_constants setting

* add private_constants property

With this change, the `enforce_privacy` setting will support only four
values: true, false, 'strict', and nil. The array of private constant
names can now only be specified via a new setting
called `private_constants`. If `enforce_privacy` is set to false or
nil, these private constants are ignored by the checker. If
`private_constants` is not specified or is an empty list, all
constants in the package are assumed to be private unless
they are defined in the public directory.

* bump minor version

Author: mclark

Gemfile.lock

@@ -17,7 +17,7 @@ GIT
 PATH
   remote: .
   specs:
-    packwerk-extensions (0.0.11)
+    packwerk-extensions (0.1.0)
       packwerk (>= 2.2.1)
       railties (>= 6.0.0)
       sorbet-runtime

README.md

@@ -56,6 +56,9 @@ Example:
 public_path: my/custom/path/
 ```
 
+### Using specific private constants
+Sometimes it is desirable to only enforce privacy on a subset of constants in a package. You can do so by defining a `private_constants` list in your package.yml. Note that `enforce_privacy` must be set to `true` or `'strict'` for this to work.
+
 ### Package Privacy violation
 A constant that is private to its package has been referenced from outside of the package. Constants are declared private in their package’s `package.yml`.
 

lib/packwerk/privacy/checker.rb

@@ -30,7 +30,9 @@ def invalid_reference?(reference)
         return false if privacy_package.public_path?(reference.constant.location)
 
         privacy_option = privacy_package.enforce_privacy
-        !enforcement_disabled?(privacy_option)
+        return false if enforcement_disabled?(privacy_option)
+
+        explicitly_private_constant?(reference.constant, explicitly_private_constants: privacy_package.private_constants)
       end
 
       sig do
@@ -63,6 +65,20 @@ def message(reference)
 
       private
 
+      sig do
+        params(
+          constant: ConstantContext,
+          explicitly_private_constants: T::Array[String]
+        ).returns(T::Boolean)
+      end
+      def explicitly_private_constant?(constant, explicitly_private_constants:)
+        return true if explicitly_private_constants.empty?
+
+        explicitly_private_constants.include?(constant.name) ||
+          # nested constants
+          explicitly_private_constants.any? { |epc| constant.name.start_with?("#{epc}::") }
+      end
+
       sig do
         params(privacy_option: T.nilable(T.any(T::Boolean, String, T::Array[String])))
           .returns(T::Boolean)

lib/packwerk/privacy/package.rb

@@ -8,7 +8,8 @@ class Package < T::Struct
 
       const :public_path, String
       const :user_defined_public_path, T.nilable(String)
-      const :enforce_privacy, T.nilable(T.any(T::Boolean, String, T::Array[String]))
+      const :enforce_privacy, T.nilable(T.any(T::Boolean, String))
+      const :private_constants, T::Array[String]
 
       sig { params(path: String).returns(T::Boolean) }
       def public_path?(path)
@@ -23,7 +24,8 @@ def from(package)
           Package.new(
             public_path: public_path_for(package),
             user_defined_public_path: user_defined_public_path(package),
-            enforce_privacy: package.config['enforce_privacy']
+            enforce_privacy: package.config['enforce_privacy'],
+            private_constants: package.config['private_constants'] || []
           )
         end
 

lib/packwerk/privacy/validator.rb

@@ -19,6 +19,8 @@ def call(package_set, configuration)
           results << check_enforce_privacy_setting(config_file_path, setting)
         end
 
+        results += verify_private_constants_setting(package_set, configuration)
+
         public_path_settings = package_manifests_settings_for(configuration, 'public_path')
         public_path_settings.each do |config_file_path, setting|
           results << check_public_path(config_file_path, setting)
@@ -29,11 +31,49 @@ def call(package_set, configuration)
 
       sig { override.returns(T::Array[String]) }
       def permitted_keys
-        %w[public_path enforce_privacy]
+        %w[public_path enforce_privacy private_constants]
       end
 
       private
 
+      sig { params(package_set: PackageSet, configuration: Configuration).returns(T::Array[Result]) }
+      def verify_private_constants_setting(package_set, configuration)
+        private_constants_setting = package_manifests_settings_for(configuration, 'private_constants')
+        results = T.let([], T::Array[Result])
+        resolver = ConstantResolver.new(
+          root_path: configuration.root_path,
+          load_paths: configuration.load_paths
+        )
+
+        private_constants_setting.each do |config_file_path, setting|
+          next if setting.nil?
+
+          unless setting.is_a?(Array)
+            results << Result.new(
+              ok: false,
+              error_value: "Invalid 'private_constants' setting: #{setting.inspect}"
+            )
+            next
+          end
+
+          constants = setting
+
+          results += assert_constants_can_be_loaded(constants, config_file_path)
+
+          constant_locations = constants.map { |c| [c, resolver.resolve(c)&.location] }
+
+          constant_locations.each do |name, location|
+            results << if location
+                         check_private_constant_location(configuration, package_set, name, location, config_file_path)
+                       else
+                         private_constant_unresolvable(name, config_file_path)
+                       end
+          end
+        end
+
+        results
+      end
+
       sig do
         params(config_file_path: String, setting: T.untyped).returns(Result)
       end
@@ -52,7 +92,7 @@ def check_public_path(config_file_path, setting)
         params(config_file_path: String, setting: T.untyped).returns(Result)
       end
       def check_enforce_privacy_setting(config_file_path, setting)
-        if [TrueClass, FalseClass, Array, NilClass].include?(setting.class) || setting == 'strict'
+        if [TrueClass, FalseClass, NilClass].include?(setting.class) || setting == 'strict'
           Result.new(ok: true)
         else
           Result.new(
@@ -61,6 +101,55 @@ def check_enforce_privacy_setting(config_file_path, setting)
           )
         end
       end
+
+      sig do
+        params(configuration: Configuration, package_set: PackageSet, name: T.untyped, location: T.untyped,
+               config_file_path: T.untyped).returns(Result)
+      end
+      def check_private_constant_location(configuration, package_set, name, location, config_file_path)
+        declared_package = package_set.package_from_path(relative_path(configuration, config_file_path))
+        constant_package = package_set.package_from_path(location)
+
+        if constant_package == declared_package
+          Result.new(ok: true)
+        else
+          Result.new(
+            ok: false,
+            error_value: "'#{name}' is declared as private in the '#{declared_package}' package but appears to be " \
+                         "defined\nin the '#{constant_package}' package. Packwerk resolved it to #{location}."
+          )
+        end
+      end
+
+      sig { params(constants: T.untyped, config_file_path: String).returns(T::Array[Result]) }
+      def assert_constants_can_be_loaded(constants, config_file_path)
+        constants.map do |constant|
+          if constant.start_with?('::')
+            constant.try(&:constantize) && Result.new(ok: true)
+          else
+            error_value = "'#{constant}', listed in the 'private_constants' option " \
+                          "in #{config_file_path}, is invalid.\nPrivate constants need to be " \
+                          'prefixed with the top-level namespace operator `::`.'
+            Result.new(
+              ok: false,
+              error_value: error_value
+            )
+          end
+        end
+      end
+
+      sig { params(name: T.untyped, config_file_path: T.untyped).returns(Result) }
+      def private_constant_unresolvable(name, config_file_path)
+        explicit_filepath = "#{(name.start_with?('::') ? name[2..] : name).underscore}.rb"
+
+        Result.new(
+          ok: false,
+          error_value: "'#{name}', listed in #{config_file_path}, could not be resolved.\n" \
+                       "This is probably because it is an autovivified namespace - a namespace module that doesn't have a\n" \
+                       "file explicitly defining it. Packwerk currently doesn't support declaring autovivified namespaces as\n" \
+                       "private. Add a #{explicit_filepath} file to explicitly define the constant."
+        )
+      end
     end
   end
 end

packwerk-extensions.gemspec

@@ -1,6 +1,6 @@
 Gem::Specification.new do |spec|
   spec.name          = 'packwerk-extensions'
-  spec.version       = '0.0.11'
+  spec.version       = '0.1.0'
   spec.authors       = ['Gusto Engineers']
   spec.email         = ['[email protected]']
 

test/fixtures/skeleton/components/timeline/package.yml

@@ -1,2 +1,3 @@
-enforce_privacy:
+enforce_privacy: true
+private_constants:
   - "::PrivateThing"

test/unit/privacy/checker_test.rb

@@ -19,8 +19,23 @@ class CheckerTest < Minitest::Test
       end
 
       test 'ignores if destination package is not enforcing' do
+        destination_package = Packwerk::Package.new(
+          name: 'destination_package',
+          config: { 'enforce_privacy' => false, 'private_constants' => ['::SomeName'] }
+        )
         checker = privacy_checker
-        reference = build_reference
+        reference = build_reference(destination_package: destination_package)
+
+        refute checker.invalid_reference?(reference)
+      end
+
+      test 'ignores if destination package is only enforcing for other constants' do
+        destination_package = Packwerk::Package.new(
+          name: 'destination_package',
+          config: { 'enforce_privacy' => true, 'private_constants' => ['::SomeOtherConstant'] }
+        )
+        checker = privacy_checker
+        reference = build_reference(destination_package: destination_package)
 
         refute checker.invalid_reference?(reference)
       end
@@ -34,21 +49,29 @@ class CheckerTest < Minitest::Test
       end
 
       test 'complains about private constant if enforcing for specific constants' do
-        destination_package = Packwerk::Package.new(name: 'destination_package', config: { 'enforce_privacy' => ['::SomeName'] })
+        destination_package = Packwerk::Package.new(name: 'destination_package', config: { 'enforce_privacy' => true, 'private_constants' => ['::SomeName'] })
         checker = privacy_checker
         reference = build_reference(destination_package: destination_package)
 
         assert checker.invalid_reference?(reference)
       end
 
       test 'complains about nested constant if enforcing for specific constants' do
-        destination_package = Packwerk::Package.new(name: 'destination_package', config: { 'enforce_privacy' => ['::SomeName'] })
+        destination_package = Packwerk::Package.new(name: 'destination_package', config: { 'enforce_privacy' => true, 'private_constants' => ['::SomeName'] })
         checker = privacy_checker
-        reference = build_reference(destination_package: destination_package)
+        reference = build_reference(destination_package: destination_package, constant_name: '::SomeName::Nested')
 
         assert checker.invalid_reference?(reference)
       end
 
+      test 'ignores constant that starts like enforced constant' do
+        destination_package = Packwerk::Package.new(name: 'destination_package', config: { 'enforce_privacy' => true, 'private_constants' => ['::SomeName'] })
+        checker = privacy_checker
+        reference = build_reference(destination_package: destination_package, constant_name: '::SomeNameButNotQuite')
+
+        refute checker.invalid_reference?(reference)
+      end
+
       test 'ignores public constant even if enforcing privacy for everything' do
         destination_package = Packwerk::Package.new(name: 'destination_package', config: { 'enforce_privacy' => true })
         checker = privacy_checker
@@ -58,7 +81,7 @@ class CheckerTest < Minitest::Test
       end
 
       test 'only checks the package TODO file for private constants' do
-        destination_package = Packwerk::Package.new(name: 'destination_package', config: { 'enforce_privacy' => ['::SomeName'] })
+        destination_package = Packwerk::Package.new(name: 'destination_package', config: { 'enforce_privacy' => true, 'private_constants' => ['::SomeName'] })
         checker = privacy_checker
         reference = build_reference(destination_package: destination_package)
 

test/unit/privacy/package_test.rb

@@ -12,7 +12,7 @@ class PackageTest < Minitest::Test
 
       setup do
         setup_application_fixture
-        @package = Packwerk::Package.new(name: 'components/timeline', config: { 'enforce_privacy' => ['::Test'] })
+        @package = Packwerk::Package.new(name: 'components/timeline', config: { 'enforce_privacy' => true, 'private_constants' => ['::Test'] })
       end
 
       teardown do
@@ -25,7 +25,7 @@ def privacy_package
       end
 
       test '#enforce_privacy returns same value as from config' do
-        assert_equal(['::Test'], privacy_package.enforce_privacy)
+        assert_equal(true, privacy_package.enforce_privacy)
       end
 
       test '#public_path returns expected path when using the default public path' do

test/unit/privacy/validator_test.rb

@@ -49,6 +49,60 @@ class ValidatorTest < Minitest::Test
       assert_match(/'public_path' option must be a string/, result.error_value)
     end
 
+    test 'check_package_manifests_for_privacy returns an error for unresolvable privatized constants' do
+      use_template(:skeleton)
+      ConstantResolver.expects(:new).returns(stub('resolver', resolve: nil))
+
+      result = Packwerk::Privacy::Validator.new.call(package_set, config)
+      refute result.ok?, result.error_value
+      assert_match(
+        /'::PrivateThing', listed in #{to_app_path('components\/timeline\/package.yml')}, could not be resolved/,
+        result.error_value
+      )
+      assert_match(
+        /Add a private_thing.rb file/,
+        result.error_value
+      )
+    end
+
+    test 'check_package_manifests_for_privacy returns an error for privatized constants in other packages' do
+      use_template(:skeleton)
+      context = ConstantResolver::ConstantContext.new('::PrivateThing', 'private_thing.rb')
+
+      ConstantResolver.expects(:new).returns(stub('resolver', resolve: context))
+
+      result = Packwerk::Privacy::Validator.new.call(package_set, config)
+
+      refute result.ok?, result.error_value
+      assert_match(
+        %r{'::PrivateThing' is declared as private in the 'components/timeline' package},
+        result.error_value
+      )
+      assert_match(
+        /but appears to be defined\sin the '.' package/,
+        result.error_value
+      )
+    end
+
+    test 'check_package_manifests_for_privacy returns an error for constants without `::` prefix' do
+      use_template(:minimal)
+      merge_into_app_yaml_file('package.yml', { 'private_constants' => ['::PrivateThing', 'OtherThing'] })
+
+      result = Packwerk::Privacy::Validator.new.call(package_set, config)
+
+      refute result.ok?, result.error_value
+      assert_match(
+        /'OtherThing', listed in the 'private_constants' option in .*package.yml, is invalid./,
+        result.error_value
+      )
+      assert_match(
+        /Private constants need to be prefixed with the top-level namespace operator `::`/,
+        result.error_value
+      )
+    end
+
+    private
+
     sig { returns(Packwerk::ApplicationValidator) }
     def validator
       @validator ||= Packwerk::ApplicationValidator.new