8000 Fix a mutation-during-iteration bug in package object loading by retronym · Pull Request #11064 · scala/scala · GitHub
[go: up one dir, main page]
Skip to content

Fix a mutation-during-iteration bug in package object loading #11064

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
May 23, 2025
Merged

Fix a mutation-during-iteration bug in package object loading #11064

merged 1 commit into from
May 23, 2025

Conversation

retronym
Copy link
Member
@retronym retronym commented May 20, 2025 •
edited
Loading

The deferredOpen mechanism was added in the 2.13.x series to avoid prematurely loading a package.class prior to finding that the current sources contain the source of this that package object. #9661

This mechanism was refined to support a use case with -Yimports (custom predef imports). (#10333)

The original change to packageObjects appears to have have incorrectly put the "load whatever is is left in deferredOpen logic in apply (per unit logic) rather than run (per-phase logic). I've moved it.

We then need to take a defensive copy of the collection before iteration asn openPackageModule can trigger typechecking of package object template parents, which now mutates deferredOpen. The behaviour is undefined in this case.

I've also switched the backing collection to a j.u.LinkedHashSet which fails fast when if we mutate during iteration. The enclosed test fails before the final commit make it green. I figured that using a Linked collection would make the compilation order deterministic between runs which might help diagnose any remnant bugs in this area.

@scala-jenkins scala-jenkins added this to the 2.13.17 milestone May 20, 2025
@retronym retronym force-pushed the bug/package-object-deferred branch from b26db93 to d1b5ea3 Compare May 20, 2025 06:07
@retronym retronym requested a review from som-snytt May 20, 2025 06:10
lrytz
lrytz reviewed May 20, 2025
View reviewed changes
Copy link
Member
@lrytz lrytz left a comment

Choose a reason for hiding this comment

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

Looks good! Compiling src/library also triggers the ConcurrentModificationException...

Let's squash into a single commit.

@retronym
Fail fast if we mutate deferredOpen during iteration
4b5ec3e 
Test for edge case with package objects load

Avoid mutation-during-iteration during package object initialization
@retronym retronym force-pushed the bug/package-object-deferred branch from d1b5ea3 to 4b5ec3e Compare May 20, 2025 12:59
som-snytt
som-snytt approved these changes May 20, 2025
View reviewed changes
Copy link
Contributor
@som-snytt som-snytt left a comment

Choose a reason for hiding this comment

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

We know deferredOpen is finally empty because isPast(namerPhase) so that forcing can remove but not add to deferreds.

I confirmed that this is not the longest pos test name.

@lrytz lrytz merged commit f4ca74b into scala:2.13.x May 23, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lrytz lrytz lrytz left review comments

@som-snytt som-snytt som-snytt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.13.17
Development

Successfully merging this pull request may close these issues.
4 participants
@retronym @lrytz @som-snytt @scala-jenkins
0