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

Fix potential soundness hole when adding references to a mapped capture set #18758

Merged
merged 3 commits into from
Oct 30, 2023

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Oct 25, 2023

Fix potential soundness hole when adding references to a set that is the image of an
idempotent tm map tm. If the element ref did not come from the source
of the set, we still assumed that tm(ref) = ref, so that we simply added
the reference to the set and also back-propagated it to source. But that is not
necessarily the case. (Although it is the case in our complete test suite,
so I am not sure this case can actually arise in practice. Nevertheless,
it's better to not leave a potential soundness hole here.)

In the new implementation, we test whether tm(ref) = ref, and only proceed
as before if that's the case. If not there are two sub-cases:

  • {ref} <:< tm(ref) and the variance of the set is positive. In that case we
    can soundly add tm(ref) to the set while back-propagating ref as before.
  • Otherwise there's nothing obvious left to do except fail (which is always
    sound).

@odersky odersky added the cc-experiment Intended to be merged with cc-experiment branch on origin label Oct 25, 2023
Fix soundness hole when adding references to a set that is the image of an
idempotent `tm` map `tm`. If the element `ref` did not come from the source
of the set, we still assumed that `tm(ref) = ref`, so that we simply added
the reference to the set and also back-propagated it to source. But that is not
necessarily the case (although it is the case in our complete test suite,
so I am not sure this case can actually arise in practice. Nevertheless,
it's better to not leave a potential soundness hole here.

In the new implementation, we test whether `tm(ref) = ref`, and only proceed
as before if that's the case. If not there are two sub-cases:

 - `{ref} <:< tm(ref)` and the variance of the set is positive. In that case we
   can soundly add `tm(ref)` to the set while back-propagating `ref` as before.
 - Otherwise there's nothing obvious left to do except fail (which is always
   sound.
@odersky odersky changed the title Drop outdated CC config settings Fix potential soundness hole when adding references to a mapped capture set Oct 25, 2023
@odersky odersky requested a review from Linyxus October 28, 2023 16:29
Copy link
Contributor

@Linyxus Linyxus left a comment

Choose a reason for hiding this comment

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

LGTM!

@Linyxus Linyxus enabled auto-merge October 30, 2023 13:47
@Linyxus Linyxus merged commit 252e78a into scala:main Oct 30, 2023
18 checks passed
@Linyxus Linyxus deleted the cc-drop-outdated branch October 30, 2023 16:36
@Kordyjan Kordyjan added this to the 3.4.0 milestone Dec 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cc-experiment Intended to be merged with cc-experiment branch on origin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants