[Discussion] "Internal" attribute and warning

11 months ago by Saki Takamachi — view source

unread

Hi Robert,

Hello internals,

I've been thinking about having an "internal" attribute that will emit
a warning if called from outside it's left-most namespace.

It might look something like this:

namespace MyCompany\PackageA {
#[\Internal] function doStuff() {}
}

namespace OtherCompany\PackageB {
\MyCompany\PackageA\doStuff(); // warning emitted
}

namespace MyCompany\PackageB {
\MyCompany\PackageB\doStuff(); // left-most part of namespace
matches, no warning
}

This would allow for library maintainers to mark internal constructs
as such and provide users with feedback that they are using code that
may be changed without any notice.

Any thoughts?

Robert Landers
Software Engineer
Utrecht NL

It seems to be a similar concept to package-private in Java, is my understanding correct?

Regards,

Saki

11 months ago by Rowan Tommins [IMSoP] — view source

unread

I've been thinking about having an "internal" attribute that will emit
a warning if called from outside it's left-most namespace.

I like the general idea, but I don't think limiting to "left-most
namespace" is the best semantics.

It's very common for the top-level namespace to represent a vendor, and
the second level to be the specific package, e.g. Doctrine\DBAL vs
Doctrine\ORM. You've even used that in your example - I presume you've
made a typo, and meant both examples to be calling PackageA not
PackageB. In other cases, there are more levels - e.g. Composer package
"doctrine/mongodb-odm" has root namespace "Doctrine\ODM\MongoDB".

Possibly the attribute would need some argument to specify its
granularity, e.g. #[Internal('\MyCompany\PackageA')],
#[Internal('\Doctrine\ODM\MongoDB')], but that would be annoying to
write each time.

This is another case where PHP suffers from its lack of a separate
concept of "package" or "module" to scope things to.

My second concern is how to implement this efficiently. The check can't
happen at compile-time, because we don't know the definition of
SomeOtherNamespace\Foo; so the check would need to be at run-time when
the method/function is called. But at run-time, namespaces have very
little existence - they really are just part of the names of functions,
classes, and constants.

So when calling a marked function, we would have to look up the name of
the calling function or the class name of the calling method, and then
do a string comparison against the namespace constraint. Maybe that
would be easy and fast, I don't know.

Regards,

--
Rowan Tommins
[IMSoP]

11 months ago by Robert Landers — view source

unread

On Sat, May 18, 2024 at 5:18 PM Rowan Tommins [IMSoP]
imsop.php@rwec.co.uk wrote:

I've been thinking about having an "internal" attribute that will emit
a warning if called from outside it's left-most namespace.

I like the general idea, but I don't think limiting to "left-most namespace" is the best semantics.

It's very common for the top-level namespace to represent a vendor, and the second level to be the specific package, e.g. Doctrine\DBAL vs Doctrine\ORM. You've even used that in your example - I presume you've made a typo, and meant both examples to be calling PackageA not PackageB. In other cases, there are more levels - e.g. Composer package "doctrine/mongodb-odm" has root namespace "Doctrine\ODM\MongoDB".

I thought about that too, but in-general, a vendor has the knowledge
and capability to ensure any two packages work together (like Doctrine
plugins in your example).

Possibly the attribute would need some argument to specify its granularity, e.g. #[Internal('\MyCompany\PackageA')], #[Internal('\Doctrine\ODM\MongoDB')], but that would be annoying to write each time.

That could be a useful optional parameter, and might be worth considering.

This is another case where PHP suffers from its lack of a separate concept of "package" or "module" to scope things to.

My second concern is how to implement this efficiently. The check can't happen at compile-time, because we don't know the definition of SomeOtherNamespace\Foo; so the check would need to be at run-time when the method/function is called. But at run-time, namespaces have very little existence - they really are just part of the names of functions, classes, and constants.

So when calling a marked function, we would have to look up the name of the calling function or the class name of the calling method, and then do a string comparison against the namespace constraint. Maybe that would be easy and fast, I don't know.

This is a concern of mine as well, but I'm mostly curious if anyone
even wants this before I worry about implementation details. Right
now, I don't see it being any slower than type-checking, though I
foresee a bit more memory usage to keep track of the caller/callee
namespace.

Regards,

--
Rowan Tommins
[IMSoP]

11 months ago by Rowan Tommins [IMSoP] — view source

unread

I thought about that too, but in-general, a vendor has the knowledge
and capability to ensure any two packages work together (like Doctrine
plugins in your example).

How do they achieve that "knowledge and capability" other than documentation, and tooling making use of that documentation?

Doctrine DBAL and Doctrine ORM are both large open-source codebases, which happen to have a dependency relationship, and also happen to have the same vendor namespace. Documentation and warnings about using internal functions/classes of the DBAL would be just as useful to a developer of the ORM as they would be to an application developer.

As another example, within the completely private codebase I work on professionally, we have shared modules, parts of which are intended to be implementation details and not subject to compatibility guarantees. It would be really useful to get an automatic notification if those were used in other parts of our codebase, but all of our code shares the same vendor namespace, so a single-level #[Internal] attribute would be entirely useless.

Rowan Tommins
[IMSoP]

11 months ago by Andreas Heigl — view source

unread

Hey all.

Am 18.05.24 um 16:00 schrieb Robert Landers:

Hello internals,

I've been thinking about having an "internal" attribute that will emit
a warning if called from outside it's left-most namespace.

It might look something like this:

namespace MyCompany\PackageA {
#[\Internal] function doStuff() {}
}

namespace OtherCompany\PackageB {
\MyCompany\PackageA\doStuff(); // warning emitted
}

namespace MyCompany\PackageB {
\MyCompany\PackageB\doStuff(); // left-most part of namespace
matches, no warning
}

This would allow for library maintainers to mark internal constructs
as such and provide users with feedback that they are using code that
may be changed without any notice.

Any thoughts?
I do like the idea in general of being able to mark certain things as
internal to a certain namespace.

My question is more in the direction of: If we are not enforcing
breaking the code but merely emit a warning then that is IMO not
something that we need to implement on the language level. People will
still be able to use the internal method and just ignore the warning.

So when we not break the flow this is something that static analysers
can do as well. They will immediately spot when an internal function is
used outside the expected namespace and can then raise the appropriate
flags so that the problem can be fixed.

That then also raises the question for me whether the attribute actually
needs to be provided by the language itself or whether that is something
that can be added by the static analyzers or - if we want one attribute
for all namespaces - perhaps even by the FIG. THough that would probably
be a case where the FIG would not provide an interface but an actual
class implementation.

Regarding the attribute itself I would declare the namespace it is
supposed to be internal to in the attribute itself.

So something like

namespace MyCompany\PackageA\SubpackageA {
#[Internal(MyCompany\PackageA)] function doStuff(){}
}

might be used to define a function that can be used in all code of
PackageA while still being declared in the Subpackage.

I would like that more than relative "paths" like

namespace MyCompany\PackageA\SubpackageA {
#[Internal(..)] function doStuff(){}
}

or something like that.

Yes, this opens the possibility to even declare functions internal to
packages outside the actual namespace and we might need something to
make sure to disallow that so that the namespace the part is internal to
can only be part of the namespace it is defined in.

My 0.02 €

Cheers

Andreas

Robert Landers
Software Engineer
Utrecht NL

--
,,,
(o o)
+---------------------------------------------------------ooO-(_)-Ooo-+
| Andreas Heigl |
| mailto:andreas@heigl.org N 50°22'59.5" E 08°23'58" |
| https://andreas.heigl.org |
+---------------------------------------------------------------------+
| https://hei.gl/appointmentwithandreas |
+---------------------------------------------------------------------+
| GPG-Key: https://hei.gl/keyandreasheiglorg |
+---------------------------------------------------------------------+

11 months ago by Larry Garfield — view source

unread

Hey all.

Am 18.05.24 um 16:00 schrieb Robert Landers:

Hello internals,

I've been thinking about having an "internal" attribute that will emit
a warning if called from outside it's left-most namespace.

It might look something like this:

namespace MyCompany\PackageA {
#[\Internal] function doStuff() {}
}

snip

I think an important question to answer here is what we want to have as our definition of "package".

Should the package be defined by the namespace? If so, anyone can put code into any namespace; it's not even hard to add to someone else's namespace.
Should the package be implicit, or explicit? If it's a namespace, is it auto-implicit or an "empty" package?
Should package be defined/implied by the file system, like many languages? Then we'd need a way to define/declare a package on the file system. (I have some thoughts there.) But that may run into performance issues, though they could be solved by opcache. This could also make it harder to inject code into someone else's namespace. (Whether that's good or bad is a matter of opinion.)

The proposed attribute would be going with point 2, implicitly. That may be a useful approach, I don't know, but it's not something we should do "implicitly", lest it cause issues for us in the future.

--Larry Garfield

11 months ago by Robert Landers — view source

unread

Hey all.

Am 18.05.24 um 16:00 schrieb Robert Landers:

Hello internals,

I've been thinking about having an "internal" attribute that will emit
a warning if called from outside it's left-most namespace.

It might look something like this:

namespace MyCompany\PackageA {
#[\Internal] function doStuff() {}
}

snip

I think an important question to answer here is what we want to have as our definition of "package".

Should the package be defined by the namespace? If so, anyone can put code into any namespace; it's not even hard to add to someone else's namespace.

Should the package be implicit, or explicit? If it's a namespace, is it auto-implicit or an "empty" package?

Should package be defined/implied by the file system, like many languages? Then we'd need a way to define/declare a package on the file system. (I have some thoughts there.) But that may run into performance issues, though they could be solved by opcache. This could also make it harder to inject code into someone else's namespace. (Whether that's good or bad is a matter of opinion.)

The proposed attribute would be going with point 2, implicitly. That may be a useful approach, I don't know, but it's not something we should do "implicitly", lest it cause issues for us in the future.

--Larry Garfield

Oof, I wasn't trying to open the "what is a package" can of worms,
though that may need to happen sooner-or-later. I was mainly trying to
be able to mark malleable APIs and set a conservative delimiter
(top-level/root namespace) for the warning. I think being able to
specify a more local delimiter (working on teams in large codebases)
might also be useful.

I don't really see an issue with injecting code into other namespaces,
though. That's basically the only way to properly implement proxies,
afterall.

Robert Landers
Software Engineer
Utrecht NL

PS pro-tip: use reply-to-all to email the original author of the email
as well. I saw this email almost two hours ago on another email
address I'm subscribed with but couldn't reply until now. If you also
email the original author, we can communicate faster (for whatever
that is worth) while the list plays catch-up.

11 months ago by Larry Garfield — view source

unread

I think an important question to answer here is what we want to have as our definition of "package".

Should the package be defined by the namespace? If so, anyone can put code into any namespace; it's not even hard to add to someone else's namespace.

Should the package be implicit, or explicit? If it's a namespace, is it auto-implicit or an "empty" package?

Should package be defined/implied by the file system, like many languages? Then we'd need a way to define/declare a package on the file system. (I have some thoughts there.) But that may run into performance issues, though they could be solved by opcache. This could also make it harder to inject code into someone else's namespace. (Whether that's good or bad is a matter of opinion.)

The proposed attribute would be going with point 2, implicitly. That may be a useful approach, I don't know, but it's not something we should do "implicitly", lest it cause issues for us in the future.

--Larry Garfield

Oof, I wasn't trying to open the "what is a package" can of worms,
though that may need to happen sooner-or-later. I was mainly trying to
be able to mark malleable APIs and set a conservative delimiter
(top-level/root namespace) for the warning. I think being able to
specify a more local delimiter (working on teams in large codebases)
might also be useful.

Understandable, but "easy simple solutions" have a tendency to get in the way of more complete solutions later, or at least cause confusion. So considering the broader scope is important. Like, what happens in the future if we have both package visibility (whatever that means) and an #[Internal] marker attribute (with whatever details)? What happens if the attribute's namespace doesn't line up with a package? Which one should we use when, or at that point do we call #[Internal] soft-deprecated?

I don't have answers to those, but we would want answers before we added the attribute.

PS pro-tip: use reply-to-all to email the original author of the email
as well. I saw this email almost two hours ago on another email
address I'm subscribed with but couldn't reply until now. If you also
email the original author, we can communicate faster (for whatever
that is worth) while the list plays catch-up.

PS: I absolutely despise it when people use reply-all, and I get two copies of the email in 2 different folders that I have to clean up manually, and if I reply to the wrong one it breaks threading because I'm then replying to a non-list email but sending it to the list. This is the only list I'm on that has always had that problem. If people need to communicate faster than email speeds, that's what the dozen available chat media are for.

--Larry Garfield

11 months ago by Robert Landers — view source

unread

I think an important question to answer here is what we want to have as our definition of "package".

Should the package be defined by the namespace? If so, anyone can put code into any namespace; it's not even hard to add to someone else's namespace.

Should the package be implicit, or explicit? If it's a namespace, is it auto-implicit or an "empty" package?

Should package be defined/implied by the file system, like many languages? Then we'd need a way to define/declare a package on the file system. (I have some thoughts there.) But that may run into performance issues, though they could be solved by opcache. This could also make it harder to inject code into someone else's namespace. (Whether that's good or bad is a matter of opinion.)

The proposed attribute would be going with point 2, implicitly. That may be a useful approach, I don't know, but it's not something we should do "implicitly", lest it cause issues for us in the future.

--Larry Garfield

Oof, I wasn't trying to open the "what is a package" can of worms,
though that may need to happen sooner-or-later. I was mainly trying to
be able to mark malleable APIs and set a conservative delimiter
(top-level/root namespace) for the warning. I think being able to
specify a more local delimiter (working on teams in large codebases)
might also be useful.

Hey Larry,

Understandable, but "easy simple solutions" have a tendency to get in the way of more complete solutions later, or at least cause confusion. So considering the broader scope is important. Like, what happens in the future if we have both package visibility (whatever that means) and an #[Internal] marker attribute (with whatever details)? What happens if the attribute's namespace doesn't line up with a package? Which one should we use when, or at that point do we call #[Internal] soft-deprecated?

I think this is something of a moot point. PHP has been around for
30ish years or so. If nobody has championed packages by now ... I
don't think holding back useful features until someone does is a
worthwhile exercise. If we don't like it, we have several options:

I'm not necessarily going to work on this now ... and it probably
wouldn't happen until this fall (well after the 8.4 cut off). So,
there's plenty of time to convince me this is a terrible idea.
vote "no" before it ever gets past the RFC stage
create a competing RFC
create an RFC to deprecate/change the functionality afterwards
don't use it

Anyway, there are currently only namespaces for organization in PHP.
Any package system would almost certainly use them, so I'm not too
worried about it.

I don't have answers to those, but we would want answers before we added the attribute.

PS pro-tip: use reply-to-all to email the original author of the email
as well. I saw this email almost two hours ago on another email
address I'm subscribed with but couldn't reply until now. If you also
email the original author, we can communicate faster (for whatever
that is worth) while the list plays catch-up.

PS: I absolutely despise it when people use reply-all, and I get two copies of the email in 2 different folders that I have to clean up manually, and if I reply to the wrong one it breaks threading because I'm then replying to a non-list email but sending it to the list. This is the only list I'm on that has always had that problem. If people need to communicate faster than email speeds, that's what the dozen available chat media are for.

--Larry Garfield

Robert Landers
Software Engineer
Utrecht NL

11 months ago by Robert Landers — view source

unread

Hey all.

Am 18.05.24 um 16:00 schrieb Robert Landers:

Hello internals,

I've been thinking about having an "internal" attribute that will emit
a warning if called from outside it's left-most namespace.

It might look something like this:

namespace MyCompany\PackageA {
#[\Internal] function doStuff() {}
}

namespace OtherCompany\PackageB {
\MyCompany\PackageA\doStuff(); // warning emitted
}

namespace MyCompany\PackageB {
\MyCompany\PackageB\doStuff(); // left-most part of namespace
matches, no warning
}

This would allow for library maintainers to mark internal constructs
as such and provide users with feedback that they are using code that
may be changed without any notice.

Any thoughts?
I do like the idea in general of being able to mark certain things as
internal to a certain namespace.

My question is more in the direction of: If we are not enforcing
breaking the code but merely emit a warning then that is IMO not
something that we need to implement on the language level. People will
still be able to use the internal method and just ignore the warning.

I guess that depends on what you mean by "break the code." Many people
turn warnings into exceptions, and for those people this will
effectively break their code. Some people may choose to ignore it one
way or another, but that would be up to them. The idea isn't to break
people's code though, it's to provide a way to mark very-malleable
APIs in libraries. Maybe, "Internal" is a bad name for this, but it's
the most logical.

                                                           ,,,
                                                          (o o)
+---------------------------------------------------------ooO-(_)-Ooo-+
| Andreas Heigl |
| mailto:andreas@heigl.org N 50°22'59.5" E 08°23'58" |
| https://andreas.heigl.org |
+---------------------------------------------------------------------+
| https://hei.gl/appointmentwithandreas |
+---------------------------------------------------------------------+
| GPG-Key: https://hei.gl/keyandreasheiglorg |
+---------------------------------------------------------------------+

Robert Landers
Software Engineer
Utrecht NL

11 months ago by Andreas Heigl — view source

unread

Am 18.05.24 um 19:46 schrieb Robert Landers:

Hey all.

Am 18.05.24 um 16:00 schrieb Robert Landers:

Hello internals,

I've been thinking about having an "internal" attribute that will emit
a warning if called from outside it's left-most namespace.

It might look something like this:

namespace MyCompany\PackageA {
#[\Internal] function doStuff() {}
}

namespace OtherCompany\PackageB {
\MyCompany\PackageA\doStuff(); // warning emitted
}

namespace MyCompany\PackageB {
\MyCompany\PackageB\doStuff(); // left-most part of namespace
matches, no warning
}

This would allow for library maintainers to mark internal constructs
as such and provide users with feedback that they are using code that
may be changed without any notice.

Any thoughts?
I do like the idea in general of being able to mark certain things as
internal to a certain namespace.

My question is more in the direction of: If we are not enforcing
breaking the code but merely emit a warning then that is IMO not
something that we need to implement on the language level. People will
still be able to use the internal method and just ignore the warning.

I guess that depends on what you mean by "break the code." Many people
turn warnings into exceptions, and for those people this will
effectively break their code. Some people may choose to ignore it one
way or another, but that would be up to them. The idea isn't to break
people's code though, it's to provide a way to mark very-malleable
APIs in libraries. Maybe, "Internal" is a bad name for this, but it's
the most logical.

The trouble I see is that there are two kind of people: Those that care
about such things and those that don't.

Those that care about such things will already find these issues during
static analysis.

Those that don't care will not turn errors into exceptions but instead
have error-reporting set to 0

That's why I am hesitant in adding this to the engine but instead would
love to see this as part of userland maintained static analyzers.

Cheers

Andreas

--
,,,
(o o)
+---------------------------------------------------------ooO-(_)-Ooo-+
| Andreas Heigl |
| mailto:andreas@heigl.org N 50°22'59.5" E 08°23'58" |
| https://andreas.heigl.org |
+---------------------------------------------------------------------+
| https://hei.gl/appointmentwithandreas |
+---------------------------------------------------------------------+
| GPG-Key: https://hei.gl/keyandreasheiglorg |
+---------------------------------------------------------------------+

11 months ago by Robert Landers — view source

unread

Am 18.05.24 um 19:46 schrieb Robert Landers:

Hey all.

Am 18.05.24 um 16:00 schrieb Robert Landers:

Hello internals,

I've been thinking about having an "internal" attribute that will emit
a warning if called from outside it's left-most namespace.

It might look something like this:

namespace MyCompany\PackageA {
#[\Internal] function doStuff() {}
}

namespace OtherCompany\PackageB {
\MyCompany\PackageA\doStuff(); // warning emitted
}

namespace MyCompany\PackageB {
\MyCompany\PackageB\doStuff(); // left-most part of namespace
matches, no warning
}

This would allow for library maintainers to mark internal constructs
as such and provide users with feedback that they are using code that
may be changed without any notice.

Any thoughts?
I do like the idea in general of being able to mark certain things as
internal to a certain namespace.

My question is more in the direction of: If we are not enforcing
breaking the code but merely emit a warning then that is IMO not
something that we need to implement on the language level. People will
still be able to use the internal method and just ignore the warning.

I guess that depends on what you mean by "break the code." Many people
turn warnings into exceptions, and for those people this will
effectively break their code. Some people may choose to ignore it one
way or another, but that would be up to them. The idea isn't to break
people's code though, it's to provide a way to mark very-malleable
APIs in libraries. Maybe, "Internal" is a bad name for this, but it's
the most logical.

The trouble I see is that there are two kind of people: Those that care
about such things and those that don't.

Those that care about such things will already find these issues during
static analysis.

Those that don't care will not turn errors into exceptions but instead
have error-reporting set to 0

That's why I am hesitant in adding this to the engine but instead would
love to see this as part of userland maintained static analyzers.

Cheers

Andreas

--
,,,
(o o)
+---------------------------------------------------------ooO-(_)-Ooo-+
| Andreas Heigl |
| mailto:andreas@heigl.org N 50°22'59.5" E 08°23'58" |
| https://andreas.heigl.org |
+---------------------------------------------------------------------+
| https://hei.gl/appointmentwithandreas |
+---------------------------------------------------------------------+
| GPG-Key: https://hei.gl/keyandreasheiglorg |
+---------------------------------------------------------------------+

Hey Andreas,

I don't think this is a useful way to look at it. Otherwise, what is
the point of making any error or warning? Static analysis is useful,
but there's a lot of things it misses, especially when dynamic calls
start happening.

Robert Landers
Software Engineer
Utrecht NL

11 months ago by Andreas Heigl — view source

unread

Hey Robert.

Am 19.05.24 um 09:38 schrieb Robert Landers:

Am 18.05.24 um 19:46 schrieb Robert Landers:

Hey all.

Am 18.05.24 um 16:00 schrieb Robert Landers:
[snip]
I guess that depends on what you mean by "break the code." Many people
turn warnings into exceptions, and for those people this will
effectively break their code. Some people may choose to ignore it one
way or another, but that would be up to them. The idea isn't to break
people's code though, it's to provide a way to mark very-malleable
APIs in libraries. Maybe, "Internal" is a bad name for this, but it's
the most logical.

The trouble I see is that there are two kind of people: Those that care
about such things and those that don't.

Those that care about such things will already find these issues during
static analysis.

Those that don't care will not turn errors into exceptions but instead
have error-reporting set to 0

That's why I am hesitant in adding this to the engine but instead would
love to see this as part of userland maintained static analyzers.

[snip]

Hey Andreas,

I don't think this is a useful way to look at it. Otherwise, what is
the point of making any error or warning? Static analysis is useful,
but there's a lot of things it misses, especially when dynamic calls
start happening.

Probably the most interesting question is about our and the users
expectation when declaring respectively using an internal entity.

Is it's use something that should just pop up in the logs? And if so:
What does it mean? Is using an internal entity outside it's expected
usage actually an error? After all an error should be used when the
application hits an issue preventing one or more functionalities from
properly functioning. We can not really say that is happening just
because someone uses a function in a place we do not expect it to be
used. It might still work.

But from the point of view of the person declaring that internal entity
muich more should happen. The code shouldn't work AT ALL any more.
Someone is using my code in a way that I did not intend it to be used,
so the whole application should break.

But we can not assert that.

So by having a look at the expectations of the different parties we can
calibrate our intentions and then see how we can provide the best solution.

And to me in that case the best solution would not be to enforce this on
the language level but to leave it to static analysis. Because I know
how inventive people can get to use things in ways they are not supposed
to be used. Search for composer unfinalize...

Cheers

Andreas

--
,,,
(o o)
+---------------------------------------------------------ooO-(_)-Ooo-+
| Andreas Heigl |
| mailto:andreas@heigl.org N 50°22'59.5" E 08°23'58" |
| https://andreas.heigl.org |
+---------------------------------------------------------------------+
| https://hei.gl/appointmentwithandreas |
+---------------------------------------------------------------------+
| GPG-Key: https://hei.gl/keyandreasheiglorg |
+---------------------------------------------------------------------+

11 months ago by Robert Landers — view source

unread

Hey Robert.

Am 19.05.24 um 09:38 schrieb Robert Landers:

Am 18.05.24 um 19:46 schrieb Robert Landers:

Hey all.

Am 18.05.24 um 16:00 schrieb Robert Landers:
[snip]
I guess that depends on what you mean by "break the code." Many people
turn warnings into exceptions, and for those people this will
effectively break their code. Some people may choose to ignore it one
way or another, but that would be up to them. The idea isn't to break
people's code though, it's to provide a way to mark very-malleable
APIs in libraries. Maybe, "Internal" is a bad name for this, but it's
the most logical.

The trouble I see is that there are two kind of people: Those that care
about such things and those that don't.

Those that care about such things will already find these issues during
static analysis.

Those that don't care will not turn errors into exceptions but instead
have error-reporting set to 0

That's why I am hesitant in adding this to the engine but instead would
love to see this as part of userland maintained static analyzers.

[snip]

Hey Andreas,

I don't think this is a useful way to look at it. Otherwise, what is
the point of making any error or warning? Static analysis is useful,
but there's a lot of things it misses, especially when dynamic calls
start happening.

Probably the most interesting question is about our and the users
expectation when declaring respectively using an internal entity.

Is it's use something that should just pop up in the logs? And if so:
What does it mean? Is using an internal entity outside it's expected
usage actually an error? After all an error should be used when the
application hits an issue preventing one or more functionalities from
properly functioning. We can not really say that is happening just
because someone uses a function in a place we do not expect it to be
used. It might still work.

Indeed! The issue isn't that it works, it's knowing what to check very
carefully when updating vendored code. It also sets a reasonable
expectation that I can't complain that a maintainer broke something in
my code because I was doing something unsupported.

When it is only used in static analysis, it won't be noticed by people
not using static analysis.

But from the point of view of the person declaring that internal entity
muich more should happen. The code shouldn't work AT ALL any more.
Someone is using my code in a way that I did not intend it to be used,
so the whole application should break.

But we can not assert that.

I disagree with this stance for many principled reasons.

If someone finds a class/method in library code useful, they should be
able to use it. Code reuse is important, and since the library code
could be licensed under the GPL while the main code is MIT or
unlicensed, the developer cannot simply copy the routine into the main
code. Further, they can't simply implement themselves now either,
because they've seen the licensed code.

Another issue I have with this stance is that if there is a major bug
in a library, I should be free to work around the issue by calling
"internal" code to ensure the state doesn't cause a crash in
production.

And yet another issue I have is that as a library author, I don't
really care how people use my code. It's none of my business. If it
works for them, great! I just want to be able to easily communicate
that some code may change unexpectedly between library versions.
Having something like this can be the difference between making huge
refactorings and improvements a patch release or a major release.

Code exists to solve problems, not create new ones and artificial
barriers because of ego.

Having this as part of the engine means that I know it is communicated
even if someone isn't using static analysis.

So by having a look at the expectations of the different parties we can
calibrate our intentions and then see how we can provide the best solution.

And to me in that case the best solution would not be to enforce this on
the language level but to leave it to static analysis. Because I know
how inventive people can get to use things in ways they are not supposed
to be used. Search for composer unfinalize...

Off-topic, but sharing this just to show my stance on this:

Final is an evil in library code, so I'm not surprised to see people
doing something like that, especially if final classes are used as
arguments to core library functions. It prevents authors from
attaching important metadata to objects, injecting failures during
tests, and ensuring application/business behavior is used -- and no,
composition cannot always solve this problem. I tend to find
alternatives if final gets in my way, or implement an alternative
myself.

Cheers

Andreas

--
,,,
(o o)
+---------------------------------------------------------ooO-(_)-Ooo-+
| Andreas Heigl |
| mailto:andreas@heigl.org N 50°22'59.5" E 08°23'58" |
| https://andreas.heigl.org |
+---------------------------------------------------------------------+
| https://hei.gl/appointmentwithandreas |
+---------------------------------------------------------------------+
| GPG-Key: https://hei.gl/keyandreasheiglorg |
+---------------------------------------------------------------------+

11 months ago by Barney Laurance — view source

unread

Hello internals,

I've been thinking about having an "internal" attribute that will emit
a warning if called from outside it's left-most namespace.

Hello Robert,

It's worth looking at the prior art on this - the @internal annotation
from PHPDoc, and its implementation PSalm, PhpStorm, and also the
@psalm-internal annotation, which I added to Psalm. @internal requires
the leftmost namespace segment to match between declaration and
reference, while @psalm-internal is used with a specification of the
namespace to which the annotated element is internal, which may be a
deeply nested namespace.

I had the idea to create @psalm-internal originally because the Drupal
project was recommending plugin and site developers create independent
work within the Drupal namespace, while having certain elements
documented as internal in the Drupal\Core namespace, with breaking
changes in point releases.

Another prominent left-most namespace with code developed by people who
might like a tool to help them avoid depending on each other's internals
is League, as in https://thephpleague.com/

I think if something like this is added to the language it would be a
shame not to give it something like the flexibility of @psalm-internal,
instead of only dealing with the left-most namespace part.

See

https://docs.phpdoc.org/latest/guide/references/phpdoc/tags/internal.html
https://psalm.dev/docs/annotating_code/supported_annotations/

I'm not currently a PHPStan user, but from what I read PHPStan doesn't
have this feature but has started considering it:
https://github.com/phpstan/phpstan/issues/1178

11 months ago by Robert Landers — view source

unread

On Sat, May 18, 2024 at 10:33 PM Barney Laurance
barney@barneylaurance.uk wrote:

Hello internals,

I've been thinking about having an "internal" attribute that will emit
a warning if called from outside it's left-most namespace.

Hello Robert,

It's worth looking at the prior art on this - the @internal annotation
from PHPDoc, and its implementation PSalm, PhpStorm, and also the
@psalm-internal annotation, which I added to Psalm. @internal requires
the leftmost namespace segment to match between declaration and
reference, while @psalm-internal is used with a specification of the
namespace to which the annotated element is internal, which may be a
deeply nested namespace.

I had the idea to create @psalm-internal originally because the Drupal
project was recommending plugin and site developers create independent
work within the Drupal namespace, while having certain elements
documented as internal in the Drupal\Core namespace, with breaking
changes in point releases.

Another prominent left-most namespace with code developed by people who
might like a tool to help them avoid depending on each other's internals
is League, as in https://thephpleague.com/

I think if something like this is added to the language it would be a
shame not to give it something like the flexibility of @psalm-internal,
instead of only dealing with the left-most namespace part.

See

https://docs.phpdoc.org/latest/guide/references/phpdoc/tags/internal.html
https://psalm.dev/docs/annotating_code/supported_annotations/

I'm not currently a PHPStan user, but from what I read PHPStan doesn't
have this feature but has started considering it:
https://github.com/phpstan/phpstan/issues/1178

Thanks Barney,

Yeah, I think having an optional parameter to specify the namespace
the class/function is internal to would be immensely useful on
bigger/complex codebases. It'd be annoying to spell it out, but so
long as the attribute definition isn't marked as final, people would
be able to extend it with defaults:

#[Attribute]
class InternalToMyPackage extends Internal {
public function __construct() {
parent::__construct("\MyPackage\Deeply\Nested");
}
}

Robert Landers
Software Engineer
Utrecht NL