Auto-recover constructor expressions #4124

chalcolith · 2022-05-24T15:42:47Z

This PR adds checks when assigning a constructor expression or passing one as a parameter to see if it can be auto-recovered, and if so, does not output a subtyping error. E.g. for the following code:

actor Main
  new create(env: Env) =>
    Bar.take_foo(Foo(88))
    let bar: Foo iso = Foo(88)

class Foo
  new create(v: U8) =>
    None

primitive Bar
  fun take_foo(foo: Foo iso) =>
    None

We'd previously get errors on both lines in Main.create(); whereas with this PR they are now accepted.

Fixes #702

SeanTAllen · 2022-05-24T15:48:30Z

test/libponyc/recover.cc

@@ -738,3 +738,19 @@ TEST_F(RecoverTest, CanWriteTrn_TrnAutoRecovery)

  TEST_COMPILE(src);
 }
+TEST_F(RecoverTest, CanAutorecover_Newref)


there should be a blank line here.

I think we said that we would add all tests that we can to runner tests in which case this should be a runner test as we are only checking that it compiles.

OK, I've moved it.

ponylang-main · 2022-05-24T15:49:55Z

Hi @kulibali,

The changelog - added label was added to this pull request; all PRs with a changelog label need to have release notes included as part of the PR. If you haven't added release notes already, please do.

Release notes are added by creating a uniquely named file in the .release-notes directory. We suggest you call the file 4124.md to match the number of this pull request.

The basic format of the release notes (using markdown) should be:

## Title

End user description of changes, why it's important,
problems it solves etc.

If a breaking change, make sure to include 1 or more
examples what code would look like prior to this change
and how to update it to work after this change.

Thanks.

.release-notes/4124.md

src/libponyc/type/cap.c

Co-authored-by: Sean T Allen <sean@seantallen.com>

jasoncarr0

Pretty good overall. There's some fixes needed for how the upgrading is done.

Otherwise more tests would be good (as I think was mentioned in sync). Some suggestions:

Annotate variable as iso, ref constructor with 0 arguments,
Same with 1 or more arguments
Annotated variable as val
Same as above with parameters
Verify that inference hasn't been broken by this, with code like the below:

let x = String
let y: String ref = x

jasoncarr0 · 2022-05-24T23:39:20Z

src/libponyc/expr/call.c

+    ast_t* wp_type;
+    if(check_auto_recover_newref(arg))
+    {
+      wp_type = unisolated(p_type);


This isn't quite the right behavior, although it might be on the right track. You should be upgrading the argument/constructor capability instead trying to downgrade the parameter.

That way for instance, a String val argument can be fulfilled by auto-recovery of String (which is a ref constructor). It would also be a bit clearer what's going on that way. Or for another example, the linked issue used array val

I believe I've fixed this now.

jasoncarr0 · 2022-05-24T23:40:38Z

src/libponyc/expr/call.c

+
+  token_id tcap = ast_id(cap_fetch(receiver_type));
+  if (tcap == TK_REF || tcap == TK_BOX)
+    return true;


It might be nice to instead eagerly return false if the capability doesn't match, that way you don't have to check the arguments if the return capability is already wrong.

jasoncarr0 · 2022-05-24T23:41:15Z

src/libponyc/expr/operator.c

+  if(check_auto_recover_newref(right))
+  {
+    wl_type = unisolated(fl_type);
+  }


Similar comment to above, the r_type should be upgraded instead, as with recover blocks.

Done as above.

- Change the rhs type instead of the lhs type when auto-recovering constructor calls - Bail out of the auto-recover check early if we don't need to auto-recover. - Add tests that make sure we still error out when passing non-sendable args to the constructor call we're trying to auto-recover.

chalcolith · 2022-06-11T00:21:38Z

I restarted the failing test and it succeeded.

SeanTAllen · 2022-06-11T01:39:00Z

What test failed and then passed when rerun?

chalcolith · 2022-06-11T17:10:44Z

Multiple libponyc tests failed on arm64 Apple Darwin. When I re-ran it they succeeded.

SeanTAllen · 2022-06-11T17:16:29Z

That's concerning. If it happens again let's record the failures in an issue. Actually, can you go through the CI history on cirrus and get the failures and open an issue?

SeanTAllen · 2022-06-12T00:23:21Z

@kulibali i've added issue #4138 for tracking failures in arm64 apple darwin tests. Please add the information about your failures to it.

jemc · 2022-06-14T18:32:36Z

Looks good to me. During sync Gordon waiting for @jasoncarr0 to review again.

chalcolith · 2022-07-03T20:36:26Z

Ping @jasoncarr0 will you have time to give this another review?

jasoncarr0 · 2022-07-04T16:18:40Z

@kulibali Yup, I've been looking at it just now, should get something in before tomorrow but I'm expecting a small number of changes

jasoncarr0

I have a few suggestions for additions, I've put them in a PR as it's hard to make for suggestions with all of it.

#4155

The only code change is a small cap function update that's necessary for val recovery to handle arguments correctly. We can talk about it in sync to clear anything up.

I.e. what I'm requesting (and providing):

More tests
Proper support for recovery to val

src/libponyc/expr/call.c

…02_ctor_autorecover

chalcolith · 2022-07-05T17:57:31Z

I have merged @jasoncarr0 's changes, thanks!

SeanTAllen · 2022-12-28T20:47:00Z

test/libponyc/recover.cc

@@ -738,3 +738,171 @@ TEST_F(RecoverTest, CanWriteTrn_TrnAutoRecovery)

  TEST_COMPILE(src);
 }
+TEST_F(RecoverTest, LetIso_CtorAutoRecovery)


I have pushed a fix for the incorrect spacing between functions in this diff.

chalcolith added 3 commits May 23, 2022 16:16

Autorecover constructor calls in arguments

60219da

Auto-recover constructor calls in assignments

bea586c

Remove leftover unused parameter

de55261

ponylang-main added the discuss during sync Should be discussed during an upcoming sync label May 24, 2022

SeanTAllen reviewed May 24, 2022

View reviewed changes

SeanTAllen added the changelog - added Automatically add "Added" CHANGELOG entry on merge label May 24, 2022

chalcolith added 2 commits May 24, 2022 09:13

Move test to be a runner test

6656a03

Add release notes

bc67a7c

SeanTAllen reviewed May 24, 2022

View reviewed changes

.release-notes/4124.md Outdated Show resolved Hide resolved

SeanTAllen reviewed May 24, 2022

View reviewed changes

.release-notes/4124.md Outdated Show resolved Hide resolved

ergl reviewed May 24, 2022

View reviewed changes

src/libponyc/type/cap.c Show resolved Hide resolved

chalcolith and others added 2 commits May 24, 2022 11:13

Update .release-notes/4124.md

8f6f695

Co-authored-by: Sean T Allen <sean@seantallen.com>

Update .release-notes/4124.md

1755649

Co-authored-by: Sean T Allen <sean@seantallen.com>

jasoncarr0 suggested changes May 24, 2022

View reviewed changes

chalcolith added do not merge This PR should not be merged at this time and removed discuss during sync Should be discussed during an upcoming sync labels May 31, 2022

ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Jun 10, 2022

chalcolith added do not merge This PR should not be merged at this time and removed do not merge This PR should not be merged at this time labels Jun 10, 2022

Disallow trying to autorecover to a complex type

2c5535c

chalcolith requested a review from jasoncarr0 June 11, 2022 00:02

chalcolith removed the do not merge This PR should not be merged at this time label Jun 11, 2022

chalcolith mentioned this pull request Jun 14, 2022

Arm64 Apple Darwin CI test failures #4138

Closed

jemc removed the discuss during sync Should be discussed during an upcoming sync label Jun 14, 2022

ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Jun 14, 2022

SeanTAllen removed the discuss during sync Should be discussed during an upcoming sync label Jun 14, 2022

ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Jul 3, 2022

Add tests; support val better

c5113e7

jasoncarr0 suggested changes Jul 4, 2022

View reviewed changes

src/libponyc/expr/call.c Outdated Show resolved Hide resolved

chalcolith added 2 commits July 5, 2022 10:43

Rename parameter

a53ea16

Merge remote-tracking branch 'jaoncarr0/702_suggestions' into issue_7…

8f6d5a1

…02_ctor_autorecover

chalcolith mentioned this pull request Jul 5, 2022

Suggestions for #4124: Add tests; support val better #4155

Merged

jemc approved these changes Jul 5, 2022

View reviewed changes

jasoncarr0 approved these changes Jul 5, 2022

View reviewed changes

chalcolith merged commit 7b01704 into main Jul 5, 2022

chalcolith deleted the issue_702_ctor_autorecover branch July 5, 2022 18:38

ponylang-main removed the discuss during sync Should be discussed during an upcoming sync label Jul 5, 2022

github-actions bot pushed a commit that referenced this pull request Jul 5, 2022

Update CHANGELOG for PR #4124

922350a

github-actions bot pushed a commit that referenced this pull request Jul 5, 2022

Updates release notes for PR #4124

06a9542

SeanTAllen mentioned this pull request Dec 28, 2022

Direct writing to a field of a val object #4282

Closed

SeanTAllen reviewed Dec 28, 2022

View reviewed changes

ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Dec 28, 2022

jasoncarr0 mentioned this pull request Feb 20, 2024

Segfault when trying to compile incorrect capabilities #4344

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Auto-recover constructor expressions #4124

Auto-recover constructor expressions #4124

chalcolith commented May 24, 2022 •

edited

Loading

SeanTAllen May 24, 2022

SeanTAllen May 24, 2022

chalcolith May 24, 2022

ponylang-main commented May 24, 2022

jasoncarr0 left a comment

jasoncarr0 May 24, 2022 •

edited

Loading

chalcolith Jun 10, 2022

jasoncarr0 May 24, 2022

chalcolith Jun 10, 2022

jasoncarr0 May 24, 2022

chalcolith Jun 10, 2022

chalcolith commented Jun 11, 2022

SeanTAllen commented Jun 11, 2022

chalcolith commented Jun 11, 2022

SeanTAllen commented Jun 11, 2022

SeanTAllen commented Jun 12, 2022

jemc commented Jun 14, 2022

chalcolith commented Jul 3, 2022

jasoncarr0 commented Jul 4, 2022

jasoncarr0 left a comment •

edited

Loading

chalcolith commented Jul 5, 2022

SeanTAllen Dec 28, 2022

Auto-recover constructor expressions #4124

Auto-recover constructor expressions #4124

Conversation

chalcolith commented May 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ponylang-main commented May 24, 2022

jasoncarr0 left a comment

Choose a reason for hiding this comment

jasoncarr0 May 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chalcolith commented Jun 11, 2022

SeanTAllen commented Jun 11, 2022

chalcolith commented Jun 11, 2022

SeanTAllen commented Jun 11, 2022

SeanTAllen commented Jun 12, 2022

jemc commented Jun 14, 2022

chalcolith commented Jul 3, 2022

jasoncarr0 commented Jul 4, 2022

jasoncarr0 left a comment • edited Loading

Choose a reason for hiding this comment

chalcolith commented Jul 5, 2022

Choose a reason for hiding this comment

chalcolith commented May 24, 2022 •

edited

Loading

jasoncarr0 May 24, 2022 •

edited

Loading

jasoncarr0 left a comment •

edited

Loading