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

Downgrade iso and trn to ref when replacing this in a box method. #2503

Merged
merged 3 commits into from
Nov 7, 2018

Conversation

plietar
Copy link
Contributor

@plietar plietar commented Jan 17, 2018

Fixes #1887

@jemc
Copy link
Member

jemc commented Jan 17, 2018

@plietar - did you come to a conclusion about this concern from this comment (#1887 (comment))?

Worth noting, this may interact with (auto) recovery, such that despite decaying to ref you can still get an iso back.


if(ast_id(find) == TK_FUN && ast_id(ast_child(find)) == TK_BOX)
orig = downcast_iso_trn_receiver_to_ref(orig);

return deferred_reify_new(find, typeparams, typeargs, orig);
}
Copy link
Member

Choose a reason for hiding this comment

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

This needs an ast_free call with orig if downcast_iso_trn_receiver_to_ref did a copy, as deferred_reify_new also does a copy. Another possibility would be to add a new version of deferred_reify_new that doesn't copy the AST for the this type and use it in that case.

@jemc
Copy link
Member

jemc commented Mar 21, 2018

This has been stalled for a while, and I'd like to see it move forward to fix the soundness issue.

@plietar - can you add the requested ast_free, or if you don't have time to do it, let us know that and maybe someone else can take over bringing this to completion?

(It also has merge conflicts at this point, but those look simple to resolve).

@dipinhora
Copy link
Contributor

@plietar due to the recent releases and their related changes to the CHANGELOG file, please merge or rebase onto master and adjust your CHANGELOG entries into the appropriate area of "unreleased" in the CHANGELOG file.

@dipinhora
Copy link
Contributor

@plietar due to the release of 0.24.1 and its related changes to the CHANGELOG file, please merge or rebase onto master and adjust your CHANGELOG entries into the appropriate area of "unreleased" in the CHANGELOG file.

@mfelsche
Copy link
Contributor

@plietar or anyone else in @ponylang/committer we have another user hitting an issue that is related to #1875 which has a fix #1888 but we agreed on fixing #1887 first.

The related new issue is #2881

How are the chances someone finds time for this one?
Or @plietar or @Praetonus would anyone of you have 5 minutes left to explain to someone like me (in here or IRC or slack) what needs to be done to finish this up, so I can proceed on this one?

@jemc
Copy link
Member

jemc commented Sep 26, 2018

Discussed this in today's sync call. @sylvanc reasoned that this does not cause problems with auto-recovery, and ratified the change.

All this needs is the missing ast_free, and to fix merge conflicts then it can be merged.

@mfelsche
Copy link
Contributor

I took the freedom to rebase this branch onto current master and add the ast_free @Praetonus mentioned and pushed that to your form @plietar

@mfelsche mfelsche added bug: 4 - in progress triggers release Major issue that when fixed, results in an "emergency" release and removed needs discussion during sync labels Sep 28, 2018
@jemc
Copy link
Member

jemc commented Nov 6, 2018

Fixed the CHANGELOG merge conflict. Letting the CI run to see how it does against latest masterchanges, then this is ready to merge.

@mfelsche
Copy link
Contributor

mfelsche commented Nov 7, 2018

Thanks @jemc !

Everything is green, merging.

@mfelsche mfelsche merged commit 18305fb into ponylang:master Nov 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

6 participants