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.

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

Create boxed environments only for references and function values #16136

Merged
merged 5 commits into from
Oct 19, 2022
Merged

Create boxed environments only for references and function values #16136

merged 5 commits into from
Oct 19, 2022

Conversation

Linyxus
Copy link
Contributor

@Linyxus Linyxus commented Oct 3, 2022

Fixes #16114.

According to the comment in the issue, only create boxed environment when the rechecked tree is a reference or a function value.

This PR also fixes a testcase, as explained in this commit.

@Linyxus Linyxus marked this pull request as draft October 3, 2022 13:44
@Linyxus
Copy link
Contributor Author

Linyxus commented Oct 4, 2022 •
edited
Loading

Note on limitation: ideally we should allow the following code snippet to typecheck:

val op1: {io} Unit -> Unit = (x: Unit) =>  // error // limitation
  expect[{*} Cap] {
    io.use()
    fs
  }

Since fs is a reference on the expr place of the block, thus being boxed. The captured set of the block should be only {io}. However, since right now the expected type of a block is not propagated to its expr (as seen here), we do not know that the expected type is boxed when typing fs, therefore mark fs as free in the environment. Therefore for the code above we will complain that the actual type captures {io, fs} while the expected capture set is only {io}. We would have to keep track of the fact that the expected type is boxed when we decide not to propagate it to typecheck such code snippets.

@Linyxus Linyxus marked this pull request as ready for review October 4, 2022 12:13
@Linyxus Linyxus requested a review from odersky October 4, 2022 12:15
@odersky
Copy link
Contributor

odersky commented Oct 15, 2022

Do we still need this one after #16141 is merged? If yes it needs to be rebased.

@Linyxus
only create boxed environment for function values and references
08321d4
@Linyxus
fix boxmap test
7ae54f1 
The result should capture {f}, since the expression `box(f(x))` should be
translated as:

  let z = f(x) in
    let y = □ z in
      box(y)

Denoting the above term as `t`, we can see that `f ∈ cv(t)`.
@Linyxus
create boxed environment for all ref trees
4805007
@Linyxus Linyxus force-pushed the toplevel-boxes branch 2 times, most recently from 9e20cf2 to 8bab429 Compare October 16, 2022 07:34
odersky
odersky requested changes Oct 16, 2022
View reviewed changes
Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

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

In general, it's better to push PRs to dotty-staging. Then they are easier to find, and we can collaborate on them.

compiler/src/dotty/tools/dotc/cc/CheckCaptures.scala Outdated
case _ =>
false

private def isRefTree: Boolean = tree match
Copy link
Contributor

Choose a reason for hiding this comment

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

Just use isInstanceOf[RefTree] instead, that avoids having to define a new function.

compiler/src/dotty/tools/dotc/cc/CheckCaptures.scala Outdated
@@ -507,14 +507,29 @@ class CheckCaptures extends Recheck, SymTransformer:
recheckFinish(result, arg, pt)
*/

/** If expected type `pt` is boxed, don't propagate free variables.
extension (tree: Tree)
private def isFunctionLiteral(using Context): Boolean = tree match
Copy link
Contributor

Choose a reason for hiding this comment

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

TreeInfo has a closure extractor, which could be used instead of defining a new function here.

compiler/src/dotty/tools/dotc/cc/CheckCaptures.scala Outdated
* Otherwise, if the result type is boxed, simulate an unboxing by
* adding all references in the boxed capture set to the current environment.
*/
override def recheck(tree: Tree, pt: Type = WildcardType)(using Context): Type =
if tree.isTerm && pt.isBoxedCapturing then
val saved = curEnv
curEnv = Env(curEnv.owner, nestedInOwner = false, CaptureSet.Var(), isBoxed = true, curEnv)
if tree.isRefTree || tree.isFunctionLiteral then
Copy link
Contributor

Choose a reason for hiding this comment

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

So this could be:

tree match
  case _: RefTree | closure(_, _, _) => ...

@Linyxus
Copy link
Contributor Author

Linyxus commented Oct 16, 2022

Thanks for your comment! I have refactored the code to use pattern match and the extractors in TreeInfo instead. closureDef is used here because closure can also match block expressions like:

{
  val x = { ... }
  () => io.use()
}

But what we want to match here is only function values.

Regarding dotty-staging, I have not obtained the write access to it yet. Could you possibly grant me the access? I would create all future PRs from the staging repo. :)

@Linyxus Linyxus requested a review from odersky October 16, 2022 18:07
@odersky
Copy link
Contributor

odersky commented Oct 16, 2022 via email

@Linyxus
Copy link
Contributor Author

Linyxus commented Oct 17, 2022 •
edited
Loading

Hi Yichen, I just added you to dotty-staging. - Martin

Thanks! I've joined dotty-staging and will create future PRs from there.

@Linyxus Linyxus merged commit 0c6744d into scala:main Oct 19, 2022
@Kordyjan Kordyjan added this to the 3.2.2 milestone Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@odersky odersky odersky approved these changes

Assignees

@Linyxus Linyxus

Labels
None yet
Projects
None yet
Milestone
3.2.2
Development

Successfully merging this pull request may close these issues.

Leaking scoped capability when the expected type of a block is boxed
3 participants
@Linyxus @odersky @Kordyjan