-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Lexically Require Module #36
Open
Ovid
wants to merge
4
commits into
Perl:main
Choose a base branch
from
Ovid:main
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,152 @@ | ||
# Lexically require modules | ||
|
||
## Preamble | ||
|
||
Author: Ovid | ||
Sponsor: | ||
ID: 23 | ||
Status: Draft | ||
|
||
## Abstract | ||
|
||
When writing a module, the `use` and `require` statements make the modules | ||
available globally. This leads to strange bugs where you can write | ||
`my $object = Some::Class->new` and have it work, even if you didn't | ||
require that module. This transitive dependency is fragile and will break | ||
if the code requiring `Some::Class` decides to no longer require it. | ||
|
||
This PPC proposes: | ||
|
||
```perl | ||
package Foo; | ||
use feature 'lexical_require`; | ||
use Some::Module; | ||
``` | ||
|
||
With the above, code outside of the above scope cannot see `Some::Module` unless | ||
it explicitly requires it. | ||
|
||
## Motivation | ||
|
||
* Accidentally relying on transitive dependencies is fragile because, unless | ||
documented, transitive dependencies are not guaranteed to exist. | ||
* Currently, loading a module injects it into a global namespace, so it's not | ||
easy to prevent this problem. | ||
* Transitive dependencies are even more fragile is the code is conditionally | ||
required: | ||
|
||
```perl | ||
if ($true) { | ||
require Some::Module; | ||
} | ||
``` | ||
|
||
In the above, the transitive dependency can fail if `$true` is never true. | ||
|
||
The initial discussion is on [the P5P mailing | ||
list](https://www.nntp.perl.org/group/perl.perl5.porters/2023/07/msg266678.html). | ||
|
||
## Rationale | ||
|
||
* The new syntax ensures that the module author can `require` or `use` a module and not | ||
worry that other code will accidentally be dependent on internal implementation details. | ||
|
||
## Specification | ||
|
||
For the given lexical scope—block or file—`use feature 'lexical_require'` will | ||
allow code to use the required module. Code _outside_ of that scope cannot use | ||
the required module unless it explicitly uses it, or there's another | ||
transitive dependency injecting that module into the global namespace. | ||
|
||
```perl | ||
package Foo { | ||
use feature 'lexical_require'; | ||
use Hash::Ordered; | ||
no feature 'lexical_require'; | ||
use Some::Class; | ||
... | ||
} | ||
my $object = Some::Class->new; # succeeds if `Some::Class` has a `new` method | ||
my $cache = Hash::Ordered->new; # fails | ||
``` | ||
|
||
## Backwards Compatibility | ||
|
||
This feature should be 100% backwards compatible for new code. If retrofitted | ||
into existing code, any code relying on a transitive dependency might break | ||
and need to explicitly declare that dependency. | ||
|
||
These are probably going to be runtime errors, not compile-time. | ||
|
||
Other than the above caveats, I am not aware of any tooling which will be | ||
negatively impacted by this. However, I don't know the | ||
[`Devel::Cover`](https://metacpan.org/pod/Devel::Cover) internals and I | ||
suspect there might be an issue there. | ||
|
||
I suspect (hope), that the following will not be impacted: | ||
|
||
* [`B::Deparse`](https://metacpan.org/pod/B::Deparse) | ||
* [`Devel::NYTProf`](https://metacpan.org/pod/Devel::NYTProf) | ||
* [`PPI`](https://metacpan.org/pod/PPI) (hence [`Perl::Critic`](https://metacpan.org/pod/Perl::Critic) etc) | ||
|
||
## Security Implications | ||
|
||
If anything, this might improve security by not allowing code to have an | ||
accidental dependency on code it doesn't explicitly use. | ||
|
||
## Examples | ||
|
||
From the above: | ||
|
||
```perl | ||
package Foo { | ||
use feature 'lexical_require'; | ||
use Hash::Ordered; | ||
no feature 'lexical_require'; | ||
use Some::Class; | ||
... | ||
} | ||
my $object = Some::Class->new; # succeeds if `Some::Class` has a `new` method | ||
my $cache = Hash::Ordered->new; # fails | ||
``` | ||
|
||
## Prototype Implementation | ||
|
||
None. | ||
|
||
## Future Scope | ||
|
||
In the future, it might be nice to have `namespace` declarations. | ||
|
||
```perl | ||
namespace Las::Vegas; | ||
package ::Casino; # package Las::Vegas::Casino | ||
|
||
``` | ||
|
||
For the above, what happens in `Las::Vegas` stays in `Las::Vegas`. | ||
|
||
The above allows you to declare a new namespace and everything within that | ||
namespace is private to it. Only code that is officially exposed can be used. | ||
Companies can have teams using separate namespaces and only official APIs can | ||
be accessed. You can't "reach in" and override subroutines. This would require | ||
developers to plan their designs more carefully, including a better | ||
understanding of dependency injection and building flexible interfaces. | ||
|
||
## Rejected Ideas | ||
|
||
There really hasn't been any previous solutions on the CPAN that I've seen. | ||
I've seen closure-based solutions using lexical subs, but they're harder to | ||
write. | ||
|
||
## Open Issues | ||
|
||
We may have issues with test suites. They often take advantage of locally | ||
overriding/replacing a subroutine and if that's declared in a transitive | ||
dependency, it might fail. | ||
|
||
## Copyright | ||
|
||
Copyright (C) 2023, Curtis "Ovid" Poe | ||
|
||
This document and code and documentation within it may be used, redistributed and/or modified under the same terms as Perl itself. |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think @Grinnz is quite fairly calling out this as confusing or ambiguous. It should probably be worded:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@demerphq suggested:
I read that as changing the meaning, but I'm unsure if it was meant to. Rewriting it:
However, that doesn't mean the transitive dependencies aren't available. If scope ß uses
lexical_require
andHash::Ordered
, but scope ∂ usesHash::Ordered
but doesn't uselexical_require
, thenHash::Ordered
is still available to everyone as a transitive dependency.Thus, this doesn't offer perfect protection, but it does mean that I can write my own code in such a way that people will be less likely to rely on my internals.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the problem with this is what @Grinnz was concerned about: use of a lexical feature in one scope is affecting code in another totally unrelated scope. In simple terms that is a no-no.
Its fine to say "within THIS scope I cant do something", but the way you have worded it becomes action at a distance, and a practical problem to implement to boot. IOW, if you want those semantics you probably cant have the feature. If you are willing to adopt my version on the other hand it is doable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, now I know I'm understanding you correctly. I'll rewrite the PPC as soon as I practically can. Makes total sense.