Perl / perl5

🐪 The Perl programming language
https://dev.perl.org/perl5/
Other
1.85k stars 527 forks source link

report an error when making an object of an incomplete class #22166

Closed tonycoz closed 3 weeks ago

tonycoz commented 3 weeks ago

instead of asserting or crashing

Fixes #22159

xenu commented 3 weeks ago

Why are incomplete classes even a thing? Shouldn't the creation of the class be aborted on syntax error?

tonycoz commented 3 weeks ago

Force push moved the message tests from lib/warnings.t where I originally had them to t/lib/croak.t since they are fatal.

Why are incomplete classes even a thing? Shouldn't the creation of the class be aborted on syntax error?

That's a more complex change than I would want at this point in the dev cycle.

I'll have a look at it this afternoon, though I found a less painful class issue to work on first. We'd still need the croak for the BEGIN{} test case.

jkeenan commented 3 weeks ago

In the course of working on https://github.com/Perl/perl5/issues/22159, I had occasion to update my CPAN module Devel::Git::MultiBisect. For that purpose I needed to transform the one-liner provided in that ticket by @tonycoz into a test file.

$ cat /tmp/gh-22159-class.t
#!./perl

BEGIN {
    chdir 't' if -d 't';
    require './test.pl';
    set_up_inc('../lib');
    require Config;
}

use v5.36;
use feature 'class';
no warnings 'experimental::class';

# https://github.com/Perl/perl5/issues/22159
{
    #'eval "class Foo {"; Foo->new'
    local $@;
    eval {
        eval "class Foo {"; Foo->new;
    };
    ok( ! $@, "No exception" ) or diag($@);
}

done_testing;

I then ran this through Tony's branch, first on a plain build, then on a debugging build.

$ git checkout gh-22166-tonycoz-22159-incomplete-class-20240422
$ regular_configure && make minitest_prep
$ ./miniperl -Ilib /tmp/gh-22159-class.t 
not ok 1 - No exception
# Failed test 1 - No exception at /tmp/gh-22159-class.t line 22
# Cannot create an object of incomplete class "Foo" at /tmp/gh-22159-class.t line 20.
1..1

$ git clean -dfxq

$ sh ./Configure -des -Dusedevel -DDEBUGGING && make minitest_prep

$ ./miniperl -Ilib /tmp/gh-22159-class.t 
not ok 1 - No exception
# Failed test 1 - No exception at /tmp/gh-22159-class.t line 22
# Cannot create an object of incomplete class "Foo" at /tmp/gh-22159-class.t line 20.
1..1

LGTM; I also did a debugging build on FreeBSD-13 and ran the full test suite: PASS.

leonerd commented 3 weeks ago

Why are incomplete classes even a thing? Shouldn't the creation of the class be aborted on syntax error?

@xenu Possibly, but detecting that and rolling it back is really difficult. The class-enabled package gets created at the point of the class NAME keyword, but it isn't until the end of the block that it gets finished off to the point that a constructor can be invoked. These failures are what happens if you try to invoke a constructor after the former but before the latter.

tonycoz commented 3 weeks ago

Why are incomplete classes even a thing? Shouldn't the creation of the class be aborted on syntax error?

@xenu Possibly, but detecting that and rolling it back is really difficult. The class-enabled package gets created at the point of the class NAME keyword, but it isn't until the end of the block that it gets finished off to the point that a constructor can be invoked. These failures are what happens if you try to invoke a constructor after the former but before the latter.

The rollback is pretty painful, but I do think it's necessary.

This one of the few cases where if you do see such a compilation error you can't recover and supply a new correct definition.

For legacy classes you can simply supply the new class definition and you will have successfully defined the class, with class keyword classes, you cannot do that. You might see some redefinition warnings, but you can do the replacement.

Of course, this isn't only a problem for redefinition after error, it's also a problem with redefinition after success, which perl also generally allows.