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

yield and await don't work in let blocks #1019

Closed
pepkin88 opened this issue Mar 21, 2018 · 9 comments
Closed

yield and await don't work in let blocks #1019

pepkin88 opened this issue Mar 21, 2018 · 9 comments
Labels

Comments

@pepkin88
Copy link
Contributor

pepkin88 commented Mar 21, 2018

yield

Example:

fn = ->*
  let a = 1
    yield a

Result:

var fn;
fn = function*(){
  return (function(a){
    return (yield a);
  }.call(this, 1));
};

Expected result:

var fn;
fn = function*(){
  return (yield* (function*(a){
    return (yield a);
  }.call(this, 1)));
};

await

Example:

fn = ->>
  let a = Promise.resolve 1
    await a

Result:

var fn;
fn = async function(){
  return (function(a){
    return (await a);
  }.call(this, Promise.resolve(1)));
};

Expected result:

var fn;
fn = async function(){
  return await (async function(a){
    return await a;
  }.call(this, Promise.resolve(1)));
};

(AFAIK, unlike yield, await doesn't need wrapping in parentheses, !await something works fine)


Sidenote:

This is the first issue of many I'm going to post. Way back I announced, that I am planning to start posting some pull requests for the bugs I've found (#903 (comment)), but ended up posting nothing. I even was waiting so long, before I started doing something to fix #1005, despite knowing about this bug for years.

So this time I decided to dump my list of bugs and feature proposals one at the time, at least once a day. I hope I'll find some time and will to learn the internals to help more, by making pull requests. But "done is better than perfect". I have procrastinated for so long and I see, that LiveScript needs a help (with bugfixes, with tools, with promotion, with rewriting), and it hurts me, that I just wait and do nothing about it.

Feel free to post suggestions about how I can be more helpful, given low time resources.
I tried learning how the compiler works, but it's quite complicated and requires more analysis, so I've postponed it. Do you know some good resources to learn how to get into such code?

And I'm sorry for any duplicates. I'm not familiar with the whole issue pool, it's quite daunting.

And don't wait for me with 1.6.0. These all can be included in future versions.

@pepkin88 pepkin88 changed the title yield doesn't work in let blocks yield and await don't work in let blocks Mar 21, 2018
@rhendric
Copy link
Collaborator

Re: the issue, good catch and I agree with your expectations.

Re: the sidenote, first of all, continuing to contribute high quality bug reports like this one would indeed be helpful, so thank you! However, the number of people currently contributing (capable of contributing?) good PRs is very low, and if you are interested in taking the time to learn more about hacking LiveScript, I would be interested in taking the time to help you through that and producing resources for you and other future contributors. This issue in particular I think would be a good candidate for a tutorial—it has a fix that is fairly small and clear but involves picking up some context before it becomes obvious. I think the ideal thing, if you're willing, would be for you to write the fix for this with hints and guidance from me, and then I could take the transcript of our interactions and annotate it into a compiler hacking tutorial on the LiveScript wiki. Does that sound like something you'd be interested in doing? (Alternatively, I could write the fix myself and try to document my thought process alone, but I don't know if that would be as good at covering the things a newbie needs to know.)

@vendethiel
Copy link
Contributor

vendethiel commented Mar 21, 2018

If possible, the former would definitely be better :-)! LS could use a few more contributors.

@vendethiel
Copy link
Contributor

it has a fix that is fairly small and clea

Call.let doesn't keep around the information for compile-closure to be used, though, right? So maybe a tiny bit involved.

@rhendric
Copy link
Collaborator

No spoilers! 😄 (I stand by my original characterization, but I guess we'll see?)

@pepkin88
Copy link
Contributor Author

Thanks for the offer. Yes, that sounds interesting, I'll appreciate the help and guidance.
Right now I know very little. I managed to add a feature (related to semiautovivification, thus rewriters), but I failed at adding another (I don't really know what would it be related to, I guess lexer, maybe grammar definition). I tried to analyze grammar.ls, some things made sense, some didn't. It was quite some time ago, I have to start analyzing again.

But about that, where should I start? You suggested, that I could start from fixing this bug. And I agree, with trial and error I think I'll be able to patch things. In fact, I believe I fixed the yield part, seemed easier (while doing it I've found 2 more bugs, maybe known). But it was more of a guess work. I could really use some guidelines, what should I grasp first, what are some important things to keep in mind, what about some gotchas, maybe a general overview, I too don't really know :).

About communication, what platform would you suggest? And by you I mean all of you, the more the merrier :)

Regarding my time, currently I have little to no time left for this, next month should be more relaxed, we'll see. I still plan to keep posting issues, those are less time consuming. And perhaps, in the future I will be fixing them myself.

@rhendric
Copy link
Collaborator

Excellent! How about this: open a working PR with just (failing) tests for this bug (where every good bug fix should start!). Make sure to add the tests to test/generators.ls and test/async.ls—usually you would want to add tests to the file associated with the most significant language structure (for example, most of the let tests are in test/function.ls), but in this case, both generators and async/await are optional features and so the tests need to be in those specific files, so that the test script runs or doesn't run them as appropriate. Make sure make test-harmony fails both files. Also, be creative and add a few cases that you think might be tricky.

Once you've done that, I'll comment on the PR with further tips on how to start diagnosing the problem and what parts of the compiler code to look at.

Re: time, feel free to do all of this on whatever schedule works for you—there's no rush, my offer isn't going anywhere.

@vendethiel
Copy link
Contributor

No spoilers! 😄 (I stand by my original characterization, but I guess we'll see?)

I think I got it now :).

@rhendric
Copy link
Collaborator

@pepkin88, are you still interested in working through a fix for this? I would like it to be fixed for the 1.6 release, so if you don't think you can get to it in the next month, I'll use my own fix.

@pepkin88
Copy link
Contributor Author

@rhendric Sorry for a silence from my part. Yes, I'm still interested, I'm working toward organizing for myself some time for this. But in case I can't proceed with it, I'll let you know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants