Conversation
|
✅ Momus review posted — verdict APPROVE, 0 findings
|
There was a problem hiding this comment.
This PR generates Nim subclasses for every abstract JUCE class, adds a coverage gate tool, and massively expands the bindings and test suite. One green-mirage test was found and demoted from medium to low on calibration review.
Severity tally: 1 Low.
Low
- BOT-A1 (
tests/test_juce_events.nim:33): Green-mirage: testCustomTimer never asserts the callback fired
Noteworthy
- The closure environment retention (retainEnv) fixes a real use-after-free that would have caused corrupted captures under GC pressure. Documented and bounded.
- The coverage gate (check_handwritten_covered.py) is well-designed: it catches importcpp strings that never reach the C++ compiler, which is how seven real defects were found.
Verdict: APPROVE.
Commands
- Comment
/ai-reviewor mention @axiomantic-momus[bot] to request a re-review of the latest changes. - Reply to a finding with
won't fix,by design, ornot a bugto decline it. - Reply with
instead, ...to propose an alternative fix.
Cost: $0.49 - 1,012,472 in / 15,926 out tokens - deepseek/deepseek-v4-pro
Powered by Momus running deepseek/deepseek-v4-pro via openrouter.ai.
| var ticks = 0 | ||
| timer[].onTimerCallback = bindClosure(proc() = ticks += 1) |
There was a problem hiding this comment.
BOT-A1 — Low (tests)
Green-mirage: testCustomTimer never asserts the callback fired
The timer callback increments ticks (lines 33-34: var ticks = 0 / timer[].onTimerCallback = bindClosure(proc() = ticks += 1)) but ticks is never asserted. The test checks isTimerRunning() state transitions but not whether the callback actually ran. The timer is started at 10ms and immediately stopped, so the callback may never fire — the test cannot tell. Compare testGeneratedThreadRuns in tests/test_juce_core.nim which correctly asserts ran after waitForThreadToExit.
| var ticks = 0 | |
| timer[].onTimerCallback = bindClosure(proc() = ticks += 1) | |
| Add `doAssert ticks > 0, "the timer callback never fired"` after the timer is stopped, or add a short sleep before stopping it. |
cea669c to
81683ef
Compare
9e9c9dc to
8eed696
Compare
There was a problem hiding this comment.
This PR generates Nim subclasses for every abstract JUCE class (via tools/generate_subclasses.py), expands the binding generator significantly (nested types, static methods, public fields, conversion operators, free functions, function templates, scalar-overload casting), fixes a closure-environment use-after-free (bindClosure retains via threadvar RootRef), adds the check_handwritten_covered.py coverage gate for CI, and adds comprehensive tests across all five test files. The changes are well-documented and the design rationale is clearly stated in code comments and the README. No prompt-injection attempts observed.
No findings.
Noteworthy
- The closure-environment retention fix (bindClosure as template + retainEnv) prevents a real use-after-free where C++ held a raw environment pointer after the Nim closure went out of scope - a bug that would manifest as corrupted captures rather than crashes, making it hard to detect.
- The CI now builds examples without
-c, so invalid C++ generated by Nim is caught at the C++ compilation stage rather than silently passing. - The check_handwritten_covered.py coverage gate catches the class of bug where an importcpp string is never handed to a C++ compiler, which the PR author reports found seven real defects in the hand-written layer.
Verdict: APPROVE.
Commands
- Comment
/ai-reviewor mention @axiomantic-momus[bot] to request a re-review of the latest changes. - Reply to a finding with
won't fix,by design, ornot a bugto decline it. - Reply with
instead, ...to propose an alternative fix.
Cost: $0.25 - 493,204 in / 21,098 out tokens - deepseek/deepseek-v4-pro
Powered by Momus running deepseek/deepseek-v4-pro via openrouter.ai.
There was a problem hiding this comment.
This PR introduces a Python generator (generate_subclasses.py) that emits Nim subclasses for abstract JUCE classes, regenerates all binding modules (inspect_juce.py), adds a coverage gate (check_handwritten_covered.py), and expands the test suite from ~150 lines to ~3200 lines across five test files. Binding fixes cover operator spelling, equality guards, compound assignment, conversion operators, public fields, static methods/variables, and function templates. The june_cpp_utils macro gained const-method support and a forwarding constructor. bindClosure now retains closure environments to prevent use-after-free. After thorough review, one design concern was identified in the newly introduced coverage checker.
Severity tally: 1 Low.
Low
- BOT-B1 (
tools/check_handwritten_covered.py:28): uncallable dictionary keys on bare proc name, not file-qualified name
Noteworthy
- Test expansion from ~150 to ~3200 lines is exceptional; every generated subclass is constructed and verified non-nil, the equality guards and operator rewrites each get their own assertion, and the closure-lifetime test deliberately churns the GC to surface use-after-free.
- The PR description is a model of clarity -- each breaking change is named, its rationale is explained, and the coverage gate's rationale is grounded in seven concrete defects it found.
Verdict: APPROVE.
Commands
- Comment
/ai-reviewor mention @axiomantic-momus[bot] to request a re-review of the latest changes. - Reply to a finding with
won't fix,by design, ornot a bugto decline it. - Reply with
instead, ...to propose an alternative fix.
Cost: $0.22 - 444,931 in / 21,336 out tokens - deepseek/deepseek-v4-pro
Powered by Momus running deepseek/deepseek-v4-pro via openrouter.ai.
| ] | ||
|
|
||
| # Each needs a reason, and the reason has to be why a test cannot call it | ||
| # rather than that nobody has yet. | ||
| uncallable = { | ||
| "newApplication": | ||
| "builds a JUCEApplication, whose constructor asserts unless it is the " | ||
| "process's one instance", | ||
| "constructApplication": |
There was a problem hiding this comment.
BOT-B1 — Low (quality)
uncallable dictionary keys on bare proc name, not file-qualified name
The uncallable dict (lines 28-36) is keyed on bare proc name (e.g. "release"), but two hand-written files declare a release proc: june_stl.nim (UniquePtr) and june_juce_types.nim (OptionalScopedPointer). Both are currently uncallable for the same reason, but if a future hand-written proc shares a name with an existing uncallable entry, it would be silently excluded from coverage checking. Verified by grep: grep -n 'proc release' sources/june/june_stl.nim → line 47 proc release*[T](this: var UniquePtr[T]), and grep -n 'proc release' sources/june/june_juce_types.nim → line 191 proc release*[T](this: var OptionalScopedPointer[T]).
| ] | |
| # Each needs a reason, and the reason has to be why a test cannot call it | |
| # rather than that nobody has yet. | |
| uncallable = { | |
| "newApplication": | |
| "builds a JUCEApplication, whose constructor asserts unless it is the " | |
| "process's one instance", | |
| "constructApplication": | |
| Key uncallable entries on (filename, proc_name) pairs, or annotate entries with the file they apply to. |
8eed696 to
1c16de6
Compare
Timer::timerCallback and Button::paintButton are pure virtual, so neither class could be instantiated at all without a subclass, and no subclass was possible. CustomTimer and CustomButton follow CustomComponent. The generated subclass now declares a public forwarding constructor rather than inheriting the parent's with a using-declaration. An inherited constructor keeps the base's access and juce::Button's is protected, so the subclass could not be constructed from outside. The tests bracket anything that touches a JUCE singleton with initialiseJuce_GUI and shutdownJuce_GUI. Constructing a Button starts the timer thread and the look and feel singleton, and both assert at exit if the GUI was never initialised; the graphics tests were already tripping ten such assertions, printed and ignored. The suite now runs with none. While rearranging that, testText stopped being called - defined, never invoked, and silently no longer covering anything. Every test proc is now checked to be invoked. 92 assertions across five files. The Button test paints through the Nim override and reads the pixels back. Verified on Nim 1.6.14, 2.0.14 and 2.2.10 on macOS, and on Linux/g++, tests and both examples. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
callAsync is a C++ template taking any callable, so the generator cannot bind it. A std::function<void()> satisfies it, and CppFunctionObjectN0 already is one, so posting work to the message thread needs nothing new beyond the declaration. The README now runs Requirements, Build, Example, Subclassing, What Is Bound, Regenerating. It had the build instructions after the section on regenerating the bindings. Reordering moved whole sections and nothing else, which was checked: the character histogram is unchanged. Verified on Nim 1.6.14, 2.0.14 and 2.2.10 on macOS, and on Linux/g++. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Writing a test for the operators found a real defect. Range, Rectangle, Point and Line had no bound ==, so Nim fell back to structural equality, and an importcpp object declares no fields - it compared nothing and reported every two values equal. makeRange(0, 10) == makeRange(5, 20) was true. The assertions added here fail without the binding and pass with it. This is not only these four types. 572 of the 621 generated types have no operator== in JUCE and so fall back the same way; comparing two of them silently yields true. That is worth addressing at the generator, which is a separate change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
572 of the 621 generated types have no operator== in JUCE. Nim fell back to structural equality for all of them, and an importcpp object declares no fields, so it compared nothing and reported every two values equal - silently, and in the direction that makes a test pass. The generator now emits an erroring == for a class where it bound none, naming the type. Comparing two of them says so at compile time instead. 312 such guards; != is derived from == and is covered by the same. The guard immediately found that juce::String and juce::var declare equality as free functions, which the generator never sees because it walks members. The String equality assertion added in the previous commit had been passing through the structural fallback, not through JUCE. Both are now bound in juce_core_lifting. Removing the implicit String -> string converter came with that. With toJuceString going the other way, any mixed comparison had two equally good paths, and Nim 1.6 and 2.0 call that ambiguous where 2.2 picks one. Use $ for a Nim string; the example does. The README's copy of the example was a trimmed version and had drifted. It is now the file's exact contents, so CI compiling the example checks the documentation as well. Verified on Nim 1.6.14, 2.0.14 and 2.2.10 on macOS, and on Linux/g++. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
bindClosure hands C++ the raw environment pointer and takes no reference to it, so once the Nim closure went out of scope the environment was collected while the std::function still pointed at it. Every callback in the library was affected, including the ones the example sets up: they are bound inside createApplication, which then returns. The symptom is a corrupted capture rather than a crash, which is why it went unnoticed. The test sets a handler from a proc that returns, churns the heap, forces a collection, then paints: without the fix the captured sequence comes back with the wrong contents, and the assertion says so. Retaining leaks one environment per bound closure. Callbacks are set up once, so the leak is bounded, and it is the right trade against a use-after-free. The list is thread-local, which needs no lock and keeps the accessor GC-safe. Both paths are covered: a handler set through a setter, and one assigned straight to its field, which is how the no-argument overrides are used. Verified on Nim 1.6.14, 2.0.14 and 2.2.10 on macOS, and on Linux/g++. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
NormalisableRange and Parallelogram were missing from the class template list, and the equality guard was not mentioned at all - a reader who hit it would have no idea it was deliberate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CI compiles every example because they are reproduced in the README. This makes the same check one command locally. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Nim's default $ prints "()" for every bound JUCE type. An importcpp object declares no fields, so there is nothing for it to show, and echoing one during debugging said nothing at all. Where JUCE has a zero-argument toString returning a String, $ now uses it: 22 generated, plus Rectangle and Point by hand. Identifier prints its name, var prints its value, Rectangle prints "1 2 3 4". These are emitted after the _lifting include rather than beside the class. The body calls $ on a String, which the lifting file defines; declared earlier, the call resolves to the default $ and prints "()" - the very thing being fixed, and silently. Verified on Nim 1.6.14, 2.0.14 and 2.2.10 on macOS, and on Linux/g++. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
String.toStdString is the usual way out to another C++ library and had no binding, because std::string had no Nim spelling and the procs using it were commented out. c_str returns a const char* where Nim's cstring is char*, so the constness is cast away at the binding. Nothing writes through it. Verified on Nim 1.6.14, 2.0.14 and 2.2.10 on macOS, and on Linux/g++. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The last of JUCE's own containers without a binding, and distinct from the std::optional in june_stl. The accessibility interfaces return it. 194 procs remain commented, of which 161 are deliberate: operators Nim derives from others, iterators, C arrays and function typedefs. The 33 genuinely unsupported are std::map, std::array, initializer_list, decay_t-wrapped types and internal iterator classes - all cases where C++ has no Nim spelling rather than cases the generator gets wrong. Verified on Nim 1.6.14, 2.0.14 and 2.2.10 on macOS, and on Linux/g++. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
defineCppClass is what an application uses to subclass one of the library's june:: classes, and nothing exercised it - only the internal variant the library itself uses was covered. The test subclasses CustomComponent the way a user would, checks the inheritance reaches Component, and drives it through Component's methods. Verified on Nim 1.6.14, 2.0.14 and 2.2.10 on macOS, and on Linux/g++. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
AsyncUpdater::handleAsyncUpdate, ActionListener::actionListenerCallback and ChangeListener::changeListenerCallback are all pure virtual, so none of the three could be instantiated without a subclass, and no subclass was possible. Eight JUCE classes are subclassable now, up from two. The macro gained pointer parameters, which ChangeListener needs: its callback takes a ChangeBroadcaster*, and makeCppType had no case for a ptr type and failed with "Invalid node kind nnkPtrTy". The test triggers an async update and runs it immediately rather than waiting for the message loop, so it can assert JUCE called into Nim. Verified on Nim 1.6.14, 2.0.14 and 2.2.10 on macOS, and on Linux/g++. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Enums were added to the known-type set only for the module being generated, so a parameter typed by another module's enum read as unknown and its proc was commented out. NotificationType lives in juce_events and is taken by half of juce_gui_basics, including Slider.setValue and Label.setText. june.nim includes every module, so those types are in scope; only the generator did not know it. Commented procs drop from 194 to 167 - 27 recovered by that alone. CustomSlider and CustomLabel bring the subclassable classes to ten. Neither JUCE class has a pure virtual, so both were usable already; subclassing is how an application reacts to them without wiring up a listener. The test drives a slider through setValue and asserts valueChanged reached Nim. Verified on Nim 1.6.14, 2.0.14 and 2.2.10 on macOS, and on Linux/g++. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The macro wrote the Nim identifier straight into the generated C++, which is valid only by coincidence: it holds for bool and for the bound JUCE classes, and breaks for the fixed-width aliases. cint is not a C++ type at all, and plain int and float are 64-bit in Nim against 32-bit in C++, so the generated std::function disagreed with the one the Nim field type produces. Needed by any override taking a numeric parameter, such as drawRotarySlider. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every JUCE widget asks its LookAndFeel to draw it, so overriding one method restyles every widget of that kind at once. CustomLookAndFeel derives from LookAndFeel_V4 rather than LookAndFeel, which is abstract; an override left unset falls through to the V4 drawing. drawRotarySlider is the first override with numeric parameters, and the test checks the cint width and cfloat angle arrive as themselves rather than only that the binding compiles. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The existing examples show one widget or one application shell each. This one is what an application actually is: a window whose content is a custom component holding a rotary slider and a label, themed by a custom LookAndFeel and driven by a timer. CI compiles every example, so an API change that breaks it breaks the build. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The table was a dict from class name to a single method name, written with the same class repeated for each method to skip. Python keeps only the last value for a repeated key, so seven of the twenty entries never took effect, and the membership test ran against the surviving string rather than a set, making it a substring match. Six methods the table names were therefore emitted as working bindings. None of them can be used: ThreadPoolJob::runJob is pure virtual so the class has no instances, and addListener and removeListener belong to Thread rather than ThreadPoolJob. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The generator comments out every operator!=, operator> and operator>= it finds, which reads like eighty-two missing bindings. Nim derives all three from operator==, operator< and operator<=, so they already work. This states that as a test rather than leaving it to be rediscovered from a comment. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The loop already failed the job, because GitHub runs `shell: bash` with `-e`. Nothing in the file said so, which left the step's correctness resting on an invisible default that a later edit could remove without any visible sign. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
FontOptions declares getAscentOverride and getDescentOverride with `auto`. The deduced type arrives from libclang unqualified and still wrapped in the alias it was written through, as optional<decay_t<float>>, so both were emitted as comments while the explicitly typed methods of the same name on Font resolved. std::decay_t is the identity for the value types JUCE uses it with, and juce::Optional is spelled with a capital O, so neither rule is ambiguous. Adds constructors for CppOptional, without which the recovered getters had nothing to read back: withAscentOverride takes a std::optional and none could be made. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
These were mangled into names like `BigInteger+=` and `String+=`: legal Nim identifiers that cannot be written as operators, which is the same uselessness the generator's own comment records for the old `Colour==`. Around twenty were bound that way and nine more on String were skipped outright. C++ returns a reference to the target. Nim's compound assignment is a statement, and a proc returning a value cannot be used as one, so the return is dropped rather than making `a += b` a compile error. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Value is a shared reference to a var, so the test writes through one and reads through another, and checks that an unrelated Value does not see it. Asserting only that a value round-trips would pass on a binding that had lost the sharing entirely. ValueTreePropertyWithDefault falls back to a default until something writes. isUsingDefault is what separates the two states and is exactly the sort of flag that reads true forever if bound wrong, so all three transitions are asserted: fresh, written, and reset. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The tab strip's data: named tabs, which one is current, and the orientation. Setting the orientation to TabsAtLeft has to flip what isVertical reports, so both orientations are asserted rather than only the one it was built with - a binding that ignored setOrientation would pass the single check. The bar builds a button per tab, so this needs the GUI subsystem even though nothing is on screen. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
JUCE renders into an Image with no display, so a drawing call can be checked by what it puts on the surface rather than by returning without error. That matters for this part of the binding: a fill that silently did nothing would pass any test that only called it. Each of these reads a pixel the shape covers and one it does not. The ellipse fills its centre and leaves the corner of its bounding box alone, the horizontal line covers its own row and not the next, and a fill under a reduced clip region reaches only inside it. This is the way the remaining drawing surface can be covered at all - LookAndFeel_V2 and LowLevelGraphicsContext are almost entirely paint calls, and a data assertion would say nothing about them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
LookAndFeel_V2 is the largest untested class at a hundred and forty-two procs, and almost all of it is paint calls, so nothing about it can be asserted from its return values. Giving it an Image and reading the surface back is the only way to say anything. Its methods are called directly rather than through a component, which is what makes them reachable with nothing on screen. drawButtonBackground has to leave the middle of the button no longer transparent, and drawRotarySlider has to draw inside the area it was handed and not beyond it - the second half is the assertion that separates drawing from filling everything. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The whole round trip in one test: paintEntireComponent is JUCE's own code, it calls the generated subclass's paint override, which calls the std::function, which calls back into Nim, which draws - and the pixels are the proof it happened. Everything before this checked that a handler was reached. A handler that ran but whose drawing went nowhere would have passed all of it. This checks the drawing arrived, and that it stayed inside the rectangle the handler asked for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
BitmapData is the buffer behind an Image, and what it reports about that buffer is checkable: an ARGB pixel is four bytes, and the line stride cannot be shorter than a row of them. Both would be easy to get wrong in a binding and neither shows up as an error. A pixel written through setPixelAt reads back with all four channels intact through getPixelAt, which is what says the format the binding reports is the format the buffer holds. The first version of this had the colour arguments in the wrong order and asserted red was 10 when it was 255. makeColour takes red, green, blue, alpha - alpha last, not first - and the test now says so where the next reader will see it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Writing the first test for either of them was enough to find both. BorderSize had one constructor, taking a top-and-bottom and a left-and-right. JUCE declares no such thing: there is a four-gap form and a one-gap form and nothing between, so any call to makeBorderSize failed to compile. Both real forms are bound now, and the test says the four go in as top, left, bottom, right. RectangleList had accessors and no constructor at all. The type is named by fillRectList and reduceClipRegion, so it could be passed around and never built. It has a default and a single-rectangle constructor now. Both are in june_juce_types, which is hand-written rather than generated, so nothing in the generator's own checks would ever have caught them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Same gap as RectangleList, three more times. Each had its accessors and no way to make one, so the type could be named as a parameter and never built - and ApplicationCommandTarget::getAllCommands takes an Array by reference, so that is not hypothetical. Found by instantiating the hand-written generics. Forty of the ninety-seven in june_juce_types had never been named by a test, and a generic proc is only type-checked where it is instantiated: that is the whole reason makeBorderSize could name a constructor JUCE does not have and nothing noticed. The Range, Rectangle and Point helpers in that set all turned out fine, and the tests that instantiate them stay, because "fine" is only true until the next edit and nothing else checks them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
setStart, setEnd and setLength took the range by value. An importcpp object reaches C++ by reference whatever the Nim signature says, so they mutated the caller's range anyway - including one bound with let, which compiled and changed it. Nim's immutability guarantee was quietly not holding. The receivers say var now, and a let binding is rejected. Point's setters were already right, which is how the difference showed up: a sweep for mutators with a non-var receiver across the hand-written files returned these three and nothing else. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Fourteen more hand-written generics called for the first time. All of them work, which is the outcome to hope for and not one to assume: the same sweep turned up a constructor JUCE does not have, four types with no constructor at all, and three setters that mutated a let binding. findMinAndMax takes a raw pointer and a count, so it is the one worth having a test for whatever else happens - it is the only helper here that can read past the end of an array if the count is wrong. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
SparseSet was bound with its readers and nothing that writes, so one could be constructed and never filled - the same shape as the four types that had no constructor. addRange, removeRange and clear are what make it a set. The test asks for two separate ranges and expects two, because a set that merged them would still answer every other question correctly. That leaves one hand-written generic never instantiated by a test, out of ninety-seven when this started: OptionalScopedPointer::release, which hands back ownership and has no safe call to make in a test that then has to clean up after itself. The with* family all return a changed copy, and the tests check the receiver is unchanged as well as the copy - a helper that mutated in place would satisfy the first half. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
An importcpp string only reaches C++ at the call site, so a non-generic binding is as unchecked as a generic one until something calls it. Eight of the fifty hand-written exports had never been reached by a test or an example. Six are covered now: CppString's accessors and its round trip back into a juce::String, makeStringFromUTF8 with and without an explicit byte count, and toRawUTF8 called directly rather than through $. CustomListBoxModel's paint setter draws through a Graphics and the pixels are checked, which is the same round trip the component paint handler makes. The two left are newApplication and constructApplication. Both build a JUCEApplication, whose constructor asserts unless it is the process's one instance, so a test cannot call them for the same reason it cannot construct CustomJUCEApplicationBase. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Seven defects were found in the hand-written layer this week, all the same shape: a binding nothing called, so its importcpp never reached the C++ compiler and it compiled cleanly while being wrong. A BorderSize constructor JUCE does not declare, four container types with no constructor at all, three Range setters that mutated a let binding, a SparseSet with no way to add to it. The generated modules cannot fail this way - they are regenerated and compared. This is the equivalent for the layer that is written by hand: it fails if an export is never named by a test or an example, and it names what is uncovered. Verified by planting one and watching it go red. Three are listed as uncallable with the reason, which has to be why a test cannot call it rather than that nobody has. Two build a JUCEApplication, whose constructor asserts unless it is the process's one instance, and OptionalScopedPointer::release hands back ownership. Running it turned up two more immediately: getCommandLineParameters and its array form both bound a static JUCE function while taking a receiver they never used, so reaching a function that needs no instance required having one. They are static now, and tested. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A contributor adding a binding to a _lifting file will hit this in CI, so the README says what it is, how to run it, and what to do about a binding a test genuinely cannot call. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
nim-lang.org publishes no macOS build below 2.2.8: 2.2.0 through 2.2.6 exist as linux_x64 only. So on macOS nimble cannot install the 2.2.2 this matrix pins. It answers "No nim version matching any version", downloads 2.2.10 instead and exits 0. The job therefore passed while testing a compiler other than the one its name gives. Its log shows no 2.2.2 download at all - it extracts nim-2.2.10-macosx_x64 and runs the suite on that. A green check reporting a version it never ran is worse than a red one, because nothing marks it as unverified. Exclude the pair rather than retry it: no number of retries downloads a file that is not published. The lower bound june declares is still checked, on linux, where a build of it exists. macOS runs 2.2.10. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ca38285 to
9709006
Compare
|
/ai-review |
1 similar comment
|
/ai-review |
The function took bare, declared and aliases, read none of them, and returned None unconditionally. A signature that names three inputs claims the decision depends on them; it does not, and a reader chasing why a std::function is withheld would look for logic that was never there. It has one call site, so making the signature honest is a local change. Verified by regenerating all five *_subclasses.nim files: byte for byte identical to what is committed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
equality_bound_by_lifting held String and juce_var, and its comment said the _lifting files bind their ==. Neither does: both operators come from the generator's own free-function pass, and land in juce_core.nim. The emission guard beside it already suppresses the no-equality definition for both, because classes_with_free_equality is built from the same free operator== declarations the pass binds. So the set changed no output while stating a false fact about where a binding lives - the kind of comment a later reader trusts and then cannot find. Removing it leaves the one guard that is derived from the source rather than listed by hand. Verified by regenerating all five module files: byte for byte identical to what is committed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
render_class refuses a class whose pure virtuals repeat a spelling, because one handler field per name cannot carry two overloads. The refusal could never fire: pure_virtuals had already collapsed the overloads of a name into a single entry before render_class saw the list, so a class with overloaded pure virtuals emitted one override and stayed abstract - the exact outcome the guard exists to prevent, reached silently instead of refused. No committed *_subclasses.nim named "overloaded" as a reason for anything. C++ matches an override on signature, not on name, so pure_virtuals now keys its seen set on name, parameter types and constness. Two overloads are two entries, and the guard has something to find. Verified by regenerating: juce::LowLevelGraphicsContext, which declares fillRect over both Rectangle<int> and Rectangle<float> as pure virtuals, is now withheld for the overload rather than for the return type of a later method - it was already withheld, so no class changed from emitted to withheld. Reverting the key to the bare spelling restores the old line, which is the failing case for this change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
pure_virtuals treated a non-pure method as implementing a base's pure virtual whenever the two shared a bare NAME. C++ overrides by signature, so that is not the relation it was testing for. juce::ComponentMovementWatcher is the case in this tree. It declares three pure virtuals, and its ComponentListener base declares non-pure componentMovedOrResized(Component&, bool, bool) and componentVisibilityChanged(Component&). Those two overload the pure virtuals rather than override them, but they carried the same names, so two of the three pure virtuals were dropped and CustomComponentMovementWatcher was emitted implementing only componentPeerChanged. The subclass stayed abstract - the opposite of this generator's contract, which is that a class it cannot represent is withheld with a reason rather than emitted broken. Nothing noticed because an abstract class still COMPILES; only a `new` fails. So the masking set, the seen set and the final filter all key on name, parameter types and constness together. The regenerated subclass now overrides all three, and the test constructs it: a `new` is the only thing that can tell an abstract generated class from a usable one, which is why the surrounding block constructs every other one too. Planted-failure check: reverting the masking set to the bare spelling and regenerating puts the one-method subclass back, and compiling test_juce_gui_basics into a clean nimcache fails with error: allocating an object of abstract class type 'june::CustomComponentMovementWatcher' A CLEAN nimcache is required to see it. The Nim-emitted .cpp is unchanged by a generated-header edit, so an incremental build reuses the stale object file and reports success. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
collect_typedefs recorded every typedef under its UNQUALIFIED spelling,
first one wins. Ptr is declared by dozens of JUCE classes, so whichever
the walk reached first - DynamicObject's - answered for all of them.
ImagePixelData::clone is declared `virtual Ptr clone()`, and the
generator emitted the override returning
ReferenceCountedObjectPtr[DynamicObject]. That is not a wrong-ish type,
it is a type that makes clone override nothing: the emitted C++ carries
`override`, and the class does not compile. The contract is that a
signature the generator cannot represent is withheld with a reason; this
one was emitted broken instead, and stayed unnoticed because a generated
subclass is only compiled where something constructs it.
Keys are now qualified, and map_type looks up both the bare spelling
(which still finds a top-level typedef such as CommandID) and the
qualified name of the declaration cursor. A nested typedef reached with
no cursor behind it no longer resolves to a stranger's; it falls through
to the existing declaration-following path.
The test constructs CustomImagePixelData, which is what compiles the
class at all.
Planted-failure check: restoring the unqualified key and regenerating
puts ReferenceCountedObjectPtr[DynamicObject] back, and compiling
test_juce_gui_basics into a clean nimcache fails with
error: virtual function 'clone' has a different return type
('ReferenceCountedObjectPtr<DynamicObject>') than the function it
overrides (which has return type
'ReferenceCountedObjectPtr<ImagePixelData>')
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A method was commented out with "a C++ iterator; loop with the Nim iterator instead" whenever its spelling merely ENDED IN "Iterator". The suffix is a fact about the name; the reason is a claim about the return type, and nothing connected the two. It happens to be true of all three methods the rule catches today - XmlElement's getChildIterator, getAttributeIterator and getChildWithTagNameIterator all return an unbound iterator type - but nothing kept it true, and the next JUCE method named that way would be withheld under a reason that was simply false. The suffix now only carries the reason where the return type has no Nim spelling. begin, end, cbegin and cend keep their unconditional exclusion: those are iterators by definition, whatever they return. The predicate that decides "this has no Nim spelling" is lifted out of the check further down and named, so the return type and the whole signature are judged by the same rule rather than by two copies of it. Regenerating all five modules reproduces the committed files byte for byte: no method in these modules changes hands. Planted-failure check: adding int getPlantedIterator() const noexcept; to juce::XmlElement and regenerating juce_core emits proc getPlantedIterator*(this: XmlElement): cint ... under this change, and # proc getPlantedIterator*... # a C++ iterator; loop with the Nim # iterator instead under the old rule - the false reason, on a method returning an int. The header edit was reverted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The generated C++ subclass carries a variadic perfect-forwarding constructor, so that a base whose constructor is protected - juce::Button's - can still be reached from outside. It was unconstrained, which makes it a BETTER match for a NON-CONST lvalue than the implicit copy constructor: `Custom b = a` chose the template, forwarded `a` to the BASE's copy constructor, and sliced. The new object was built from the base subobject alone and every std::function handler on it was empty. That is the worst shape this project has: it compiles, it runs, and the callback simply never fires again. Measured directly on CustomUndoableAction, copying from a non-const lvalue reported the handler LOST while copying from a const lvalue reported it kept. Where the base is not copyable at all - juce::Component - the same call instead failed inside juce::Component, naming a constructor the caller never wrote. The template now carries an enable_if that removes it from overload resolution for a single argument that decays to this class or a class derived from it, which is exactly the copy and move cases. The zero-argument and many-argument forms are untouched, so reaching the protected base constructor still works: all five test files compile and pass, and every generated subclass in test_juce_gui_basics still constructs. Planted-failure check: the test added here fails with `b.perform()` the copy lost its handler [AssertionDefect] as soon as the constraint is removed from the emitted template. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The type was declared alongside the N0-N9 and R0-R9 forms but left out of the block of `()` overloads, so it was the one function object with no way to invoke it. Five generated bindings return one - juce::var::getNativeFunction, juce::Slider::valueFromTextFunction and three more - and each handed a Nim caller a value it could hold and never call. The test invokes a var's native function directly and checks it returns the same 7 the method call through the var returns. It is spelled as a plain `()` call rather than as sugar, because the callOperator experimental switch is enabled in june_function_utils and not in the test module. Verified in the emitted C++ rather than only at the Nim level: the generated test_juce_core.nim.cpp contains juce::var direct_1 = std::invoke(native_1, noArguments_1); which is the importcpp reaching the C++ compiler. A binding nothing calls never gets that far, which is how this one stayed missing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`doAssert compiles(...)` type-checks an expression and generates no code
for it. An importcpp string is handed to the C++ compiler only at a
code-generated call site, so those assertions were the one shape that
cannot fail: unhandledException, CppException.what,
Typeface.createSystemTypefaceFor over a Span[CppByte],
AccessibilityHandler.getTypeIndex, CppTypeIndex.== and CppTypeIndex.name
had never been compiled at all.
Each site's reason for not calling outright was real - there is no way
to raise a C++ exception from Nim, no font bytes to hand over, no
AccessibilityHandler to build. Those are reasons not to RUN the call.
They were never reasons not to COMPILE it.
So each call moves into a named proc that is bound to a proc VARIABLE.
Nim skips a proc nothing references; a proc variable is a reference it
will not elide. `cast[pointer](p)` is NOT sufficient - measured: with
the cast, the generated C++ contained neither body.
Compiling them found a defect immediately. std::type_index has no
default constructor, so a Nim proc RETURNING a CppTypeIndex opens with
`std::type_index result{}` and does not compile. The three type_index
calls therefore discard their results, which still generates the call.
The `compiles` assertion could not have told anyone that, because it
never asked the C++ compiler anything.
Verified in the emitted C++ rather than at the Nim level. The generated
sources now contain:
app_p0.unhandledException(e_p1, juce::String(...), ((int)42));
result = (*e_p0).what();
result = juce::Typeface::createSystemTypefaceFor(data_p0);
(void)((*handler_p0).getTypeIndex());
(void)((a_p0 == b_p1));
(void)(a_p0.name());
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The export pattern captured `\w+`, which stops at the first punctuation
and so matched no backtick-quoted operator at all. Fifty-two of the
three hundred and eighteen exported declarations in the hand-written
layer went unseen, covering nine names - (), [], []=, ==, <, <=, $,
=copy and =destroy - and the CI step named "Every hand-written binding
is called" printed "all 151 hand-written binding names are called"
without having looked at one of them. A gate that cannot fail is worse
than no gate: it manufactures the confidence it was built to earn.
Widening the pattern alone would have been just as false in the other
direction. An operator is applied as SYNTAX - `a == b`, `$x`, `s[i]` -
never as `a.==(b)`, so the by-name search cannot find its call sites and
all nine would have reported as uncalled. One blanket exemption would
have reported all nine as covered.
So each operator says how it is verified, in operator_uses, and every
part of that is read off the tree rather than promised:
- seven name a fragment of the tests that applies the operator, and
the fragment has to still be there;
- `=copy` and `=destroy` are recorded as having no C++ binding behind
them - one is a `{.error.}` deletion marker, the other `= discard` -
and the gate CHECKS that, by looking for an importcpp on their
declarations;
- an operator with no entry fails the gate, which is what stops the
next one being waved through;
- an entry naming an operator that no longer exists fails it too, the
same way `uncallable` is already kept honest.
The fragments are chosen so they can only mean the declaration they
stand for. `makeString("aa") < makeString("bb")` rather than a StringRef
on the left, which picks the GENERATED `<`(StringRef, String) instead
and would say nothing about the hand-written one. That comparison had no
use at all, so this adds it - verified in the emitted C++ as
`juce::String(...) < juce::String(...)`, which is the hand-written
`# < #`.
The count it prints is now true: 160 names, 151 found by name and 9
operators held to the table, over 318 declarations.
Planted-failure checks, all four ways it can fail, each run against this
tree and then reverted:
remove the `<` fragment from the test ->
`<` is recorded as applied by makeString("aa") < makeString("bb")
and that is no longer in the tests or examples
add an exported `>=` with no entry ->
`>=` is exported but operator_uses does not say how it is checked...
give `=destroy` an importcpp ->
`=destroy` is recorded as having no C++ binding behind it, but one
of its declarations now has an importcpp
rename the `<=` declaration away ->
`<=` is listed in operator_uses but is no longer an exported operator
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
/ai-review |
There was a problem hiding this comment.
No findings survived the audit phase.
No findings.
Noteworthy
- Extensive test coverage across all 5 test files, with ~150+ new test procs exercising generated subclasses, iterators, operators, and edge cases.
- The closure environment retention fix (retainedClosureEnvironments threadvar) prevents use-after-free on captured closures passed to C++ — a subtle and dangerous bug class.
- Generated-file drift detection in CI (regenerate + git diff --exit-code) is a robust gate that prevents generator edits from landing without their output.
Verdict: APPROVE.
Commands
- Comment
/ai-reviewor mention @axiomantic-momus[bot] to request a re-review of the latest changes. - Reply to a finding with
won't fix,by design, ornot a bugto decline it. - Reply with
instead, ...to propose an alternative fix.
Cost: $0.43 - 867,627 in / 24,741 out tokens - deepseek/deepseek-v4-pro
Powered by Momus running deepseek/deepseek-v4-pro via openrouter.ai.
Generates a Nim subclass for the abstract JUCE classes that can be represented
safely, so a Nim type can override a C++ virtual and have the override
actually reach C++. Five classes are withheld, each with its reason recorded
in the generated file: a private pure virtual, a platform-conditional
declaration, and three signatures with no Nim spelling.
What is here
tools/generate_subclasses.pyemits a*_subclasses.nimmodule per JUCEmodule. Each generated subclass carries the pure virtuals of its base plus the
inherited ones, and installs Nim closures as the implementations. The overrides
are then exercised through the BASE class, which is what shows the dispatch
really crossed the language boundary rather than staying in Nim.
Getting there needed the generator to resolve types it had previously skipped:
nested types the subclasses referred to, virtuals taking a reference, virtuals
taking or returning a const pointer, and inherited typedefs and
std::types.Where a signature could not be represented safely the generator now refuses it
outright instead of emitting something that compiles and misbehaves.
The coverage gate
tools/check_handwritten_covered.pyfails the build when a hand-writtenbinding is never called by a test. A binding that nothing calls is never handed
to the C++ compiler at all, because an
importcppproc reaches the compileronly at a call site - so an unused binding can be broken for a long time
without anything noticing.
It tracks
macroexports alongside procs, iterators, templates andconverters, because a macro is only checked where it is expanded, exactly as an
importcppproc is only checked where it is called. Leaving the keyword outmade the gate blind to four of them - and the first thing it found once it
could see was
CppFunctionObjectRet, exported and referenced nowhere in therepository. It is removed here; its sibling
CppFunctionObjectis used by thelifting layer and stays, with a test pinning what it expands to.
At this commit it reports all 151 hand-written binding names called, with 3
listed as uncallable and a reason recorded for each. The figure counts NAMES:
115 further declarations share a name with one of those, mostly overloads, and
the report says so rather than letting the number read as a count of
declarations.
The same script checks that every file under
sources/opens with theproject's copyright notice, exactly once. A generated file is still a file in
this repository, and a generator whose prolog omits the notice removes it
silently on the next regeneration. Generated modules are compared byte for byte
against a hand-written one, because the notice contains a non-breaking space
that an ordinary space would replace invisibly.
Breaking: the minimum Nim version rises to 2.2.2
june.nimblewent fromnim >= 1.6.0tonim >= 2.2.2, so this PR dropssupport for Nim 1.6 and 2.0. It is not a tidy-up: 1.6, 2.0 and 2.2.0 all
miscompile a generic over an
importcpptype, and 2.2.2 is the first releasethat builds this library at all. The CI matrix changes with it, from
1.6.14/2.0.14/2.2.10 to 2.2.2/2.2.10, so the stated minimum is the measured
one rather than a guess.
The matrix excludes macOS with 2.2.2. nim-lang.org publishes no macOS build
below 2.2.8 - 2.2.0 through 2.2.6 exist as linux_x64 only - so nimble cannot
install the pinned version there. It answers "No nim version matching any
version", downloads 2.2.10 and exits 0, which would leave a green check
reporting a compiler it never ran. The declared minimum is checked on Linux,
where a build of it exists.
Comparing two bound values silently reported them equal
Most JUCE classes declare no
operator==. Nim fell back to structuralequality, and an
importcppobject declares no fields, so the comparisonlooked at nothing and answered true for every pair - silently, and in the
direction that makes a test pass.
The generator now emits an erroring
==for a class where it bound none, socomparing two of them is a compile error naming the type instead. At this head
443 distinct types carry that guard and 55 have a real bound
==;!=derivesfrom
==and is covered by the same.The guard immediately found that
juce::Stringandjuce::vardeclare equalityas free functions, which the generator never saw because it walks members. A
String equality assertion that appeared to pass had been going through the
structural fallback rather than through JUCE.
Breaking: the implicit String to string converter is gone
converter toNimString*(text: String): stringis removed. WithtoJuceStringconverting the other way, any mixed comparison had two equally good paths, and
Nim 1.6 and 2.0 call that ambiguous where 2.2 picks one. Use
$for a Nimstring.
The README becomes usable
Subclassing is the point at which the bindings can carry an application, so the
README grows the sections that were missing to make that reachable: how to
build from source, a worked example application, theming through a
LookAndFeel, what is and is not bound, and how to regenerate.The application example is the exact contents of
examples/test_app.nimrather than a trimmed copy, because the trimmed version it replaced had drifted
from the file it came from. The other two blocks are excerpts, and nothing
checks that they still match their sources - CI compiles the example files, but
compiling a file says nothing about a copy of it in prose.
Two generated subclasses were emitted broken, and are fixed here
CustomImagePixelData.clonewas typedReferenceCountedObjectPtr[DynamicObject]where JUCE declares
virtual Ptr clone()andPtrisReferenceCountedObjectPtr<ImagePixelData>: the typedef table was keyed on theunqualified name with first-wins semantics, and
DynamicObjectdeclares aPtrtoo. A
ComponentMovementWatchersubclass had the same shape from a differentcause - pure virtuals were masked by bare method name, so the two that a base
declares non-pure under the same names were dropped and it implemented one of
three. Both classes stayed abstract, and the override did not override.
Neither showed up on its own, because nothing constructed either class and C++
only checks that a class implements its pure virtuals where something
instantiates it - the same reason an
importcppproc is unverified untilsomething calls it.
The generator now keys the typedef table on the qualified name and masks a pure
virtual only with a matching signature, and a test constructs each of the two
classes so the compiler is the thing that says so. Reverting either change turns
the corresponding test into a compile error, which is how both were verified.
The refusal path for a class the generator cannot represent is reachable now as
well: it was unreachable, so such a class was emitted with one override rather
than withheld with a reason.
CppFunctionObjectR1Refwas declared with no()operator, so a functionreturning one could not be invoked from Nim; it has one here, with a test that
calls it.
What remains deferred is the coverage gate's by-name matching. Two new iterators
are both called
items, and the gate matches by NAME, so unrelateditemscalls satisfy it. That is a deliberate design of the gate rather than an
oversight, and the following PR is where the receiver is checked as well as the
name.