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 bug in as capture #1981

Merged
merged 1 commit into from
Jul 5, 2017
Merged

Fix bug in as capture #1981

merged 1 commit into from
Jul 5, 2017

Conversation

Theodus
Copy link
Contributor

@Theodus Theodus commented Jun 23, 2017

Previously, the as sugar allowed unsafe captures through consume !, which could perform forbidden refcap conversion.

closes #1979

@Theodus Theodus added changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge triggers release Major issue that when fixed, results in an "emergency" release labels Jun 23, 2017
@Theodus Theodus mentioned this pull request Jun 23, 2017
Copy link
Member

@Praetonus Praetonus left a comment

Choose a reason for hiding this comment

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

Could you add a new test case demonstrating the fix?

Also, the actual rules for consume ! should be removed. This is in consume_single in alias.c:154. I think an assertion checking that ccap is a capability and not TK_ALIASED should be added as well in case there are other parts of the compiler using consume !.

I'd also like to have @sylvanc's opinion before merging since he probably knows why consume ! was added in the first place and we could be missing the point here by simply removing it.

@Theodus
Copy link
Contributor Author

Theodus commented Jun 26, 2017

One situation where removing this becomes problematic is that it removes a workaround for the situation in issue #1987 where this cannot be consumed to see the fields as trn or iso but the fields themselves are seen as those capabilities.

@Theodus Theodus added do not merge This PR should not be merged at this time and removed needs discussion during sync do not merge This PR should not be merged at this time labels Jun 26, 2017
@@ -461,6 +461,7 @@ bool expr_case(pass_opt_t* opt, ast_t* ast)
ast_error(opt->check.errors, pattern, "this pattern can never match");
ast_error_continue(opt->check.errors, match_type, "match type: %s",
ast_print_type(operand_type));
// TODO report unaliased type when body is consume !
Copy link
Member

Choose a reason for hiding this comment

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

Were you intending to resolve this TODO before merging?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried, but I couldn't figure out an easy way of reporting an 'unaliased' type.

Copy link
Member

Choose a reason for hiding this comment

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

This looks like another case where the "deferred error context" discussed during this week's sync and summarised in #1972 could be useful.

@Theodus Theodus dismissed Praetonus’s stale review June 29, 2017 22:42

The fix now reflects what was discussed during the sync meeting

TEST_ERRORS_1(src, "this capture violates capabilities");
}

TEST_F(BadPonyTest, AsUnaliased)
Copy link
Member

Choose a reason for hiding this comment

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

The intent of this test is unclear to me. The code compiles fine under the current version of the compiler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test protected me from accidentally adding an alias to the type. Two of my attempts to fix the error message mentioned in the TODO above passed all other tests but failed to compile the standard library.

@Theodus Theodus removed the request for review from sylvanc June 30, 2017 02:18
Copy link
Member

@Praetonus Praetonus left a comment

Choose a reason for hiding this comment

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

Looks good to me. There is a conflict in badpony.cc that must be resolved before this can be merged.

@Praetonus Praetonus merged commit dbc961f into ponylang:master Jul 5, 2017
ponylang-main added a commit that referenced this pull request Jul 5, 2017
@Theodus Theodus deleted the issue-1979 branch August 6, 2017 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge triggers release Major issue that when fixed, results in an "emergency" release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unsafe modification of val
3 participants