@psalm-internal namespace check confuses class name with a namespace

[someniatko]

https://psalm.dev/r/d1e3175a52 - should complain, but doesn't, because the class in the parent namespace is called the same as the child namespace

https://psalm.dev/r/58990a724b - works fine, because the callee class has different name than the namespace.

[psalm-github-bot[bot]]

I found these snippets:

https://psalm.dev/r/d1e3175a52

<?php

namespace Component\Internal
{
    class C
    {
        /** @psalm-internal Component\Internal */
        public function __construct() {
        }
    }
}

namespace Component
{
    use Component\Internal\C;
    
    class Internal
    {
     	public function getC(): C {
            return new C(); // should complain about InternalMethod
        }
    }
}

Psalm output (using commit 7c4228f):

No issues!

https://psalm.dev/r/58990a724b

<?php

namespace Component\Internal
{
    class C
    {
        /** @psalm-internal Component\Internal */
        public function __construct() {
        }
    }
}

namespace Component
{
    use Component\Internal\C;
    
    class OtherName
    {
     	public function getC(): C {
            return new C(); // complains about InternalMethod
        }
    }
}

Psalm output (using commit 7c4228f):

ERROR: InternalMethod - 20:20 - Constructor Component\Internal\C::__construct is internal to Component\Internal but called from Component\OtherName::getC

[AndrolGenhald]

This is a result of #8165. I'm open to ideas on how to disambiguate classes and namespaces, maybe we should have a separate @psalm-internal-to-class?

Edit: When I implemented this I actually did consider this case, but I figured it would be a feature rather than a bug. I would think if you have the namespace Foo\Bar and the class Foo\Bar, they're probably closely related, so if you want something internal to Foo\Bar it should allow both the class Foo\Bar and anything in the namespace Foo\Bar. @orklah thoughts?

[orklah]

I'm not sure how I feel about classes that have the same FQN as a namespace. Is it widely used? Don't we have some kind of PSR rule to forbid that?

[AndrolGenhald]

I'm not sure how I feel about classes that have the same FQN as a namespace. Is it widely used? Don't we have some kind of PSR rule to forbid that?

We use that in Psalm (check out Psalm\Type, Psalm\Type\Atomic, etc) :stuck_out_tongue:

[someniatko]

@orklah why forbidding that? Personally I use it extensively when defining a class which consists of some nested objects, and then nested classes are defined in a namespace with the same name, e.g.

MyProject\Domain\Product is a class, and then I have MyProject\Domain\Product\Id MyProject\Domain\Product\ListPrice etc.

It's convenient to further refer to the nested classes as Product\Id, Product\ListPrice in external code -- and you only have to include a single use statement: use MyProject\Domain\Product;

[someniatko]

Also, following PSR recommendations is just a personal preference. Psalm itself does not follow PSR-12's code style guidelines.

[orklah]

We use that in Psalm

Right, I never made the connection

why forbidding that

For the exact same reason this issue exists. It creates ambiguity and ambiguity leads to bugs :). FQN stands for Fully Qualified Name, I would expect that anything fully qualified should be specific enough not to be confused with another symbol but I guess I'm alone here 😄.

[AndrolGenhald]

Seems totally natural to me :shrug:

The thing I'm not sure about is whether we should call the way @psalm-internal works right now good enough, or try to add a way to disambiguate namespaces from class names.

[someniatko]

It works "good enough" in the same sense static support works "good enough". This is still a bug, namespace != class name.

[AndrolGenhald]

It works "good enough" in the same sense static support works "good enough".

I see those issues as fundamentally different. static has actual bugs that cause false positives in correct code. The issue here is with what we intend the meaning of @psalm-internal to be. Saying that @psalm-internal works by matching an identifier to a FQCN to see if it matches or is a parent namespace seems completely valid to me, it just doesn't do what you want it to.

We may end up changing it, but I can't think of a great way to do it so far. Either we add a separate tag, in which case I think we might want a separate tag for methods as well so it's consistent, or we find a way to disambiguate classes and namespaces with the current tag, which I'm not sure how to do.

[someniatko]

I'm following the official Psalm documentation, which should be the source of truth:

Used to mark a class, property or function as internal to a given namespace

(...) for @psalm-internal, the docbloc line must specify a namespace. An issue is raised if the calling code is not within the given namespace.

Here the calling code is not within the namespace Component\Internal but in the Component namespace.

You could update the documentation though, if you wish to reinterpet the behavior.

https://psalm.dev/docs/annotating_code/supported_annotations/#psalm-internal

[AndrolGenhald]

You could update the documentation though, if you wish to reinterpet the behavior.

Yeah, that should have been done in #8165, sorry I missed that. I think it's better to wait now though until we figure out if we want to do anything about this issue.

[someniatko]

I find the new behavior more confusing though, than the currently documented one. At least it's harder to explain it.

[weirdan]

or we find a way to disambiguate classes and namespaces with the current tag, which I'm not sure how to do.

One option could be postfixes. E.g. Some\NS\ (note the trailing slash) refers to a namespace, Some\NS() - to a function and Some\NS{} - to a class. Methods do not need postfixes as they already can be distinguished by ::.

[someniatko]

One option could be postfixes. E.g. Some\NS\ (note the trailing slash) refers to a namespace, Some\NS() - to a function and Some\NS{} - to a class. Methods do not need postfixes as they already can be distinguished by ::.

Yeah, that's something. I'd also prefer something along the lines of the originally suggested @psalm-internal-to-class