DaveLiddament / php-language-extensions

Attributes to define new PHP language features (that are enforced by static analysis)
MIT License
131 stars 4 forks source link

Update #[Package] functionality to allow sub namespaces #3

Closed DaveLiddament closed 1 year ago

DaveLiddament commented 2 years ago

Currently #[Package] is modelled on Java's package visibility modifier. This does not allow access to sub namespaces. E.g. in Java a call from namespace Foo\Bar would not be allowed to call a method with package visibility in Foo\Bar\Baz.

Options:

  1. Consider changing the default behaviour of #[Package] to allow calls to sub namespaces.
  2. Add an argument to the #[Package] attribute to allow/disallow sub namespaces (possibly allowing as default).

See this twitter comment.

lcobucci commented 2 years ago

I think the second option makes more sense for PHP... having it enabled as default would be a tiny BC-break, are you okay with that?

lcobucci commented 2 years ago

I think the second option makes more sense for PHP... having it enabled as default would be a tiny BC-break, are you okay with that?

On second thought, I believe we could align more with https://wiki.php.net/rfc/namespace-visibility instead of repurposing the Package attribute.

If we analyse, the access modifiers from Java don't make much sense in PHP, as we only have "top-level elements". Then, the current behaviour of #[Package] is basically the private modifier of that RFC, a package that allows subpackages is essentially the protected modifier, and no modifier is public.

What do you think about something like this:

enum AccessModifier
{
    case Public;
    case Protected;
    case Private;
}

#[Attribute(Attribute::TARGET_CLASS)]
final class NamespaceVisibility
{
    public function __construct(public AccessModifier $access = AccessModifier::Public) {}
}
DaveLiddament commented 2 years ago

Thanks for suggestions and for the reference to the RFC.

I wonder if we should consider the ultimate aim before looking into the details...

My example use cases

From my perspective I often find myself with a 2 or more classes that collaborate with each other. One class might be the "interface" to the wider application. The remaining classes should only be used by each other and not the wider application.

An example from one of my OSS projects is the class BaseLineResultsRemover. BaseLineResultsRemover uses BaseLineResultsComparator. No other classes should use BaseLineResultsComparator.

Ideally I'd put BaseLineResultsComparator in the same namespace (or maybe a sub-namespace) of BaseLineResultsRemover and mark it as #[Protected] (or similar). This would enforce that BaseLineResultsComparator could not be called from the wider project.

In the above example there are only 2 classes collaborating. A more complex example is Parser see. Parser is used by the wider codebase. Parser uses a number of classes that are all in an 'internal' namespace see. All of these classes should not be used elsewhere in the codebase. If #[Package] like functionality existed I could enforce this by static analysis.

Does this make sense?

Next steps

Is this your use case too? Or are you envisaging different needs.

Once we've bottomed out all the uses cases we can work out the simplest implementation.

Let me know your thoughts. Thanks once again for your input and interest.

DaveLiddament commented 2 years ago

How about altering the#[NamespaceVisibility] attribute to this...

 #[Attribute(Attribute::TARGET_CLASS  | Attribute::TARGET_METHOD)]
final class NamespaceVisibility
{
    public function __construct(
        ?string $namespace = null,
        bool $excludeSubNamespaces = false,
    ) {}
}

Default functionality when added to a class.

Adding #[NamespaceVisibility] to a class means the class can only be extended, instantiated and it methods can only be called by code in the same namespace or sub namespace.

E.g.

namespace Fruit {

  #[NamespaceVisibility]
  class Apple {
      public function eat(): void {} 
  }

  // Following all OK as in under the Fruit namespace
  $apple = new Apple(); 
  class GoldenDelicious extends Apple {} 

  function eat(Apple $apple) {
    $apple->eat();
  }
}

namespace Fruit\Foo {
  use Fruit\Apple;

  // Following all OK as in a sub namespace of  Fruit
  $apple = new Apple(); 
  class GoldenDelicious extends Apple {} 

  function eat(Apple $apple) {
    $apple->eat();
  }
}

namespace Computer {

  $apple = new Apple(); // ERROR can not instantiate Apple outside of Fruit namespace
  class GoldenDelicious extends Apple {}  // ERROR can not extend Apple outside of Fruit namespace  

  function eat(Apple $apple) {
    $apple->eat();  // ERROR can not call Apple::eat outside of Fruit namespace
  }
}

Using the $namespace argument

By default #[NamespaceVisibility] will default to the current namespace. Using $namespace argument can override the namespace. (This functionality is the same as @psalm-internal see docs and example usage

Example:

namespace Person {
  class Bob {
    #[NamespaceVisibility(namespace: 'PersonBuilder')]
    public function __construct() {}
  }

  new Bob(); // ERROR: Bob::__construct can only be called from PersonBuilder namespace and sub namespaces
}

namespace PersonBuilder {
  use Person\Bob;

  new Bob(); // OK Bob::__construct called from PersonBuilder namespace
}

namespace PersonBuilder\Foo {
  use Person\Bob;

  new Bob(); // OK Bob::__construct called from a sub namespace of PersonBuilder
}

Using the $excludeSubNamespaces flag

By default sub namespaces are allowed. To allow only the namespace (and not the sub namespaces) set $excludeSubNamespaces to true.

Example:

namespace Fruit {

  #[NamespaceVisibility(excludeSubNamespaces: true)]
  class Apple {
      public function eat(): void {} 
  }

  // Following all OK as in under the Fruit namespace
  $apple = new Apple(); 
  class GoldenDelicious extends Apple {} 

  function eat(Apple $apple) {
    $apple->eat();
  }
}

namespace Fruit\Foo {
  use Fruit\Apple;

  $apple = new Apple();  // ERROR can only call Apple::__construct from the Fruit namespace, not its sub namespace.
  class GoldenDelicious extends Apple {}  // ERROR can only extend Apple int the Fruit namespace, not its sub namespace.

  function eat(Apple $apple) {
    $apple->eat(); // ERROR can only call Apple::eat from the Fruit namespace, not its sub namespace.
  }
}
benr77 commented 1 year ago

I see you have a few options on the go to improve or replace the Package attribute.

What I would like to add is that with PSR4 autoloading, namespaces and sub-namespaces are intrinsically linked together with filesystem organisation.

For example, I have a service class, plus a set of classes that implement a Parser interface - initially these could live in the same namespace as the service and the interface.

However, if the number of implementations grows (imagine we now have 5 parser implementations), I would want to organise these together in a sub-directory, and hence a sub-namespace, but this change does not affect the semantic relationship between the classes.

For this reason, I think it's very restrictive to limit the Package attribute to the exact same namespace. I do think it makes logical sense to allow it to automatically and by default to apply to the current plus all sub-namespaces.

benr77 commented 1 year ago

Just had a look at the NamespaceVisibility attribute and looks really good. This is exactly how I would like to be able to use it. Cheers for your efforts!