Skip to content

fix(arrays): clean up duplicate logic and improve array helpers#317600

Open
mohityadav8 wants to merge 20 commits into
microsoft:mainfrom
mohityadav8:fix-arrays-cleanup
Open

fix(arrays): clean up duplicate logic and improve array helpers#317600
mohityadav8 wants to merge 20 commits into
microsoft:mainfrom
mohityadav8:fix-arrays-cleanup

Conversation

@mohityadav8
Copy link
Copy Markdown

Summary

This PR improves several utilities in src/vs/base/common/arrays.ts:

  • removes redundant assignment logic in range()
  • updates withoutDuplicates() to reuse distinct() instead of duplicating logic, while preserving API compatibility
  • replaces [].concat(...arrays) in concatArrays() with an iterative implementation to avoid spread argument limits and improve type safety

Testing

  • Verified the diff only affects the intended three utility functions in arrays.ts
  • No public API was removed

Notes

I intentionally did not remove deprecated exports (move, insert, remove) since that would be a breaking API change and is better handled separately.

Copilot AI review requested due to automatic review settings May 20, 2026 19:21
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

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 refactors a few core utilities in src/vs/base/common/arrays.ts to reduce duplicated logic and adjust array helper implementations within the vs/base/common layer.

Changes:

  • Reworked concatArrays() to build results iteratively instead of using [].concat(...arrays).
  • Simplified range() initialization logic.
  • Updated withoutDuplicates() to delegate to distinct().
Comments suppressed due to low confidence (1)

src/vs/base/common/arrays.ts:594

  • withoutDuplicates previously preserved sparse-array holes as undefined (via new Set(array) / array iteration). Switching to distinct(array) changes behavior because distinct uses array.filter(...), which skips holes entirely. If API compatibility for sparse arrays matters, consider keeping the old Set-based implementation here or updating distinct/this wrapper to iterate with for...of instead of filter.

export function asArray<T>(x: T | T[]): T[];
export function asArray<T>(x: T | readonly T[]): readonly T[];

Comment thread src/vs/base/common/arrays.ts Outdated
@mohityadav8
Copy link
Copy Markdown
Author

@microsoft-github-policy-service agree

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

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

Comment thread src/vs/base/common/arrays.ts Outdated
Comment thread src/vs/base/common/arrays.ts Outdated
Comment thread src/vs/base/common/arrays.ts
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

src/vs/base/common/arrays.ts:593

  • withoutDuplicates used to be Array.from(new Set(array)), which visits array holes via the iterator (treating them as undefined). distinct(array) uses filter, which skips holes entirely, so sparse arrays will now lose the hole/undefined entry and this is a behavior change despite the same signature. If you want to keep the previous semantics while reusing distinct, consider calling distinct(Array.from(array)) (or keep the original Set-based implementation).
export function withoutDuplicates<T>(array: ReadonlyArray<T>): T[] {
	return distinct(array);
}

Comment thread src/vs/base/common/arrays.ts Outdated
Comment thread src/vs/base/common/arrays.ts Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

Comment thread src/vs/base/common/arrays.ts Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

src/vs/base/common/arrays.ts:213

  • The manual copy loop in concatArrays does not preserve sparse array holes: reading arr[i] and assigning to result[...] will materialize holes as explicit undefined entries, whereas Array.prototype.concat preserves holes. If callers rely on hole semantics (e.g. i in array checks), consider copying only existing indices (e.g. if (i in arr)) and setting result.length = offset + arr.length to keep holes.
	for (const arr of arrays) {
		const offset = result.length;
		for (let i = 0; i < arr.length; i++) {
			result[offset + i] = arr[i];
		}
	}

Comment thread src/vs/base/common/arrays.ts Outdated
Comment thread src/vs/base/common/arrays.ts
mohityadav8 and others added 2 commits May 21, 2026 01:59
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@mohityadav8 mohityadav8 requested a review from Copilot May 20, 2026 20:35
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

Comment thread src/vs/base/common/arrays.ts Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.

Comment thread src/vs/base/common/arrays.ts Outdated
Comment thread src/vs/base/common/arrays.ts Outdated
mohityadav8 and others added 2 commits May 21, 2026 11:11
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@mohityadav8 mohityadav8 requested a review from Copilot May 21, 2026 05:50
@mohityadav8
Copy link
Copy Markdown
Author

cc @mjbvz :D

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

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

Comment thread src/vs/base/common/arrays.ts Outdated
Comment thread src/vs/base/common/arrays.ts Outdated
Comment thread src/vs/base/common/arrays.ts
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.

3 participants