Prevent compiler crashes on certain `consume` expressions #3650

kapilash · 2020-09-10T19:23:07Z

consume (consume variable).field must result in similar compilation error as consume (consume variable).
consume f().field must result in similar compilation error as consume f()
(consume c).field must not result in a crash.

kapilash · 2020-09-10T19:25:11Z

While this does fix #3567 , this fix looks more like a hack. The generate_multidot_name function seems to have too rigid a view of its input.

ponylang-main · 2020-09-10T19:29:42Z

Hi @kapilash,

The changelog - fixed 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 3650.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.

SeanTAllen · 2020-09-11T02:35:17Z

@kapilash shall I add a "do not merge" label to this?

Also, are you aware of "draft PRs"?

I'm asking as you added WIP to the YouTube title, I assume to indicate that this shouldn't be merged or that it wasn't ready for review.

kapilash · 2020-09-11T02:55:54Z

@kapilash shall I add a "do not merge" label to this?

Also, are you aware of "draft PRs"?

I'm asking as you added WIP to the [] title, I assume to indicate that this shouldn't be merged or that it wasn't ready for review.

@SeanTAllen Thanks for asking.

I am sorry, I am not aware of a draft PR. I will look it up and send one if that's the way to go about such changes.

I know I picked a "good first issue" and the implementation of the fix does look like one. So may be the fix is indeed right?

But then, I am worried that I am introducing fragility into the system by fixing the symptoms and not the underlying cause. But I could be totally wrong.

So I was wondering if I should send out another commit into the same PR and let the reviewers pick the right solution?
A "do not merge" or "discussion needed" tags would be right perhaps.
I am a bit confused on how to proceed, tbh.

SeanTAllen · 2020-09-11T02:57:51Z

@jemc do you have any guidance for @kapilash ?

kapilash · 2020-09-11T03:18:07Z

It looks like I am doing syntactic checks in the type checker layer. That seems wrong ?.

Perhaps we should fix things like this, #3647 and #3463 in the syntax layer?

Alternatively, refactor generate_multidot_name and/or its callers to ensure that the invariants in the function are satisfied.

jemc · 2020-09-11T14:59:15Z

Perhaps we should fix things like this, #3647 and #3463 in the syntax layer?

Yes, I think that's probably the right choice. When the feature was added to allow consuming a field, that line in the syntax checker wasn't made to be restrictive enough - it allows any TK_DOT, but we should instead ensure that the it only allows TK_DOTs that should be processable by generate_multidot_name (e.g. a.b.c.d.e).

Also the error message was not updated when the feature was added to allow consuming a field - it still says that you can only consume a single identifier, but this is no longer true. It should probably say "a local identifier or a field".

* `consume (consume variable).field` must result in a similar compillation error as `consume (consume variable)`. * `consume f().field` must result in similar compilation error as `consume f()` * `(consume f).b` must be allowed and `f` should be moved. Fixes ponylang#3463, ponylang#3567 (and ponylang#3453)

kapilash · 2020-09-14T13:12:18Z

src/libponyc/pass/refer.c

@@ -139,14 +139,16 @@ static const char* generate_multi_dot_name(ast_t* ast, ast_t** def_found) {

    default:
    {
+      if (def == NULL)
+        return stringtab("");


@jemc , this is one change needed in generate_multi_dot_name.
I believe this

will do no harm to working pony code: The new code was added in a place where it is throwing an assert. So no working code will get affected.

is in sync with the rest of the method: if the parent ast node has null data, we are supposed to return empty string. That is possible when we have expressions wrapped in braces.

kapilash · 2020-09-14T13:17:48Z

src/libponyc/pass/syntax.c

-
+    case TK_DOT: {
+        AST_GET_CHILDREN(term, left, right);
+        if (ast_id(left) != TK_CALL && ast_id(left) != TK_SEQ) 


Here I went a bit too defensive and raising errors for known errors.
Mainly because, by allowing other illegal constructs, we get more detailed/friendlier errors down the line where we do lot more tree traversal anyway.

Do you suggest we go a bit more aggressive here? It could mean we will have to do go up and down the ast a bit more.

jemc

Thanks! Just a few small formatting fixes:

src/libponyc/pass/syntax.c

src/libponyc/pass/refer.c

Co-authored-by: Joe Eli McIlvain <joe.eli.mac@gmail.com>

SeanTAllen

A few small notes, requests on the release notes.

.release-notes/3650.md

SeanTAllen · 2020-09-14T17:43:12Z

.release-notes/3650.md

+  ```
+  must be equivalent to


Suggested change

```

must be equivalent to

must be equivalent to

SeanTAllen · 2020-09-14T17:44:05Z

.release-notes/3650.md

@@ -0,0 +1,17 @@
+## Graceful error handling on `consume` expressions
+
+The fix ensures 


Suggested change

The fix ensures

The fix ensures:

.release-notes/3650.md

SeanTAllen · 2020-09-14T17:47:10Z

.release-notes/3650.md

@@ -0,0 +1,17 @@
+## Graceful error handling on `consume` expressions


I think what is missing from these release notes is an that previously some of these things might have been compiler errors etc before. The fix ensures some things, but what does that mean for the end-user?

Something along the lines of "previously any of X would result in Y that has now been fixed" is an important part of the release notes messaging.

Rephrased. Can you please check if this sounds better? also I removed one of the points. I notice that 0.38.0 already covered it. (That particular fix was only refactored this time around).

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

SeanTAllen

Excellent release note update. Thanks!

SeanTAllen added the changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge label Sep 10, 2020

kapilash changed the title ~~Consistent handling for double consume~~ Consistent handling for double consume [WIP] Sep 11, 2020

SeanTAllen added the do not merge This PR should not be merged at this time label Sep 11, 2020

kapilash marked this pull request as draft September 11, 2020 03:10

kapilash force-pushed the refer/bug3567 branch from c1d1b0d to fea88c7 Compare September 11, 2020 17:57

kapilash changed the title ~~Consistent handling for double consume [WIP]~~ Prevent compiler crashes on certain consume expressions Sep 11, 2020

kapilash marked this pull request as ready for review September 11, 2020 18:00

kapilash force-pushed the refer/bug3567 branch from 17a7a6c to 567df59 Compare September 12, 2020 13:16

kapilash commented Sep 14, 2020

View reviewed changes

jemc reviewed Sep 14, 2020

View reviewed changes

src/libponyc/pass/syntax.c Outdated Show resolved Hide resolved

src/libponyc/pass/refer.c Outdated Show resolved Hide resolved

kapilash and others added 2 commits September 14, 2020 21:15

format fixes

7c84f1b

Co-authored-by: Joe Eli McIlvain <joe.eli.mac@gmail.com>

remove extra whitespace

c08a891

Co-authored-by: Joe Eli McIlvain <joe.eli.mac@gmail.com>

SeanTAllen reviewed Sep 14, 2020

View reviewed changes

kapilash and others added 2 commits September 14, 2020 23:19

Update .release-notes/3650.md

11cb58b

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

rephrasing the release notes

3fb6649

jemc approved these changes Sep 15, 2020

View reviewed changes

SeanTAllen approved these changes Sep 15, 2020

View reviewed changes

SeanTAllen merged commit ef56b0e into ponylang:master Sep 15, 2020

github-actions bot pushed a commit that referenced this pull request Sep 15, 2020

Updates release notes for PR #3650

848536c

github-actions bot pushed a commit that referenced this pull request Sep 15, 2020

Update CHANGELOG for PR #3650

59dddb6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent compiler crashes on certain `consume` expressions #3650

Prevent compiler crashes on certain `consume` expressions #3650

kapilash commented Sep 10, 2020 •

edited

Loading

kapilash commented Sep 10, 2020

ponylang-main commented Sep 10, 2020

SeanTAllen commented Sep 11, 2020

kapilash commented Sep 11, 2020

SeanTAllen commented Sep 11, 2020

kapilash commented Sep 11, 2020 •

edited

Loading

jemc commented Sep 11, 2020

kapilash Sep 14, 2020 •

edited

Loading

kapilash Sep 14, 2020

jemc left a comment

SeanTAllen left a comment

SeanTAllen Sep 14, 2020

SeanTAllen Sep 14, 2020

SeanTAllen Sep 14, 2020

kapilash Sep 14, 2020 •

edited

Loading

SeanTAllen left a comment

		@@ -0,0 +1,17 @@
		## Graceful error handling on `consume` expressions

		The fix ensures

Prevent compiler crashes on certain consume expressions #3650

Prevent compiler crashes on certain consume expressions #3650

Conversation

kapilash commented Sep 10, 2020 • edited Loading

kapilash commented Sep 10, 2020

ponylang-main commented Sep 10, 2020

SeanTAllen commented Sep 11, 2020

kapilash commented Sep 11, 2020

SeanTAllen commented Sep 11, 2020

kapilash commented Sep 11, 2020 • edited Loading

jemc commented Sep 11, 2020

kapilash Sep 14, 2020 • edited Loading

Choose a reason for hiding this comment

kapilash Sep 14, 2020

Choose a reason for hiding this comment

jemc left a comment

Choose a reason for hiding this comment

SeanTAllen left a comment

Choose a reason for hiding this comment

SeanTAllen Sep 14, 2020

Choose a reason for hiding this comment

SeanTAllen Sep 14, 2020

Choose a reason for hiding this comment

SeanTAllen Sep 14, 2020

Choose a reason for hiding this comment

kapilash Sep 14, 2020 • edited Loading

Choose a reason for hiding this comment

SeanTAllen left a comment

Choose a reason for hiding this comment

Prevent compiler crashes on certain `consume` expressions #3650

Prevent compiler crashes on certain `consume` expressions #3650

kapilash commented Sep 10, 2020 •

edited

Loading

kapilash commented Sep 11, 2020 •

edited

Loading

kapilash Sep 14, 2020 •

edited

Loading

kapilash Sep 14, 2020 •

edited

Loading