fix: prevent reduceBy transducer throwing on nullish step result#3529
Open
ATOM00blue wants to merge 1 commit into
Open
fix: prevent reduceBy transducer throwing on nullish step result#3529ATOM00blue wants to merge 1 commit into
ATOM00blue wants to merge 1 commit into
Conversation
When reduceBy is used as a transducer, the @@transducer/result handler in _xreduceBy read '@@transducer/reduced' off the downstream step's return value without guarding against null/undefined. A reducer that legitimately returns null (or undefined) caused "TypeError: Cannot read properties of null (reading '@@transducer/reduced')". Guard the access with a null check, matching the existing guard in the non-transducer reduceBy path, so nullish results propagate correctly. Closes ramda#3238
Coverage Summary> ramda@0.32.0 coverage:summary
> BABEL_ENV=cjs nyc --reporter=text-summary mocha -- --reporter=min --require @babel/register
�[2J�[1;3H
1209 passing (868ms)
=============================== Coverage summary ===============================
Statements : 94.1% ( 2519/2677 )
Branches : 85.9% ( 981/1142 )
Functions : 93.37% ( 563/603 )
Lines : 94.36% ( 2361/2502 )
================================================================================ |
Coverage Summary> ramda@0.32.0 coverage:summary
> BABEL_ENV=cjs nyc --reporter=text-summary mocha -- --reporter=min --require @babel/register
�[2J�[1;3H
1209 passing (986ms)
=============================== Coverage summary ===============================
Statements : 94.1% ( 2519/2677 )
Branches : 85.9% ( 981/1142 )
Functions : 93.37% ( 563/603 )
Lines : 94.36% ( 2361/2502 )
================================================================================ |
There was a problem hiding this comment.
Pull request overview
This PR fixes a crash in the reduceBy transducer implementation when a downstream reducer returns a nullish accumulator (null/undefined), by guarding the reduced-sentinel check in XReduceBy’s @@transducer/result loop.
Changes:
- Add a nullish guard in
source/internal/_xreduceBy.jsbefore reading@@transducer/reducedfrom the step result. - Add a regression test ensuring
reduceByused as a transducer does not throw when the downstreamstepreturnsnull.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
source/internal/_xreduceBy.js |
Prevents TypeError by guarding reduced-sentinel access when the downstream step result is nullish. |
test/reduceBy.js |
Adds a regression test covering the transducer path when the downstream step returns null. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+89
to
+93
| it('does not throw when used as a transducer and the step returns null', function() { | ||
| var evenOddNaN = function(num) { | ||
| return !Number.isFinite(num) ? 'NaN' : num % 2 === 0 ? 'even' : 'odd'; | ||
| }; | ||
| var nullIfNotFiniteOp = function(op) { |
Contributor
There was a problem hiding this comment.
I'm not concerned about this, but feel free to add a test case to cover it if you'd like
Contributor
|
Hi @ATOM00blue , thank you for the PR. I see the test is based on the linked issue. Can we also include a minimal test that shows the issue more clearly? |
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.
Fixes the
TypeErrorraised whenreduceByis used as a transducer and the downstream reducer returnsnull/undefined.XReduceBy's@@transducer/resulthandler read'@@transducer/reduced'off the step's return value without a null guard, so a reducer that legitimately returnsnullproduced:TypeError: Cannot read properties of null (reading '@@transducer/reduced').This adds a
result != nullguard before the reduced-sentinel check, mirroring the existing guard in the non-transducerreduceBypath (source/reduceBy.js), so nullish accumulator results propagate correctly.Added a regression test covering the transducer path (including the reproducer from the issue).
Closes #3238