fix: jcpan -t Class::Trait PerlOnJava bugs + portable unit tests#638
Merged
fix: jcpan -t Class::Trait PerlOnJava bugs + portable unit tests#638
jcpan -t Class::Trait PerlOnJava bugs + portable unit tests#638Conversation
61493b5 to
0194432
Compare
jcpan -t Class::Traitjcpan -t Class::Trait PerlOnJava bugs + portable unit tests
1) Parser: `require File::Spec->catfile(...)` was parsed as
`(require File::Spec)->catfile(...)`, dispatching the method on the
bareword's require result (1) and producing
"Can't locate object method `catfile` via package `1`". Now: when the
bareword is followed by `->`, restore the position and parse the
whole thing as an expression (matches Perl semantics). Fixes 5 of 9
failing test files in Class::Trait.
2) bless after stash anonymisation: `RuntimeStash.undefine()`
anonymised the existing blessId AND left it in the cache, so any
subsequent `bless({}, "Pkg")` returned an object whose `ref` was
`__ANON__`. The Class::Trait test suite hits this in `clean_inc()`
which wipes packages with `undef *{Pkg::glob}` and then reloads them.
Now: drop the className->id cache entry on anonymise so that fresh
blesses allocate a new id mapped to the real class name (old objects
still see `__ANON__`, matching `undef %Pkg::` semantics).
Also: `undef *Pkg::` (RuntimeGlob.undefine on a `::`-suffixed name)
no longer anonymises at all — it just clears the stash slot. Perl
semantics: old blessed objects keep their package name; only
`undef %Pkg::` anonymises.
3) RuntimeHash each() iterator: clearing or reassigning a hash
(`%h = ()`, `undef %h`, hash `set` from a list) did not reset the
`hashIterator`. A subsequent `each %h` after re-populating threw
`ConcurrentModificationException`. Class::Trait's `apply()` uses
`_clear_all_caches()` (which assigns `()` to `%TRAITS_TO_CHECK`)
and then re-iterates via `each` — under PerlOnJava the second call
to `apply` after a previous croak silently iterated zero entries,
causing requirement checks to be skipped. Now: reset `hashIterator`
in all three clear-points.
4) RuntimeGlob.undefine() leaked DESTROY for ARRAY/HASH slots:
`undef *Pkg::arr` and `undef *Pkg::hsh` replaced the slot with a
fresh empty container without calling `.undefine()` on the old one,
so blessed values inside were orphaned without firing their
destructors. The SCALAR slot already went through `.set()` which
handled this. Now: call `.undefine()` on the old array / hash before
replacing. Covered by `unit/refcount/destroy_glob_undef.t` (added in
this commit, fails before this fix and passes after).
Result on `./jcpan -t Class::Trait`:
before: 9/17 test files failed, 8/219 subtests failed
after: 2/17 test files failed, 1/405 subtests failed
The remaining failures are unrelated: `t/pod_coverage.t` needs
`B::GV::GvFLAGS` (a B-module feature not yet ported), and
`t/070_Trait_mod_perl_test.t` has one warning-leak subtest under prove.
Generated with [Devin](https://devin.ai)
Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Running `prove -r src/test/resources/unit/` previously failed 14 of 210
test files. Investigation showed each was relying on a PerlOnJava-permissive
behavior, a misuse of Perl idioms, or an XS dependency without a skip guard.
The tests are now portable: they pass under `./jperl` (via `make`, all 5
shards green) AND under system `prove` (209/209 files, 7820/7820 tests).
Per-file summary:
* statement.t — `state $counter = 0 unless defined $counter` doesn't
compile under strict in real Perl (the modifier-side `defined $counter`
references the variable before `state` declares it). Rewritten to
declare `state $counter` and conditionally initialise on the next line.
* interpreter_myhash_myarray_scope_exit.t — `defined((my %copy=%$arg)
? $copy{key} : undef)` is rejected by strict in real Perl: the inner
`$copy{key}` reference is parsed before `my %copy` becomes visible.
Hoisted `my %copy = %$arg` to its own statement; the test's intent
(interpreter scope-exit on the my-hash path) is preserved.
* netssleay_*.t (7 files) — wrapped `use Net::SSLeay` in a BEGIN-time
eval guard with `Test::More::plan(skip_all => ...)` when the module
isn't installed. Net::SSLeay is bundled with PerlOnJava but is an XS
CPAN module that may not be present on a system perl, so prove was
bailing on `Can't locate Net/SSLeay.pm`.
* encoding_pragma.t — tests PerlOnJava's no-op `encoding.pm` stub.
Real CPAN `encoding.pm` dies on bare `use encoding;` (and is removed
from core since 5.26). The test now detects "running under system
perl" by the absence of `jar:` entries in @inc, walks up cwd to find
`src/main/perl/lib/encoding.pm`, and prepends it to @inc before
loading. Under `./jperl`, the bundled stub is already used. Also
hoisted `use utf8 / use Encode` to top-level — embedding them inside
an eval STRING triggered a DynaLoader bootstrap_inherit failure on
system perl.
* source_filter_scope.t — Filter::Util::Call's `filter_read` reads from
the parser's source FILE; an `eval STRING` is not filterable in real
Perl and the filter sees EOF immediately. Rewritten to write a real
`.pm` to a tempdir and `require` it (which is the actual Spiffy
pattern this test was meant to cover).
* weaken.t (test 4) and destroy.t (12 tests) — both used the pattern
`my @log; { package P; sub DESTROY { push @log, ... } }` inside a
subtest. Real Perl warns "Variable @log will not stay shared": named
subs in named packages don't bind to the per-invocation `my` scope of
the enclosing closure, so `@log` inside DESTROY is the *original*
(uninitialised) capture. Rewritten to use `@PkgName::log` package
globals which gives the same observable semantics deterministically.
destroy.t also needed `use warnings` for the `(in cleanup)` warning
to fire through `$SIG{__WARN__}`.
* overload/constant.t — used PerlOnJava's `$^H{integer} = sub {...}`
shortcut. The standard real-Perl API is `overload::constant integer
=> sub {...}`; PerlOnJava already supports this form. Also relaxed
the "handler args" assertion to test only the stable text + numeric-
value pair (real Perl 5.42 passes `undef` as the third "category"
arg for integer literals; PerlOnJava passes "integer"). bigint
smoke-test wrapped in a SKIP guard for portability.
Verification:
./gradlew classes testUnitParallel --parallel shadowJar → BUILD SUCCESSFUL
prove -r src/test/resources/unit/ → 209/209 PASS
Generated with [Devin](https://devin.ai)
Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
0347995 to
ad35e21
Compare
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Two related commits:
Commit 1 — Four PerlOnJava bugs exposed by
jcpan -t Class::TraitParser:
require File::Spec->catfile(...)was parsed as(require File::Spec)->catfile(...), dispatching the method on the bareword's require result (1) and producingCan't locate object method "catfile" via package "1". Now: when arequire-bareword is followed by->, restore the position and parse the whole thing as an expression (matches Perl semantics). Fixes 5 of 9 failing test files in Class::Trait.blessafter stash anonymisation:RuntimeStash.undefine()anonymised the existing blessId AND left it in the cache, so any subsequentbless({}, "Pkg")returned an object whoserefwas__ANON__. The Class::Trait test suite hits this inclean_inc(). Now: drop the className→id cache entry on anonymise so fresh blesses allocate a new id mapped to the real class name. Also:undef *Pkg::(RuntimeGlob.undefine on a::-suffixed name) no longer anonymises at all — Perl semantics: onlyundef %Pkg::anonymises.RuntimeHasheach()iterator: clearing or reassigning a hash (%h = (),undef %h, hashsetfrom a list) did not reset thehashIterator. A subsequenteach %hafter re-populating threwConcurrentModificationException. Class::Trait'sapply()uses_clear_all_caches()and then re-iterates viaeach— under PerlOnJava the second call after a previous croak silently iterated zero entries, causing requirement checks to be skipped. Now: resethashIteratorin all three clear-points.RuntimeGlob.undefine()leaked DESTROY for ARRAY/HASH slots:undef *Pkg::arrandundef *Pkg::hshreplaced the slot with a fresh empty container without calling.undefine()on the old one, so blessed values inside were orphaned without firing destructors. Now: call.undefine()on the old array / hash before replacing. Covered byunit/refcount/destroy_glob_undef.t(added in this commit, fails before this fix and passes after, also passes under systemprove).Result on
./jcpan -t Class::TraitRemaining failures are unrelated:
t/pod_coverage.tneedsB::GV::GvFLAGS,t/070_Trait_mod_perl_test.thas one warning-leak subtest under prove.Commit 2 — All unit tests now pass under system perl
Running
prove -r src/test/resources/unit/previously failed 14 of 210 test files (each relying on PerlOnJava-permissive behavior, a misuse of Perl idioms, or an XS dependency without a skip guard). All now pass under both./jperl(viamake) and systemprove:statement.tstate $x = 0 unless defined $x— strict rejects (refers to $x before declaration). Rewrote to declarestate $xthen conditionally init.interpreter_myhash_myarray_scope_exit.tdefined((my %copy=$h) ? $copy{k} : ...)—my %copynot visible inside same expression under strict. Hoisted declaration.netssleay_*.tNet::SSLeaynot always installed; addedTest::More::plan(skip_all)guard.encoding_pragma.tencoding.pmstub; CPAN's dies onuse encoding;. Detect system perl by absence ofjar:in @inc and force-load our stub. Hoisteduse Encodeto top to avoid DynaLoader bootstrap failure under eval STRING.source_filter_scope.tfilter_readdoesn't operate on eval STRING (sees EOF immediately). Rewrote to write a real.pmto tempdir +require.weaken.t,destroy.tmy @log; sub DESTROY { push @log, ... }triggers "Variable @log will not stay shared" — named subs in named packages don't bind to per-invocationmyscope. Switched to@PkgName::logpackage globals.destroy.talso neededuse warningsfor(in cleanup)warnings to fire through$SIG{__WARN__}.overload/constant.t$^H{integer} = sub{}shortcut; switched to standardoverload::constant integer => sub{}(PerlOnJava already supports it). Relaxed handler-args assertion (real Perl 5.42 passesundefas category arg vs PerlOnJava's"integer"). bigint smoke wrapped in SKIP guard.Test plan
make(full unit suite, jperl, 5 parallel shards) → BUILD SUCCESSFULprove -r src/test/resources/unit/(system perl) → 209/209 files, 7820/7820 tests PASSunit/refcount/destroy_glob_undef.tfails before fix variable declaration in if expression causes bytecode inconsistency #4, passes after, also passes under system perl./jcpan -t Class::Traitimproved from 9 to 2 failing test filesGenerated with Devin