Skip to content

Conversation

@jjonescz
Copy link
Member

Fixes https://dev.azure.com/devdiv/DevDiv/_workitems/edit/2641964.

StringTokenMap (added in #78033; cc @tmat) used reference equality comparer; I believe by mistake; which made it possible for it to contain the same string multiple times; leading to overflowing the user string heap limit sooner than before.

@tmat
Copy link
Member

tmat commented Nov 19, 2025

Is there a way to test?

@jjonescz
Copy link
Member Author

I'm not sure how exactly did duplicate strings find its way to the map, I can try to figure it out to add an end-to-end test.

I could also add just a unit test of the StringTokenMap.

@jjonescz jjonescz marked this pull request as ready for review November 19, 2025 18:01
@jjonescz jjonescz requested a review from a team as a code owner November 19, 2025 18:01
@tmat
Copy link
Member

tmat commented Nov 19, 2025

Unit test would do. End-to-end might be too fragile.

@jjonescz jjonescz requested a review from tmat November 19, 2025 19:32
@jjonescz

This comment was marked as off-topic.

@github-actions

This comment was marked as off-topic.

@github-actions

This comment was marked as off-topic.

@jjonescz
Copy link
Member Author

jjonescz commented Nov 20, 2025

@dotnet/roslyn-compiler for reviews, thanks; this fixes a regression and will be backported

@AlekseyTs
Copy link
Contributor

StringTokenMap (added in #78033; cc @tmat) used reference equality comparer; I believe by mistake; which made it possible for it to contain the same string multiple times; leading to overflowing the user string heap limit sooner than before.

Is my understanding correct that this issue is a regression caused by a PR that got merged violating merging policy for PRs touching code under Compilers? Specifically, the PR didn't get two sign-offs from the compiler team and the only sign-off from the compiler team wasn't from a senior developer?

@jjonescz
Copy link
Member Author

jjonescz commented Nov 20, 2025

There was a sign off from Chuck (it's not displayed as green checkmark now but was at the time) and me, so that shouldn't be a violation.

senior developer

I was not aware of this requirement.

@AlekseyTs
Copy link
Contributor

Is my understanding correct that this issue is a regression caused by a PR that got merged violating merging policy for PRs touching code under Compilers?

I guess not, for whatever reason Chuck's sign-off is grayed out in the GitHub UI.

Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 2)

@jjonescz
Copy link
Member Author

@dotnet/roslyn-compiler for a second review, thanks

@jjonescz jjonescz requested a review from a team November 20, 2025 19:01
@jjonescz
Copy link
Member Author

@dotnet/roslyn-compiler for a second review, this is soft-approved for .net servicing due next week, thanks

@jjonescz jjonescz merged commit 2d6546b into dotnet:main Nov 21, 2025
25 checks passed
@jjonescz jjonescz deleted the 2641964-DataStringLiterals branch November 21, 2025 11:29
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Nov 21, 2025
jjonescz added a commit that referenced this pull request Nov 24, 2025
Backport of #81341.

QB workitem:
https://devdiv.visualstudio.com/DevDiv/_workitems/edit/2643422

### Customer Impact

Customers are unable to build large projects which contain many string
literals because the limit is being computed incorrectly. The
[vsfeedback
ticket](https://dev.azure.com/devdiv/DevDiv/_workitems/edit/2641964) has
collected 4 votes in 1 day.

### Regression

- [x] Yes - from #78033; worked in
VS 2022, broken in VS 2026
- [ ] No

### Testing

Verified manually on user's repro. Also added a unit test.

### Risk

Low. Simple change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants