Modify the idGenerator - #333905
Draft
SteVen Batten (sbatten) wants to merge 1 commit into
Draft
Conversation
SteVen Batten (sbatten)
changed the title
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Counter wrapping introduces duplicate IDs that can overwrite active resources.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
 |
src/vs/base/common/idGenerator.ts — The modulo makes IDs repeat after 1,000 calls, violating the generator's uniqueness contract. This… |
What changed in this PR
Changes the shared ID generator to cycle numeric suffixes every 1,000 IDs.
Changes:
- Adds modulo-based counter wrapping.
| File | Description |
|---|---|
src/vs/base/common/idGenerator.ts |
Wraps generated IDs at 1,000. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+17
to
+18
| this._lastId = (this._lastId + 1) % 1000; | ||
| return this._prefix + this._lastId; |
SteVen Batten (sbatten)
force-pushed
the
sbatten/random-bunny
branch
from
September 1, 2026 23:57
991c6ac to
57578b5
Compare
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Wrapping introduces duplicate IDs for consumers that require uniqueness.
Review tier: Balanced
Findings: 1
Pre-existing issues (1)
| Severity | Finding |
|---|---|
|
src/vs/base/common/idGenerator.ts — The modulo makes IDs repeat after 1,000 calls, violating the generator's uniqueness contract. This… View comment |
Suppressed comments (1)
src/vs/base/common/idGenerator.ts:18
- The modulo makes IDs repeat every 1,000 calls (and changes the 1,000th ID to
prefix0). This generator is used where IDs must remain unique—for example,OneReference.idis the tree identity andTextEditorDecorationTypekeys may remain registered—so a large reference result or a long-running extension host can collide with a live object and overwrite/misidentify it. Keep the counter monotonic rather than wrapping it.
this._lastId = (this._lastId + 1) % 1000;
return this._prefix + this._lastId;
<!-- Thank you for submitting a Pull Request. Please: * Read our Pull Request guidelines: https://github.com/microsoft/vscode/wiki/How-to-Contribute#pull-requests * Associate an issue with the Pull Request. * Ensure that the code is up-to-date with the `main` branch. * Include a description of the proposed changes and how to test them. -->
SteVen Batten (sbatten)
force-pushed
the
sbatten/random-bunny
branch
from
September 2, 2026 00:28
57578b5 to
fec1fee
Compare
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Wrapping generates duplicate IDs and breaks callers that require uniqueness.
Review tier: Balanced
Findings: 1
Pre-existing issues (1)
| Severity | Finding |
|---|---|
|
src/vs/base/common/idGenerator.ts — The modulo makes IDs repeat after 1,000 calls, violating the generator's uniqueness contract. This… View comment |
Suppressed comments (1)
src/vs/base/common/idGenerator.ts:18
- This makes a process-wide ID generator repeat values after 1,000 calls, violating the uniqueness its callers rely on. For example,
TextEditorDecorationTypeuses a static generator for registration keys; the 1,001st decoration will reuse the first key, and disposing either instance can unregister the other. Keep the counter monotonic rather than wrapping it.
this._lastId = (this._lastId + 1) % 1000;
return this._prefix + this._lastId;
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.
No description provided.