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

Add resource finalization to handlers #701

Draft
wants to merge 27 commits into
base: master
Choose a base branch
from
Draft

Conversation

dvdvgt
Copy link
Collaborator

@dvdvgt dvdvgt commented Nov 19, 2024

This PR aims to add resource finalization to handlers. This was already implemented under the branch feature/finalizer, however, the branch is now hilariously outdated and solving the merge conflicts almost impossible. Thus, I manually try to re-implement it.

@@ -659,8 +659,17 @@ class RecursiveDescent(positions: Positions, tokens: Seq[Token], source: Source)
*/
def tryExpr(): Term =
nonterminal:
`try` ~> stmt() ~ someWhile(handler(), `with`) match {
case s ~ hs => TryHandle(s, hs)
`try` ~> stmt() ~ manyWhile(handler(), `with`) ~ finalizerClause(`suspend`, false) ~ finalizerClause(`resume`, true) ~ finalizerClause(`return`, true) ~ finalizerClause(`finally`, true) match {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For some reason, this breaks the current positions and error messages have the wrong span:

Warning:        |[warning] examples/pos/capture/regions.effekt:10:3: Handling effect Dummy, which seems not to be used by the program.
       |  try { prog() } with Dummy { resume(()) }
       |  ^
       |""".stripMargin
=> Diff (- obtained, + expected)
   try { prog() } with Dummy { resume(()) }
-  ^
+  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

@dvdvgt
Copy link
Collaborator Author

dvdvgt commented Dec 2, 2024

The changes to the inliner seem to break some tests. For example, consider the JS output for the file anf.effekt.md:

Without inlining
function traverse_0(e_0, Fresh_2, Bind_0, ks_331, k_263) {
  function b_k_38(n_6, ks_332, k_264) {
    return () => k_264(new CRet_0(new CLit_0(n_6)), ks_332);
  }
  function b_k_39(n_7, ks_333, k_265) {
    return () => k_265(new CRet_0(new CVar_0(n_7)), ks_333);
  }
  function b_k_40(name_7, arg_3, ks_334, k_266) {
    return () =>
      traverse_0(arg_3, Fresh_2, Bind_0, ks_334, (v_r_54, ks_335) =>
        Bind_0.Bind_1(v_r_54, ks_335, (v_r_55, ks_336) =>
          () => k_266(new CApp_0(name_7, v_r_55), ks_336)));
  }
  function b_k_41(x_22, b_0, body_3, ks_337, k_267) {
    return () =>
      bindHere_0((Bind_2, ks_338, k_268) =>
        () => traverse_0(b_0, Fresh_2, Bind_2, ks_338, k_268), Fresh_2, ks_337, (v_r_56, ks_339) =>
        () =>
          bindHere_0((Bind_3, ks_340, k_269) =>
            () => traverse_0(body_3, Fresh_2, Bind_3, ks_340, k_269), Fresh_2, ks_339, (v_r_57, ks_341) =>
            () => k_267(new CLet_0(x_22, v_r_56, v_r_57), ks_341)));
  }
  switch (e_0.__tag) {
    case 0: 
      const v_y_18 = e_0.value_1;
      return () => b_k_38(v_y_18, ks_331, k_263);
    case 1: 
      const v_y_19 = e_0.name_0;
      return () => b_k_39(v_y_19, ks_331, k_263);
    case 3: 
      const v_y_20 = e_0.name_2;
      const v_y_21 = e_0.arg_0;
      return () => b_k_40(v_y_20, v_y_21, ks_331, k_263);
    case 2: 
      const v_y_22 = e_0.name_1;
      const v_y_23 = e_0.binding_0;
      const v_y_24 = e_0.body_0;
      return () => b_k_41(v_y_22, v_y_23, v_y_24, ks_331, k_263);
  }
}
with inlining
function traverse_0(e_4, Fresh_0, Bind_0, ks_2182, k_1778) {
  switch (e_4.__tag) {
    case 0: 
      const v_y_356 = e_4.value_11;
      return () => k_1778(new CRet_0(new CLit_0(v_y_356)), ks_2182);
    case 1: 
      const v_y_357 = e_4.name_7;
      return () => k_1778(new CRet_0(new CVar_0(v_y_357)), ks_2182);
    case 3: 
      const v_y_358 = e_4.name_9;
      const v_y_359 = e_4.arg_0;
      return () =>
        traverse_0(v_y_359, Fresh_0, Bind_0, ks_2182, (v_r_1297, ks_2183) =>
          Bind_0.Bind_1(v_r_1297, ks_2183, (v_r_1298, ks_2184) =>
            () => k_1778(new CApp_0(v_y_358, v_r_1298), ks_2184)));
    case 2: 
      const v_y_360 = e_4.binding_0;
      return RESET((p_174, ks_2185, k_1779) =>
        () =>
          traverse_0(v_y_360, Fresh_0, {
            Bind_1: (e_5, ks_2186, k_1780) =>
              SHIFT(p_174, (k_1781, ks_2187, k_1782) =>
                Fresh_0.Fresh_1(ks_2187, (id_2, ks_2188) =>
                  RESUME(k_1781, (ks_2189, k_1783) =>
                    k_1783(new CVar_0(id_2), ks_2189), false, ks_2188, (v_r_1299, ks_2190) =>
                    k_1782(new CLet_0(id_2, e_5, v_r_1299), ks_2190))), ks_2186, k_1780, null)
          }, ks_2185, k_1779), null, null, null, ks_2182, (v_r_1300, ks_2191) =>
        RESET((p_175, ks_2192, k_1784) =>
          () =>
            traverse_0(v_y_361, Fresh_0, {
              Bind_1: (e_6, ks_2193, k_1785) =>
                SHIFT(p_175, (k_1786, ks_2194, k_1787) =>
                  Fresh_0.Fresh_1(ks_2194, (id_3, ks_2195) =>
                    RESUME(k_1786, (ks_2196, k_1788) =>
                      k_1788(new CVar_0(id_3), ks_2196), false, ks_2195, (v_r_1301, ks_2197) =>
                      k_1787(new CLet_0(id_3, e_6, v_r_1301), ks_2197))), ks_2193, k_1785, null)
            }, ks_2192, k_1784), null, null, null, ks_2191, (v_r_1302, ks_2198) =>
          () => k_1778(new CLet_0(v_y_362, v_r_1300, v_r_1302), ks_2198)));
  }
}

We can see that two of the pattern matching values are missing (inlined?), however, their names still appear unbound (v_y_361). Thus, two of the tests fail because of this.

@jiribenes
Copy link
Contributor

This issue ↑ seems similar to the problem detected in #561 (comment)

@dvdvgt
Copy link
Collaborator Author

dvdvgt commented Dec 3, 2024

Sadly, the fix proposed in #561 (comment) does not seem to fix the failing tests for anf.effekt.md and prettyprinter.effekt.md. I am still not quite sure why.

@dvdvgt
Copy link
Collaborator Author

dvdvgt commented Dec 20, 2024

The changes to the inliner seem to break some tests.

This was actually due to a faulty free variable function which caused some definitions used in continuations not be marked as used and consequently deleted as dead code. This was a fun debugging session 🙃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants