Skip to content
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.

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

MetadataStorage is mutable - structural equals in copy isn't enough #11513

Merged
merged 5 commits into from
Nov 8, 2024

Conversation

JaroslavTulach
Copy link
Member

@JaroslavTulach JaroslavTulach commented Nov 8, 2024

Pull Request Description

MetadataStorage is mutable. Using Object.equals in IR.copy methods to check whether a copy is needed isn't enough. The fact that two storages are Object.equal may be just temporary. Replacing the checks in 69 IR.copy methods with identity check - e.g. ne in Scala which is == in Java.

Using proper structural check inside of MetadataStorage fixes #11171.

Important Notes

I used this regex to find out 69 instances of IR.copy:

69 copy methods

and I modified all 69 of them.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • All code follows the
    Scala,
    Java,
  • Unit tests should be updated to new behavior of MetadataStorage.equals

@JaroslavTulach JaroslavTulach added the CI: No changelog needed Do not require a changelog entry for this PR. label Nov 8, 2024
@JaroslavTulach JaroslavTulach self-assigned this Nov 8, 2024
@JaroslavTulach JaroslavTulach force-pushed the wip/jtulach/ShallowCopy11171 branch from 1b4ab22 to 2705645 Compare November 8, 2024 08:15
@JaroslavTulach JaroslavTulach changed the title Avoid sharing of MetadataStorage between two IR nodes when copying MetadataStorage is mutable - structural equals in copy isn't enough Nov 8, 2024
@JaroslavTulach JaroslavTulach added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label Nov 8, 2024
@JaroslavTulach JaroslavTulach added the CI: Ready to merge This PR is eligible for automatic merge label Nov 8, 2024
@mergify mergify bot merged commit 67db825 into develop Nov 8, 2024
41 of 42 checks passed
@mergify mergify bot deleted the wip/jtulach/ShallowCopy11171 branch November 8, 2024 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Clean build required CI runners will be cleaned before and after this PR is built. CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Two BindingsMap$ResolvedImport aren't equal and raise exception on the CI
2 participants