Skip to content

fix(#6640): take heap blocks only through the releasing scope - #6668

Open
yangjj-iso wants to merge 1 commit into
objectionary:masterfrom
yangjj-iso:6640
Open

fix(#6640): take heap blocks only through the releasing scope#6668
yangjj-iso wants to merge 1 commit into
objectionary:masterfrom
yangjj-iso:6640

Conversation

@yangjj-iso

Copy link
Copy Markdown

@yegor256 — this resolves the 6507-93ee2a46 puzzle in Heaps.

Closes #6640

What the puzzle asked for

Four steps, and they only work together: free cannot become private until nothing outside the class calls it, so the test moves and the visibility change have to land in one commit.

  • the negative-argument, size and resize tests in HeapsTest moved onto the scoped malloc;
  • both free probes in EOmallocEOofTest moved off free;
  • two-argument malloc and free are private;
  • failsOnClearingEmptyBlock is gone.

What changed

Heaps. The two-argument malloc handed out a block with nobody responsible for releasing it, and free let any caller release a block it did not own — so malloc(phi, size, scope) was a convention rather than a rule. Both are private now, which makes the scope the only way in and out. Nothing in src/main was affected: EOmalloc$EOof already goes through the scoped form, and it was the only production caller.

HeapsTest. Seven cases took a block raw and freed it by hand; they now run inside the scope. Two of them got shorter — returnsValidSize is a one-liner against Heaps.INSTANCE::size — and the ones that assert on an exception mid-scope keep the assertion inside the lambda so the block is still allocated when it runs. failsOnClearingEmptyBlock is deleted rather than adapted, because "freeing an unallocated block fails" is no longer a reachable state.

EOmallocEOofTest. Both probes asked "is it freed?" by calling free a second time and expecting a throw. They now ask size instead, which throws the same ExFailure for an unallocated block, so the assertion type is unchanged and the probe no longer needs a private method.

On the size of this one

185 hits, over the 40–100 the guidelines ask for, and I could not find an honest way to split it.

Roughly 56 of those are mechanical: qulice orders methods by visibility, so leaving malloc where it was reported MethodsOrderCheck against every package-private method below it. Moving the private pair to the end of the class fixes that but counts as a delete plus an add of the same body. The remaining change is the seven test conversions, which cannot be separated from the visibility change without leaving the tree uncompilable in between. Happy to reshape it if you would rather have it another way.

Verification

$ mvn -pl eo-runtime test -Dtest=HeapsTest,EOmallocEOofTest
Tests run: 25, Failures: 0, Errors: 0, Skipped: 0 -- in org.eolang.HeapsTest
Tests run:  4, Failures: 0, Errors: 0, Skipped: 0 -- in org.eolang.EOmallocEOofTest
BUILD SUCCESS

That run compiles the whole module, main and test, which is also what rules out a missed caller of the now-private methods.

mvn -Pqulice caught the ordering problem above; after the move it reports no violations on the changed file.

What I could not run here, and why

I am on Windows, and mvn clean install -Pqulice does not complete on this machine for two reasons that predate this branch. I checked both by stashing the change and re-running on unmodified master, which fails identically:

  • eo-parser fetches blns.txt from raw.githubusercontent.com through maven-antrun-plugin, which times out here (github.com and api.github.com are fine, that host is not). Worked around with -Dmaven.antrun.skip=true.
  • eo-runtime's project-validate step then fails with Missing: EOwin32$EOφ.class / Missing: EOposix$EOφ.class — "Not all .java files were compiled to .class files". Same failure with an empty working tree, so it is not from this change, but it does mean I have not run the full -Pqulice lifecycle end to end.

Separately, format reports 2 of 170 EO source(s) are not formatted canonically on a clean checkout. I left those alone — no .eo file is touched here — and used -Deo.autoFix locally to get past it.

…cope

Resolves the objectionary#6507 puzzle in Heaps. The two-argument malloc handed out a
block with nobody responsible for releasing it, and free let any caller
release a block it did not own, so the scoped malloc was a convention
rather than a rule.

Both are private now, which makes the scope the only way in and out. The
seven HeapsTest cases that took a block raw and freed it by hand moved onto
the scoped form, the two EOmallocEOofTest probes now ask size whether the
block survived instead of trying to free it a second time, and
failsOnClearingEmptyBlock is gone because no caller can reach free.

The private pair sits at the end of the class: qulice orders methods by
visibility, and leaving malloc where it was put every package-private
method after it out of order.

Closes objectionary#6640
Copilot AI lite review requested due to automatic review settings August 12, 2026 13:41
@sonarqubecloud

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown
Contributor

🚀 Performance Analysis

All benchmarks are within the acceptable range. No critical degradation detected (threshold is 100%). Please refer to the detailed report for more information.

Click to see the detailed report
Test Base Score PR Score Change % Change Unit Mode
benchmarks.XslBench.manySheetsOnLargeXmir 870.713 839.204 -31.509 -3.62% ms/op Average Time

✅ Performance gain: benchmarks.XslBench.manySheetsOnLargeXmir is faster by 31.509 ms/op (3.62%)

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR enforces scope-based ownership for heap blocks in eo-runtime by removing external access to raw allocation/free APIs, ensuring blocks can only be acquired through a releasing scope and making misuse (double-free / freeing чужой block) impossible by construction.

Changes:

  • Made raw malloc(phi, size) and free(id) private in Heaps, leaving malloc(phi, size, scope) as the only allocation/release path.
  • Refactored HeapsTest cases to allocate blocks through the scoped malloc, removing manual free calls and deleting the now-unreachable failsOnClearingEmptyBlock.
  • Updated EOmallocEOofTest to verify release via Heaps.size(id) (expecting failure) instead of probing with a second free.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
eo-runtime/src/main/java/org/eolang/Heaps.java Removes the puzzle/todo and makes raw allocation/free private to enforce scope-only heap ownership.
eo-runtime/src/test/java/org/eolang/HeapsTest.java Moves tests to scoped allocation, removes manual frees, and drops an unreachable free-on-empty test.
eo-runtime/src/test/java/org/eolang/EOmallocEOofTest.java Switches “freed?” probes from free to size to avoid relying on now-private APIs.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@yangjj-iso

Copy link
Copy Markdown
Author

runtime (23) is red here, but it is red on master too — including on e59be523, the commit this branch is based on:

run head runtime (23)
2026-08-12 13:16 38c14519 (master) failure
2026-08-12 12:15 e59be523 (master, this branch's base) failure
2026-08-12 09:31 20e15091 (master) failure

The failure is a Saxon error inside MjFormat.canonicalMjFormat.reformat, i.e. the EO source formatter, which also reports 2 of 170 EO source(s) are not formatted canonically on a clean checkout. This branch touches three Java files in eo-runtime and no .eo source, so it cannot reach that step.

Everything else is green, including the two checks that matter most for this change and that I could not complete locally on Windows: qulice and mvn (macos-15, 26).

@yegor256

Copy link
Copy Markdown
Member

@yangjj-iso

Copy link
Copy Markdown
Author

Fair — I was explaining the red, not asking you to look past it. A broken master is the reason not to merge, not a reason to merge anyway.

So I went after the break instead: #6672 fixes #6669. The template at inline-cactoos.xsl:314 hands @name — an attribute node — to nine functions that each declare $name as xs:string, leaving the atomization to the function conversion rules. Saxon may bind the parameter to a closure over the argument expression rather than over its converted value, so the node reaches a comparison that was already compiled to a ValueComparison on the strength of the declared types, and the cast to AtomicValue fails. Atomizing once in the template covers all nine call sites.

I will rebase this branch onto a green master once that lands, rather than ask for anything here in the meantime.

@yangjj-iso

Copy link
Copy Markdown
Author

Moving this to a draft while master is red. The post is explicit that you wait, and leaving it open asks for review time the build has not earned yet.

Sweeping every job rather than just the one I had looked at turned up two more reds on master, both now reported:

So #6672 on its own will not turn master green. I said otherwise earlier in this thread and that was too quick — I had checked the runtime workflow and not the others.

I will mark this ready again once master is green, rebased onto it.

@yangjj-iso
yangjj-iso marked this pull request as draft August 12, 2026 14:42
@yegor256

Copy link
Copy Markdown
Member

@yangjj-iso try to sync with master, it should work now

@yangjj-iso
yangjj-iso marked this pull request as ready for review August 12, 2026 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Negative-argument, size and related parameters not correctly handled in Heaps.java

3 participants