Skip to content

fix(utilities): do not mutate typed arrays in shuffle() when modify is false - #9186

Merged
perminder-17 merged 2 commits into
processing:mainfrom
Pcmhacker-piro:fix/shuffle-typedarray-copy
Sep 24, 2026
Merged

perminder-17 merged 2 commits into
processing:mainfrom
Pcmhacker-piro:fix/shuffle-typedarray-copy

Conversation

@Pcmhacker-piro

@Pcmhacker-piro Pcmhacker-piro commented Sep 16, 2026 •

Copy link
Copy Markdown
Contributor

Resolves #9128

Changes:

shuffle() documentation states: "By default, the original array won't be modified. Instead, a copy will be created, shuffled, and returned."

However, in src/utilities/utility_functions.js, shuffle() contained a legacy isView check:

const isView = ArrayBuffer && ArrayBuffer.isView && ArrayBuffer.isView(arr);
arr = modify || isView ? arr : arr.slice();

Because isView was truthy for any TypedArray (Float32Array, Uint8Array, etc.), typed arrays were always treated as if modify was true, causing them to be mutated in-place and returning the original array reference (b === a) even when modify was omitted or false.

TypedArray.prototype.slice() has been standard across all modern JavaScript runtimes since ES2015, returning a copy of the typed array.

This PR:

  • Removes the redundant isView condition in src/utilities/utility_functions.js so that arr = modify ? arr : arr.slice();.
  • When modify is false (default), arr.slice() creates a copy for typed arrays as well as regular arrays without modifying the input array.
  • When modify is true, the array is still modified in-place as expected.
  • Adds comprehensive unit tests in test/unit/utilities/utility_functions.js covering both default (non-mutating) and in-place (modify: true) behavior for both regular arrays and typed arrays.

Verification:

1. Terminal Output & Verification

Shuffle Terminal Verification


PR Checklist

  • npm run lint passes (0 errors)
  • Inline reference is included / updated (N/A, behavior bug fix conforming to existing documentation)
  • Unit tests are included / updated (26/26 tests passing)

@Pcmhacker-piro

Copy link
Copy Markdown
Contributor Author

Hi @davepagurek @ksen0! 👋

Could you please take a look at this PR when you get a chance?

It resolves #9128 by removing the legacy isView check from fn.shuffle. Typed arrays have supported .slice() natively since ES2015, so shuffle(typedArray) now correctly creates and returns a shuffled copy by default without mutating the input typed array in place, matching documented behavior.

  • All 26 utility unit tests are passing (with new test coverage for both typed arrays and regular arrays in default and modify: true modes).
  • npm run lint exits clean with 0 errors.
  • Realistic terminal proof screenshot and animated workflow demonstration are included above.

Thank you so much!

@p5-bot

p5-bot Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

Continuous Release

CDN link

Published Packages

Commit hash: 7f36d5a

Previous deployments

afb453e


This is an automated message.

@perminder-17 perminder-17 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good to me!

@perminder-17
perminder-17 merged commit fe22531 into processing:main Sep 24, 2026
4 checks passed
@Pcmhacker-piro

Pcmhacker-piro commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor Author

Hey @perminder-17, sir

My real GitHub contribution account is Pcmhacker-piro. But the git bot wrongly added my other account, Pcmhacker-hero, to the contribution list, so please update this, and future commits will be added via Pcmhacker-piro.

Thanks, sir.

@perminder-17

Copy link
Copy Markdown
Collaborator

@all-contributors please add @Pcmhacker-piro for code

@allcontributors

Copy link
Copy Markdown
Contributor

@perminder-17

@Pcmhacker-piro already contributed before to code

@Pcmhacker-piro

Copy link
Copy Markdown
Contributor Author

@perminder-17
Hey sir, it's still showing my other account (Pcmhacker-hero), so please update all 5 commits into the Pcmhacker-piro GitHub account

6a7553d2147c42ebf680cd0c0e41c94da0275bba4001a7883b4c4538aa351a43

@davepagurek

Copy link
Copy Markdown
Contributor

Hi, I think we misread your message as being related to the all-contributors bot which does have your -piro account correctly and is something we manually change. The commits themselves are connected to that account: https://github.com/processing/p5.js/pull/9186/commits but this is not something we are responsible for generally. Git commit authors can be changed for your own branch like so: https://stackoverflow.com/questions/3042437/how-can-i-change-the-commit-author-for-a-single-commit I think github connects accounts to commits via email address, so it looks like you were committing code with an old email address.

@ksen0 now that this is merged, we would have to rebase our branches and rewrite history to change the author of commits.

@Pcmhacker-piro

Copy link
Copy Markdown
Contributor Author

@davepagurek

Thanks for checking. Yes, the issue I’m referring to is specifically the repository’s Contributors list. My commits are currently being attributed to my old GitHub account there, while I want them to be attributed to my current -piro account.

I understand that this is related to the email address used in the commits. Since the PR is already merged, is there any way to correct the contributor attribution without rewriting the repository history? If not, I’ll make sure my Git email is correctly configured for future contributions.

@davepagurek

Copy link
Copy Markdown
Contributor

Unfortunately generally we want to avoid force pushes to main to avoid the risk of messing something up or causing issues with people's forks. We do mostly use the all-contributors spec for tracking p5 contributions though, as not all code changes persist through git and also not all contributions are in the form of code checked into git either, and that looks like it has the right account.

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.

[p5.js 2.0+ Bug Report]: shuffle() mutates typed arrays in place even when modify is false

4 participants