First round of feature 'class'
#20647
Conversation
leonerd
force-pushed
the
feature-class
branch
from
ea397cc to
5d3f732
Compare
pad.c
Outdated
| =for apidoc_section $lexer | ||
| =for apidoc suspend_compcv | ||
|
|
||
| Implements part of the concept of a "suspended complication CV"; which can be |
There was a problem hiding this comment.
Will the concept of a "suspended complication CV" be explained in user-facing documentation in a subsequent commit (i.e., pod/perlsomething.pod)?
Also, what is the point of the semicolon in the line above. Shouldn't it either be a comma or rewritten as "suspended complication CV." This can be
There was a problem hiding this comment.
Will the concept of a "suspended complication CV" be explained in user-facing documentation in a subsequent commit (i.e.,
pod/perlsomething.pod)?
Not sure what you mean "user-facing"... it is explained in the rest of the sentence after the semicolon (to be a comma, as per below)
Also, what is the point of the semicolon in the line above. Shouldn't it either be a comma or rewritten as
"suspended complication CV." This can be
Ahyes a comma probably reads better.
|
Some perl-level warnings are being emitted during |
pod/perlclassguts.pod
Outdated
| This document intends to provide in-depth information about the way in which | ||
| the perl interpreter internals around the C<feature 'class'> syntax and overall | ||
| behaviour are provided. It is not intended as an end-user guide on how to use | ||
| the feature; for that see L<perlclass>. |
pod/perlclassguts.pod
Outdated
|
|
||
| =head2 Classes | ||
|
|
||
| A class is still fundamentally a package, and exists in the symbol table as an |
pod/perlclassguts.pod
Outdated
| =head2 Classes | ||
|
|
||
| A class is still fundamentally a package, and exists in the symbol table as an | ||
| HV with an aux structure in exactly the same way. It is distinguished from a |
|
I'm building from source on my Debian WSL container with |
|
For what it's worth, I just ported HTML::TokeParser::Simple to use |
|
Getting some odd behavior with #!/usr/bin/env perl
use 5.37.8;
use experimental 'class';
use Data::Dumper;
class Foo {}
my $foo = Foo->new;
say Dumper($foo);Running that prints: I assume this is expected, but it might be nice to have something a touch cleaner for this. |
|
Update: there is now a branch of Perl::Tidy is also struggling. Since it's not core, it probably is less important, but it's widely used. Leaving this here for others. #!perl
use 5.37.8;
use experimental 'class';
class Point {
field $x :param;
field $y :param;
method to_string () {
return "($x, $y)";
}
}Running I tried reading through my %is_my_our_state;
@q = qw(my our state);
@is_my_our_state{@q} = (1) x scalar(@q);To this: my %is_my_our_state;
@q = qw(my our state field);
@is_my_our_state{@q} = (1) x scalar(@q);And that fixes the issue. I'm sure there will be others in Perl::Tidy. You can also add
# or a sub or package BLOCK
elsif ( ( $last_nonblank_type eq 'i' || $last_nonblank_type eq 't' )
&& $last_nonblank_token =~ /^(sub|package|class)\b/ )
{
return $last_nonblank_token;
}
# or a sub alias
elsif (( $last_nonblank_type eq 'i' || $last_nonblank_type eq 't' )
&& ( $is_sub{$last_nonblank_token} ) )
{
return 'sub';
}
elsif ( $statement_type =~ /^(sub|package|class)\b/ ) {
return $statement_type;
}(I really should make these diffs and stop hacking my local perltidy code) I've filed a report for Perl::Tidy at https://rt.cpan.org/Ticket/Display.html?id=145706&results=44284c8317c4fa0392f65a81614b024c |
Should this be added to dist/Data-Dumper/t/ ? |
#!perl
use 5.37.8;
use experimental 'class';
use Test::Exception;
use Test::More;
BEGIN { use_ok('Data::Dumper') };
BEGIN { plan tests => 1; }
class Foo {}
my $foo = Foo->new;
lives_ok { say Dumper( $foo ) } 'expecting to live';A few comments on testing. First, you have Second, unless there's a standard for it in the Perl core, I wouldn't use Third, |
|
This is missing the build changes for win32 to compile class.c, you have to do that manually in the makefiles in win32. See the changes i did recently to add regcomp_debug.c: |
I tend to use fresh_perl_is() and fresh_perl_like(). |
leonerd
force-pushed
the
feature-class
branch
from
5d3f732 to
2113cbf
Compare
|
There are no examples of calling object methods. From within a method of the class, can we just call method() or do we need to call $self->method()? Are they equivalent? |
|
@leonerd I think we need an implicit #!/usr/bin/env perl
use lib 'lib';
use v5.37.8;
use HTML::TokeParser::Corinna::Token::Text;
my $token = HTML::TokeParser::Corinna::Token::Text->new( token => [ 'T', 'This is my text' ] ),That fails with:
The class I'm trying to instantiate looks like this: use experimental 'class';
class HTML::TokeParser::Corinna::Token::Text : isa(HTML::TokeParser::Corinna::Token) {
no warnings 'experimental::builtin';
use builtin 'true';
method is_text { true }
}
1;My expectation is that, this should be like using |
|
On Fri, 30 Dec 2022 at 10:44, Ovid ***@***.***> wrote:
class HTML::TokeParser::Corinna::Token::Text : isa(HTML::TokeParser::Corinna::Token) {
Maybe I have been overinfluenced by Rust, but I sure wish that lone ':'
token was as '->' or '=>' instead. It would read much nicer. There are a
*lot* of colons on that line.
cheers,
Yves
|
Agreed. It looks kinda ugly, but the KIM grammar we've adopted for the keywords is: Modifiers are always attributes. So Corinna only offers four new keywords (at this time) and all provide a consistent syntax: # KEYWORD IDENTIFIER MODIFIERS? DEFINITION?
class Foo :isa(Bar) { ... }
field $foo :param;
method foo :private { ... }
role Role::Serializable :does(Role::JSON) { ... }Once Damian suggested this syntax, it was a unanimous decision by the Corinna team to adopt it (I was pleased, but surprised, by the unanimity). Plus, it mostly leverages what we already have in Perl. |
|
On Fri, 30 Dec 2022 at 12:32, Ovid ***@***.***> wrote:
On Fri, 30 Dec 2022 at 10:44, Ovid *@*.***> wrote:
class HTML::TokeParser::Corinna::Token::Text : isa(HTML::TokeParser::Corinna::Token) {
Maybe I have been overinfluenced by Rust, but I sure wish that lone ':'
token was as '->' or '=>' instead. It would read much nicer. There are a
*lot* of colons on that line. cheers, Yves
Agreed. It looks kinda ugly, but the KIM grammar
<https://ovid.github.io/articles/language-design-consistency.html> we've
adopted for the keywords is:
KEYWORD IDENTIFIER MODIFIERS? DEFINITION?
Modifiers are always attributes. So Corinna only offers four new keywords
(at this time) and all provide a consistent syntax:
# KEYWORD IDENTIFIER MODIFIERS? DEFINITION?
class Foo :isa(Bar) { ... }
field $foo :param;
method foo :private { ... }
role Role::Serializable :does(Role::JSON) { ... }
Things often look fine when you have a single or double word class, but
dont when you have a deeply nested one. Our errors for require statements
have this problem, we duplicate the class name twice in the message, which
looks fine when its a single or maybe double word example, but becomes too
much When::It::Is::A::Really::Deeply::Nested::Class::Name. Oh well.
Once Damian suggested this syntax, it was a unanimous decision by the
Corinna team to adopt it (I was pleased, but surprised, by the unanimity).
Plus, it mostly leverages what we already have in Perl.
I can see the line of thought here. But playing devils advocate for a
moment, it doesn't seem like it would be a parse conflict to allow a =>
instead of colon to identify the modifiers. But I can live with "that boat
has sailed already". It reads very dense however. :-(
No reply necessary, just sharing my thoughts, I can live with this.
cheers,
Yves
…--
perl -Mre=debug -e "/just|another|perl|hacker/"
|
|
@leonerd I don't know if this is intended, but I thought we were also injecting a method normalize_tag {
my $new_tag = lc $tag;
return $class->new( token => [ 'E', $new_tag, sprintf "</%s>" => $new_tag ] );
}Changing Section 7.2 of the spec reads:
|
|
Update: Overload works fine. It's the Also, is it known that we cannot yet handle overloading? use experimental 'class';
class HTML::TokeParser::Corinna::Exception {
use overload '""' => 'to_string', fallback => 1;
use Devel::StackTrace;
field $message : param;
field $stack_trace = Devel::StackTrace->new->as_string;
method message () {$message}
method stack_trace () {$stack_trace}
method to_string () {
return join "\n" => $message, $stack_trace;
}
}
1;Later, I have this: eval { die HTML::TokeParser::Corinna::Exception->new( message => 'foo' ) };
explain $@;And it gives this warning: |
|
On Fri, 30 Dec 2022 at 16:45, Ovid ***@***.***> wrote:
Also, is it known that we cannot yet handle overloading?
use experimental 'class';
class HTML::TokeParser::Corinna::Exception {
use overload '""' => 'to_string', fallback => 1;
use Devel::StackTrace;
field $message : param;
field $stack_trace = Devel::StackTrace->new->as_string;
method message () {$message}
method stack_trace () {$stack_trace}
method to_string () {
return join "\n" => $message, $stack_trace;
}
}
1;
Later, I have this:
eval { die HTML::TokeParser::Corinna::Exception->new( message => 'foo' ) };
explain $@;
And it gives this warning:
cannot handle ref type 16 at /Users/ovid/perl5/perlbrew/perls/feature-class/lib/5.37.8/darwin-2level/Data/Dumper.pm line 212.
Please change the explain command to use Devel::Peek::Dump and let us
know what it says.
Yves
--
perl -Mre=debug -e "/just|another|perl|hacker/"
|
|
@demerphq Doing this: use Devel::Peek;
eval { die HTML::TokeParser::Corinna::Exception->new( message => 'foo' ) };
Dump($@);Resulted in this: |
|
On Fri, 30 Dec 2022 at 17:03, Ovid ***@***.***> wrote:
@demerphq <https://github.com/demerphq> Doing this:
use Devel::Peek;eval { die HTML::TokeParser::Corinna::Exception->new( message => 'foo' ) };
Dump($@);
Resulted in this:
SV = PV(0x13200b6a0) at 0x13200adc8
REFCNT = 1
FLAGS = (ROK)
RV = 0x1327dd228
SV = PVOBJ(0x13303d000) at 0x1327dd228
REFCNT = 1
FLAGS = (OBJECT)
STASH = 0x1327dd1f8 "HTML::TokeParser::Corinna::Exception"
MAXFIELD = 1
FIELDS = 0x600003216430
Field No. 0 ($message)
SV = PV(0x132852e20) at 0x133046030
REFCNT = 1
FLAGS = (POK,pPOK)
PV = 0x600003216280 "foo"\0
CUR = 3
LEN = 16
Field No. 1 ($stack_trace)
SV = PV(0x132852700) at 0x1326b1090
REFCNT = 1
FLAGS = (POK,pPOK)
PV = 0x131ebc390 "Trace begun at lib/HTML/TokeParser/Corinna/Exception.pm line 9\nHTML::TokeParser::Corinna::Exception::__ANON__('HTML::TokeParser::Corinna::Exception=OBJECT(0x1327dd228)', '** argument not available anymore **') called at t/exceptions.t line 16\neval {...} at t/exceptions.t line 16\n"\0
CUR = 280
LEN = 282
PV = 0x1327dd228 ""
CUR = 0
LEN = 0``
Ok, so the feature class patch breaks Data::Dumper and likely anything XS
that expects to handle a code ref (including the perl internals). We
havent taught various bits of code about this new type SvPVOBJ. I guess
there is a good reason we aren't generating a PVCV, but this means a lot of
code will have to be changed to deal with this. I have to admit, I find
this surprising. This means a lot more code has to change than is necessary
if this was just a special type of PVCV. I think this might be a bigger
blocker than Paul realized. We have 233 mentions of PVCV just in the core
repo alone, and god knows how many out there in the wild. A lot of code
will expect that the only form of a code ref will be a PVCV.
Yves
…--
perl -Mre=debug -e "/just|another|perl|hacker/"
|
|
On Fri, 30 Dec 2022 at 17:23, demerphq ***@***.***> wrote:
On Fri, 30 Dec 2022 at 17:03, Ovid ***@***.***> wrote:
> @demerphq <https://github.com/demerphq> Doing this:
>
> use Devel::Peek;eval { die HTML::TokeParser::Corinna::Exception->new( message => 'foo' ) };
> Dump($@);
>
> Resulted in this:
>
> SV = PV(0x13200b6a0) at 0x13200adc8
> REFCNT = 1
> FLAGS = (ROK)
> RV = 0x1327dd228
> SV = PVOBJ(0x13303d000) at 0x1327dd228
> REFCNT = 1
> FLAGS = (OBJECT)
> STASH = 0x1327dd1f8 "HTML::TokeParser::Corinna::Exception"
> MAXFIELD = 1
> FIELDS = 0x600003216430
> Field No. 0 ($message)
> SV = PV(0x132852e20) at 0x133046030
> REFCNT = 1
> FLAGS = (POK,pPOK)
> PV = 0x600003216280 "foo"\0
> CUR = 3
> LEN = 16
> Field No. 1 ($stack_trace)
> SV = PV(0x132852700) at 0x1326b1090
> REFCNT = 1
> FLAGS = (POK,pPOK)
> PV = 0x131ebc390 "Trace begun at lib/HTML/TokeParser/Corinna/Exception.pm line 9\nHTML::TokeParser::Corinna::Exception::__ANON__('HTML::TokeParser::Corinna::Exception=OBJECT(0x1327dd228)', '** argument not available anymore **') called at t/exceptions.t line 16\neval {...} at t/exceptions.t line 16\n"\0
> CUR = 280
> LEN = 282
> PV = 0x1327dd228 ""
> CUR = 0
> LEN = 0``
>
>
Ok, so the feature class patch breaks Data::Dumper and likely anything XS
that expects to handle a code ref (including the perl internals). We
havent taught various bits of code about this new type SvPVOBJ. I guess
there is a good reason we aren't generating a PVCV, but this means a lot of
code will have to be changed to deal with this. I have to admit, I find
this surprising. This means a lot more code has to change than is necessary
if this was just a special type of PVCV. I think this might be a bigger
blocker than Paul realized. We have 233 mentions of PVCV just in the core
repo alone, and god knows how many out there in the wild. A lot of code
will expect that the only form of a code ref will be a PVCV.
HMM, actually i just realized PVOBJ is distinct from PVCV, so some of the
above is totally wrong, apologies. So long as it passes is_object() most
code will be fine. Except for code that expects to introspect the object
via the normal type system. What should Data::Dumper do witha
PVOBJ reference? Dump it like it would dump a hash? Same question for
Sereal and Storable and other serialization tools. So while I was wrong
that the scope of this problem is proportional to PVCV checks, but anything
that expects to introspect on the type of an object will be broken by this
new type.
Yves
…--
perl -Mre=debug -e "/just|another|perl|hacker/"
|
| { | ||
| class Test2 { | ||
| my $ok = "OK"; | ||
| sub NotAMethod { return $ok } |
There was a problem hiding this comment.
Will it (having my and sub within classes) go away in the future?
What's the use of it? Is it the same as static methods as private methods will have method :private syntax I believe.
It's currently a bit confusing.
There was a problem hiding this comment.
@rwp0 Not sure if I understand your question, but I'll try to answer what I think you're asking.
sub will still be allowed in classes. There's no problem with that, but eventually we'll need work to ensure that subroutines cannot be called as methods.
Also, when the MOP is online, we'll need debugger work to ensure that typing m only shows subroutines, not methods (might be possibly now, but I haven't looked into it).
my will also be allowed in a top-level scope in a class (no sense arbitrarily disallowing it). It will effectively be class data, but as a my variable would be an internal implementation detail rather than a structural part of the class, its use should be strongly discouraged if the anything outside the class is able to use that data.
leonerd
force-pushed
the
feature-class
branch
from
e9dc443 to
47ac109
Compare
There was a problem hiding this comment.
Also maybe worthy to note is that the docs stand out in using British English spelling while the rest of Perl POD is using American English spelling.
cf. initialise -> initialize which is caught by spellcheckers, GitHub editors included.
But I think that's okay because TMTOWTDI (ie. variation in written English).
pod/perlclassguts.pod
Outdated
| =head2 C<ADJUST> Phasers | ||
|
|
||
| During compiletime, parsing of an C<ADJUST> phaser is handled in a | ||
| fundamentally different way to the the existing perl phasers (C<BEGIN>, |
There was a problem hiding this comment.
fundamentally different way to the the existing perl phasers (C,
Repeated "the"
pod/perlclassguts.pod
Outdated
| C<xhv_class_fields> will point to a C<PADNAMELIST> containing C<PADNAME>s, | ||
| each being one defined field of the class. They are stored in order of | ||
| declaration. Note however, that the index into this array will not necessarily | ||
| be equal to the fieldix of each field, because in the case of a subclass, the |
pod/perlclassguts.pod
Outdated
|
|
||
| #define SVt_PVOBJ | ||
|
|
||
| An SV type constant used for comparision with the C<SvTYPE()> macro. |
There was a problem hiding this comment.
comparision -> comparison
extra i
pod/perlclass.pod
Outdated
|
|
||
| =item * Metaprogramming | ||
|
|
||
| An extension of the metaprogramming API (currently proposed by RFC0022) which |
There was a problem hiding this comment.
Maybe link RFC's in POD for better visibility esp. by newcomers (or those who are unaware of the RFC) since googling them is hard.
But, that's unfortunately not an RFC yet, only a PR.
Adds a new experimental warning, feature, keywords and enough parsing to implement basic classes with an empty `new` constructor method. Inject a $self lexical into method bodies; populate it with the object instance, suitably shifted Creates a new OP_METHSTART opcode to perform method setup Define an aux flag to remark which stashes are classes Basic implementation of fields. Basic anonymous methods.
…o attrs are yet defined
… with it and break stuff
…so eventually it can store OP fragments
Allows non-constant expressions with side effects. Evaluated during the constructor of each instance.
* perlclass.pod: Document field initialising expressions and :param attribute * perlclass.pod: Add a TODO section to explain what more is to be added * Added pod/perlclassguts.pod which contains internal implementation notes and details about how the class system works.
leonerd
force-pushed
the
feature-class
branch
from
47ac109 to
734489d
Compare
|
Thanks @rwp0 for the docs review. I believe I've fixed those all up now. If we're all happy with this I'd like to start a merge sometime soon. My attack plan is probably going to be three rounds of commandline Edit: @demerphq's review is still currently in "changes requested" state because of the |
|
This has now been manually merged in stages, so I can close this PR now. |
|
Congrats @leonerd |
|
@leonerd sorry for the late response but while reading 5.37.9s perlclass, I wondered why you wrote |
|
@abraxxa |
|
So Perl 5.38 adds the class and field keywords even without use v5.38? |
|
No. Feature bundles have no relation to feature 'class' yet.
Though 5.38 could add module_true which the class keyword probably provides
by default.
28 fev 2023, Ç.a. 20:15 tarixində Alexander Hartmaier <
***@***.***> yazdı:
… So Perl 5.38 adds the class and field keywords even without use v5.38?
—
Reply to this email directly, view it on GitHub
<#20647 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AQHI3ZZMASZAKFQGIVGKKRLWZYQBZANCNFSM6AAAAAATIUSODA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
No, |
Overall Notes To Reviewers
This is an epic-sized PR. I am not expecting that any one person can review everything all at once. It is perfectly fine to decide to just review some part of it - maybe just the docs, or the tests, or some small part of the implementation. Whatever you feel most confident in. When you leave a review comment please explain which part(s) of the PR you read and are commenting on, so it's clear.
Reminder: You do not have to be an internals expert, or even read any C code at all, to be able to review and comment on the tests, documentation, and overall shape of the idea.
Additionally, anyone who does wish to read and comment on the actual implementation is advised to start by reading the new documentation file
pod/perlclassguts.podwhich explains many details of how the internals work. Please start there to try to understand how the pieces all fit together. If there's something still unclear after reading it then that in itself is a valuable review comment. :) So please let me know.PR Specifics
I have tried to squash down my original list of about 60 commits into larger pieces here, but I've still kept various stages split apart by empty-commit markers, as well as leaving several of them separate generally. I'm open to suggestion on whether to further squash them down before merging.
In particular the very first commit, titled "Define the concept of a suspended compcv", could stand alone as its own PR to be merged directly first as a prerequisite, although it would have no real utility in core other than to support the rest of this work here so it seems less useful to bother doing so in its own PR.