{"action":"created","issue":{"url":"https://api.github.com/repos/probmods/webppl/issues/388","repository_url":"https://api.github.com/repos/probmods/webppl","labels_url":"https://api.github.com/repos/probmods/webppl/issues/388/labels{/name}","comments_url":"https://api.github.com/repos/probmods/webppl/issues/388/comments","events_url":"https://api.github.com/repos/probmods/webppl/issues/388/events","html_url":"https://github.com/probmods/webppl/pull/388","id":145140773,"number":388,"title":"Make arguments work in all contexts.","user":{"login":"null-a","id":9109012,"avatar_url":"https://avatars.githubusercontent.com/u/9109012?v=3","gravatar_id":"","url":"https://api.github.com/users/null-a","html_url":"https://github.com/null-a","followers_url":"https://api.github.com/users/null-a/followers","following_url":"https://api.github.com/users/null-a/following{/other_user}","gists_url":"https://api.github.com/users/null-a/gists{/gist_id}","starred_url":"https://api.github.com/users/null-a/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/null-a/subscriptions","organizations_url":"https://api.github.com/users/null-a/orgs","repos_url":"https://api.github.com/users/null-a/repos","events_url":"https://api.github.com/users/null-a/events{/privacy}","received_events_url":"https://api.github.com/users/null-a/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":2,"created_at":"2016-04-01T10:08:21Z","updated_at":"2016-04-17T17:15:32Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/probmods/webppl/pulls/388","html_url":"https://github.com/probmods/webppl/pull/388","diff_url":"https://github.com/probmods/webppl/pull/388.diff","patch_url":"https://github.com/probmods/webppl/pull/388.patch"},"body":"This is intended to fix #31.\r\n\r\nAs suggested in the ticket, I've moved the `varargs` transform before `cps`. I side stepped the stack overflow which results from doing this by switching the strategy used to find a node's enclosing function. Specifically, we now maintain a stack of function nodes as we traverse, rather than annotate the AST. With that, the failing test passes.\r\n\r\nOne thing I'm not sure about is whether I've done the right thing with the tests. I've changed `transformAstVarargs` so that `varargs` runs before `cps` in the var args tests, and by doing it this way I don't think I've changed any other tests. (e.g. by including the `varargs` transform in tests that previously didn't include it.) Does this seems right, or does changing the order of the transforms require us to make more substantial changes to the tests?\r\n\r\nI'm not entirely sure whether the existing `seenByVarargs` check is necessary any more. I can't quite fathom why it was needed originally, so for now I've done the conservative thing and left it as is.\r\n\r\n(This probably isn't top priority, but I've had it laying around for a while and figured I should clean it up and make a PR.)"},"comment":{"url":"https://api.github.com/repos/probmods/webppl/issues/comments/211061561","html_url":"https://github.com/probmods/webppl/pull/388#issuecomment-211061561","issue_url":"https://api.github.com/repos/probmods/webppl/issues/388","id":211061561,"user":{"login":"null-a","id":9109012,"avatar_url":"https://avatars.githubusercontent.com/u/9109012?v=3","gravatar_id":"","url":"https://api.github.com/users/null-a","html_url":"https://github.com/null-a","followers_url":"https://api.github.com/users/null-a/followers","following_url":"https://api.github.com/users/null-a/following{/other_user}","gists_url":"https://api.github.com/users/null-a/gists{/gist_id}","starred_url":"https://api.github.com/users/null-a/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/null-a/subscriptions","organizations_url":"https://api.github.com/users/null-a/orgs","repos_url":"https://api.github.com/users/null-a/repos","events_url":"https://api.github.com/users/null-a/events{/privacy}","received_events_url":"https://api.github.com/users/null-a/received_events","type":"User","site_admin":false},"created_at":"2016-04-17T17:15:32Z","updated_at":"2016-04-17T17:15:32Z","body":"> Out of curiosity, why did moving varargs before cps result in a stack overflow again?\r\n\r\nI don't know. The stack overflow comes from a recursive call to esmangle's `deepCopyInternal` within the `optimize` transform. How it is that the setting `parentNode` and the extra transforms are interacting to cause this is a mystery to me.\r\n\r\n> One principle we could follow with the transform tests is to test each transform in the context that it is actually used in\r\n\r\nI think this is tricky to do in general because for the `varargs` tests we need to run `cps` and `store` after the `varargs` transform. (Because the `varargs` transform slices the original JS `arguments` in a way that assumes they includes the store, continuation and address.) You can imagine doing something about this of course, but I'm not sure it buys us much?\r\n\r\n> we'd want to include the varargs transform among the transforms that are applied in the cps, store, and optimize tests\r\n\r\nI've done this bit though.\r\n\r\n> feel free to remove it.\r\n\r\nDone."}}