Class methods can still be replaced outside the class

[Ovid]

Description

Classes cannot be redeclared. Declaring one more than once causes a compile time failure:

perl -E 'use experimental "class"; class Test; say "Got to here"; class Test'
Cannot reopen existing class "Test" at -e line 1.

This was designed to prevent sloppy programmers rewriting classes without going through the MOP (which, to be fair, doesn't yet exist). However, it's still easy to workaround this issue, as shown in the next section.

Steps to Reproduce

use v5.40.0;
use experimental 'class';

class Test {
    field $name :reader :param;
}

my $test = Test->new(name => 'test');
say $test->name;

no warnings 'redefine';
*Test::name = sub { 'test2' };
say $test->name;

{
    package Test;
    sub name { 'test3' };
}

say $test->name;

And that prints:

test3
test2
test2

We never see our test value and test3 happens before test2 due to control flow. It's a touch confusing and is the kind of bugs we'd like to avoid.

Expected behavior

It would be nice if attempting to alter/replace any method, other than using the MOP, was fatal. I recognize that classes and packages using the same namespace mechanism possibly makes this difficult to address.

Perl configuration

Summary of my perl5 (revision 5 version 40 subversion 0) configuration:
   
  Platform:
    osname=darwin
    osvers=23.5.0
    archname=darwin-2level
    uname='darwin ovids-m1-laptop.local 23.5.0 darwin kernel version 23.5.0: wed may 1 20:12:58 pdt 2024; root:xnu-10063.121.3~5release_arm64_t6000 arm64 '
    config_args='-de -Dprefix=/Users/ovid/perl5/perlbrew/perls/perl-5.40.0 -Aeval:scriptdir=/Users/ovid/perl5/perlbrew/perls/perl-5.40.0/bin'
    hint=recommended
    useposix=true
    d_sigaction=define
    useithreads=undef
    usemultiplicity=undef
    use64bitint=define
    use64bitall=define
    uselongdouble=undef
    usemymalloc=n
    default_inc_excludes_dot=define
  Compiler:
    cc='cc'
    ccflags ='-fno-common -DPERL_DARWIN -mmacosx-version-min=14.5 -DNO_THREAD_SAFE_QUERYLOCALE -DNO_POSIX_2008_LOCALE -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include -I/opt/local/include'
    optimize='-O3'
    cppflags='-fno-common -DPERL_DARWIN -mmacosx-version-min=14.5 -DNO_THREAD_SAFE_QUERYLOCALE -DNO_POSIX_2008_LOCALE -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include -I/opt/local/include'
    ccversion=''
    gccversion='Apple LLVM 15.0.0 (clang-1500.3.9.4)'
    gccosandvers=''
    intsize=4
    longsize=8
    ptrsize=8
    doublesize=8
    byteorder=12345678
    doublekind=3
    d_longlong=define
    longlongsize=8
    d_longdbl=define
    longdblsize=8
    longdblkind=0
    ivtype='long'
    ivsize=8
    nvtype='double'
    nvsize=8
    Off_t='off_t'
    lseeksize=8
    alignbytes=8
    prototype=define
  Linker and Libraries:
    ld='cc'
    ldflags =' -mmacosx-version-min=14.5 -fstack-protector-strong -L/usr/local/lib -L/opt/local/lib'
    libpth=/usr/local/lib /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/clang/15.0.0/lib /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/lib /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib /opt/local/lib /usr/lib
    libs=-lgdbm
    perllibs=
    libc=
    so=dylib
    useshrplib=false
    libperl=libperl.a
    gnulibc_version=''
  Dynamic Linking:
    dlsrc=dl_dlopen.xs
    dlext=bundle
    d_dlsymun=undef
    ccdlflags=' '
    cccdlflags=' '
    lddlflags=' -mmacosx-version-min=14.5 -bundle -undefined dynamic_lookup -L/usr/local/lib -L/opt/local/lib -fstack-protector-strong'


Characteristics of this binary (from libperl): 
  Compile-time options:
    HAS_LONG_DOUBLE
    HAS_STRTOLD
    HAS_TIMES
    PERLIO_LAYERS
    PERL_COPY_ON_WRITE
    PERL_DONT_CREATE_GVSV
    PERL_HASH_FUNC_SIPHASH13
    PERL_HASH_USE_SBOX32
    PERL_MALLOC_WRAP
    PERL_OP_PARENT
    PERL_PRESERVE_IVUV
    PERL_USE_SAFE_PUTENV
    USE_64_BIT_ALL
    USE_64_BIT_INT
    USE_LARGE_FILES
    USE_LOCALE
    USE_LOCALE_COLLATE
    USE_LOCALE_CTYPE
    USE_LOCALE_NUMERIC
    USE_LOCALE_TIME
    USE_PERLIO
    USE_PERL_ATOF
  Built under darwin
  Compiled at Jul  2 2024 09:26:02
  %ENV:
    PERL5LIB="/Users/ovid/perlcustom/lib"
    PERLBREW_HOME="/Users/ovid/.perlbrew"
    PERLBREW_MANPATH="/Users/ovid/perl5/perlbrew/perls/perl-5.40.0/man"
    PERLBREW_PATH="/Users/ovid/perl5/perlbrew/bin:/Users/ovid/perl5/perlbrew/perls/perl-5.40.0/bin"
    PERLBREW_PERL="perl-5.40.0"
    PERLBREW_ROOT="/Users/ovid/perl5/perlbrew"
    PERLBREW_SHELLRC_VERSION="0.98"
    PERLBREW_VERSION="0.98"
  @INC:
    /Users/ovid/perlcustom/lib
    /Users/ovid/perl5/perlbrew/perls/perl-5.40.0/lib/site_perl/5.40.0/darwin-2level
    /Users/ovid/perl5/perlbrew/perls/perl-5.40.0/lib/site_perl/5.40.0
    /Users/ovid/perl5/perlbrew/perls/perl-5.40.0/lib/5.40.0/darwin-2level
    /Users/ovid/perl5/perlbrew/perls/perl-5.40.0/lib/5.40.0

[leonerd]

Classes cannot be redeclared. Declaring one more than once causes a compile time failure:

perl -E 'use experimental "class"; class Test; say "Got to here"; class Test'
Cannot reopen existing class "Test" at -e line 1.

This was designed to prevent sloppy programmers rewriting classes without going through the MOP

That's not really the case, though. Reöpening a class means having to invalidate the constructor and field-init methods, because it might now gain more fields. Additionally, because fields are just weirdly scoped lexical variables, any newly-created methods in this "new scope but same class" wouldn't be able to see those existing fields. But it's all about the fields.

As you point out, adding or replacing methods would be permitted via whatever MOP-like system exists (likely something in meta). These are just neater notations for what you can already do currently with glob refs, for people allergic to writing the * symbol. There doesn't seem to be anything to be gained by permitting methods to be replaced via MOP/meta, but not by glob refs.

[Ovid]

@leonerd wrote:

There doesn't seem to be anything to be gained by permitting methods to be replaced via MOP/meta, but not by glob refs.

In a large code review, it's easy to miss a subtle *, but it's easier to see they've loaded the MOP and are doing something interesting. It's about making something that might be bad easier to spot in the code.

[guest20]

@Ovid If you are missing things in your code review, please do adjust your set.

I'm not sure anyone should be reviewing code in an environment when they don't have the time / space / legibility / attention to actually sit down and read the code being submitted. If the patch is too big either make the review longer, or the patch shorter.

To me it feels like a longer syntax for replacing methods makes the problem of "a big patch" worse, because more code, no?

[Ovid]

@guest20 I hear where you're coming from and I wish that could work. In reality, people have deadlines, they get tired, they make mistakes.

To me it feels like a longer syntax for replacing methods makes the problem of "a big patch" worse, because more code, no?

That depends on the "longer syntax". Sure, you have to write more code to allow $object->limit(20) to be validated correctly, but $object->{limit} = 20 has no validation whatsoever and when it's used inside the code, it's far more likely that you'll encounter a problem because encapsulation isn't just about state, it's about state plus behavior.

In the case of using * to "fix" a method, what happens if you have this?

sub replace_method ( $self, $sub_to_replace, $new_sub ) {
    $self->_assert_fqname($sub_to_replace);
    no strict 'refs';
    $self->_maybe_cache(*$sub_to_replace{CODE});
    no warnings 'redefine';
    *$sub_to_replace = $new_sub;
    return $self;
}

You can't tell by visual inspection if that's correct or not. So let's rewrite it.

sub replace_method ( $self, $sub_to_replace, $new_sub ) {
    $self->_assert_fqname($sub_to_replace);
    my ( $package, $subname ) = $sub_to_replace =~ /^(.*)::(.*)$/;
    my $metapackage = meta::get_package($package);
    $self->_maybe_cache($metapackage->get_symbol($subname));
    $metapackage->add_symbol( $subname, $new_sub );
    return $self;
}

It's not much longer than the original version. In the above, we not only don't have to change the interface, but we no longer have to disable strict or warnings, and we're much more likely to have a correctly functioning code.

If we're short on time or resources, better to write solid code that we can test and verify it works rather than rely on the human frailty of code reviews. I've been doing the latter for many years and things slip through the cracks all the time.

And regarding my second version of that sub, did you see the bug in it? it won't run correctly, but unlike the first version, it will at least die instead of failing silently.

[leonerd]

These are certainly some excellent reasons why your code style guide, reviewer notes, etc.. should suggest the use of safer mechanisms like MOP/meta, over plain globref hackery. But by itself it doesn't seem a compelling reason why we should intentionally break the globref mechanism by adding special code to forbid it in these situations, purely for the reason of "we don't like it".

[ajmetz]

So is the discussion about whether or not class methods should be allowed to be replaced outside the class or not, by means other than meta or MOP::Class (and currently they can be)...?

I've recently begun following Perl on github, to keep me abreast of things,
and it's great to have learned about
https://metacpan.org/pod/meta
https://metacpan.org/pod/Object::Pad::MOP::Class
and to have skim read a bit of:
https://github.com/Perl/PPCs/blob/main/ppcs/ppc0022-metaprogramming.md
...even if my Apple Mac brain still says PowerPC every time it encounters PPC, ^_^.
Ah! Just seen it stands for Proposed Perl Changes =).
Very cool.
https://github.com/Perl/PPCs

[guest20]

@Ovid If you don't have time to actually do code reviews, you're not actually reviewing the code. Just remove reviews from your work flow completely, because there's a high chance you're just approving based on author anyway. Pretending to do reviews takes much more time than not pretending to do reviews, so you'll have an even easier time of getting patches merged before your deadline.

I don't know why you think that second version is easier to read. I don't know what any of those _ subs do and only thanks to @ajmetz did i find out that meta:: wasn't more facebook nonsense.

I do know what assigning a code-ref to *{ $thing } does though. I also rely heavily on that when setting up mocks in my unit tests.

@ajmetz

• Pay Per Click, i guess my brain isn't as rainbow-apple as I thought
• handy, all i got when searching ddg for "perldoc meta" was metacpan and Parse::CPAN::Meta

[ajmetz]

Re: PPC [ Proposed Perl Changes, PowerPC processor, Pay Per Click advertising... ] Ah..that's me...too much in the editorial world, and need a better head for advertising. ^_^.

I like ovid's point, because it's metaphorically akin to fixing a leaky bucket.

Of course, I don't normally use * (I am one of those people "allergic" to writing it) - except am currently using it to redefine a sub of Data Dumper, to get proper UTF-8 characters in the dumps ala https://stackoverflow.com/questions/50489062/

Presumably, if Data Dumper upgraded itself to use the class feature, AND redefining a method via a glob was made fatal, I'd have to update my glob line of code to use proper meta ways of doing the same thing. Of course, if Data Dumper ever got a UTF-8 output method, that might be preferable. Maybe there's a CPAN module that's a simple wrapper for Data Dumper and does that already, that I could look for? Anyway, anything of that ilk would all help make my code look less like a dirty hack copy and pasted from stack overflow, =).

Because the code would look cleaner and I wouldn't feel I was resorting to hacks,
I am sympathetic to Ovid's asking for it to be fatal,
and realise Leonerd is saying not liking the approach isn't compelling enough to make the approach fatal.
So my question is - is this what the debate comes down to?
Is that why it's in "triage" - to get some community debate going about whether there is any point to going as far as fatal, ultimately to arrive at a decision?
If you attempted to do this without using the meta approach - do you get even a warning?
Perhaps a warning might be a good compromise until this debate over whether it should be fatal or not can be decided.

I'm not nearly as advanced as Leonerd or Ovid when it comes to Perl. You are rolemodels and expert peers within the Perl community, and I am a mere mortal, trying to understand the thread, =) [and totally hyped for October's London Perl Workshop, where I can hopefully rub shoulders with such Perl experts and learn a thing or two].

P.S. I didn't search for perldoc meta - I simply saw meta::get_package in the code, so realised meta was a package name, and went to cpan.org to and typed in "meta" in the search box, and clicked on the result with 11+ good reviews, =). - https://metacpan.org/pod/meta - the POD then led me to: https://github.com/Perl/PPCs/blob/main/ppcs/ppc0022-metaprogramming.md

[Ovid]

@guest20 wrote:

If you don't have time to actually do code reviews, you're not actually reviewing the code. Just remove reviews from your work flow completely, because there's a high chance you're just approving based on author anyway.

Yes, that would be true if it's not "actually reviewing the code" and yes, I've seen "code reviews" that get rubber stamps based on author, so I get your point. But I've routinely found that even code reviews which miss stuff are generally not rubber stamps. We still get value out of them, including finding bugs and knowledge sharing. It's not perfect, but it's not worthless. And it's not a simple either/or, so saying "if it's not perfect, you shouldn't try" doesn't help.

[Ovid]

As an aside, while I'd be delighted if glob assignment syntax for methods (*Some::Class::some_method = sub {...}) threw an exception (and only for methods, not changing existing behavior), I realize this may be complicated to enforce and would probably irritate enough developers who like quick 'n dirty that it may not be worth the trouble.