Everything
php
Learn Community Releases The PHP Project
Home » Project » Newsgroups » php.internals » [Discussion] "Internal" attribute and warning »

[Discussion] "Internal" attribute and warning

php.internals

Robert Landers

2 years ago
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

Saki Takamachi

2 years ago
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

Rowan Tommins [IMSoP]

2 years ago
On 18/05/2024 15:00, Robert Landers 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". 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]

Robert Landers

2 years ago
On Sat, May 18, 2024 at 5:18 PM Rowan Tommins [IMSoP] <imsop.php@rwec.co.uk> wrote:
> > On 18/05/2024 15:00, Robert Landers 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.

Rowan Tommins [IMSoP]

2 years ago
On 18 May 2024 16:25:06 BST, Robert Landers <landers.robert@gmail.com> wrote:
>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]

Andreas Heigl

2 years ago
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 | +---------------------------------------------------------------------+

Larry Garfield

2 years ago
On Sat, May 18, 2024, at 11:06 AM, Andreas Heigl wrote:
> 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". 1. 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. 2. Should the package be implicit, or explicit? If it's a namespace, is it auto-implicit or an "empty" package? 3. 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

Robert Landers

2 years ago
On Sat, May 18, 2024 at 7:58 PM Larry Garfield <larry@garfieldtech.com> wrote:
> > On Sat, May 18, 2024, at 11:06 AM, Andreas Heigl wrote: > > 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". > > 1. 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. > 2. Should the package be implicit, or explicit? If it's a namespace, is it auto-implicit or an "empty" package? > 3. 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.

Larry Garfield

2 years ago
On Sat, May 18, 2024, at 6:15 PM, Robert Landers wrote:
>> I think an important question to answer here is what we want to have as our definition of "package". >> >> 1. 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. >> 2. Should the package be implicit, or explicit? If it's a namespace, is it auto-implicit or an "empty" package? >> 3. 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

Robert Landers

2 years ago
On Mon, May 20, 2024 at 6:58 PM Larry Garfield <larry@garfieldtech.com> wrote:
> > On Sat, May 18, 2024, at 6:15 PM, Robert Landers wrote: > > >> I think an important question to answer here is what we want to have as our definition of "package". > >> > >> 1. 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. > >> 2. Should the package be implicit, or explicit? If it's a namespace, is it auto-implicit or an "empty" package? > >> 3. 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: 0. 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. 1. vote "no" before it ever gets past the RFC stage 2. create a competing RFC 3. create an RFC to deprecate/change the functionality afterwards 4. 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

Robert Landers

2 years ago
On Sat, May 18, 2024 at 7:38 PM Andreas Heigl <andreas@heigl.org> wrote:
> > 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

Andreas Heigl

2 years ago
Am 18.05.24 um 19:46 schrieb Robert Landers:
> On Sat, May 18, 2024 at 7:38 PM Andreas Heigl <andreas@heigl.org> wrote: >> >> 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 | +---------------------------------------------------------------------+

Robert Landers

2 years ago
On Sat, May 18, 2024 at 9:11 PM Andreas Heigl <andreas@heigl.org> wrote:
> > > > Am 18.05.24 um 19:46 schrieb Robert Landers: > > On Sat, May 18, 2024 at 7:38 PM Andreas Heigl <andreas@heigl.org> wrote: > >> > >> 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

Andreas Heigl

2 years ago
Hey Robert. Am 19.05.24 um 09:38 schrieb Robert Landers:
> On Sat, May 18, 2024 at 9:11 PM Andreas Heigl <andreas@heigl.org> wrote: >> >> >> >> Am 18.05.24 um 19:46 schrieb Robert Landers: >>> On Sat, May 18, 2024 at 7:38 PM Andreas Heigl <andreas@heigl.org> wrote: >>>> >>>> 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 | +---------------------------------------------------------------------+

Robert Landers

2 years ago
On Sun, May 19, 2024 at 10:52 AM Andreas Heigl <andreas@heigl.org> wrote:
> > Hey Robert. > > Am 19.05.24 um 09:38 schrieb Robert Landers: > > On Sat, May 18, 2024 at 9:11 PM Andreas Heigl <andreas@heigl.org> wrote: > >> > >> > >> > >> Am 18.05.24 um 19:46 schrieb Robert Landers: > >>> On Sat, May 18, 2024 at 7:38 PM Andreas Heigl <andreas@heigl.org> wrote: > >>>> > >>>> 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.

Barney Laurance

2 years ago
On 18/05/2024 15:00, Robert Landers 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

Robert Landers

2 years ago
On Sat, May 18, 2024 at 10:33 PM Barney Laurance <barney@barneylaurance.uk> wrote:
> > On 18/05/2024 15:00, Robert Landers 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