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

add robustness to generated loop code #841

Merged
merged 2 commits into from Feb 8, 2016

Conversation

Projects
None yet
3 participants
@rhendric
Collaborator

rhendric commented Feb 5, 2016

Certain expressions—integers (with or without a preceding +/-), null,
and void—when used as sources of for loops resulted in illegal
JavaScript being generated. While (42).length is a silly but legal thing
to do, 42.length is an actual SyntaxError in JS. This commit adds checks
to wrap potentially problematic expressions in parentheses.

src/ast.ls Outdated
@@ -2350,6 +2350,9 @@ class exports.For extends While
then "#idx #{ '<>'char-at pvar < 0 }#eq #tvar"
else "#pvar < 0 ? #idx >#eq #tvar : #idx <#eq #tvar"
else
if @source instanceof Literal and @source.value is /^[0-9]+$/

This comment has been minimized.

@vendethiel

vendethiel Feb 5, 2016

Contributor

This seems a bit too specific

This comment has been minimized.

@rhendric

rhendric Feb 5, 2016

Collaborator

I thought so at first too, but all of the following are handled gracefully by JavaScript:

  • 1e8
  • 0.0
  • true/false/null

This is the most general case I could find that triggers the syntax error. (Ah crap, I just found one more: negatives. I'll throw a -? in there.)

This comment has been minimized.

@vendethiel

vendethiel Feb 5, 2016

Contributor

(JS doesn't even have negative literals)

This comment has been minimized.

@rhendric

rhendric Feb 5, 2016

Collaborator

And yet -42.length triggers a syntax error anyway.

I'm starting to have misgivings about this approach; maybe I should be adding robustness (generating (42).length) instead of trying to catch the problem cases and raise errors? I was reluctant to do that at first because errors seem better than compiling things that I know are wrong, but this is turning into a slightly awkward patch. Do you have an opinion?

This comment has been minimized.

@vendethiel

vendethiel Feb 6, 2016

Contributor

Not really, but I never had this kind of error

rhendric added some commits Feb 7, 2016

add robustness to generated loop code
Certain expressions--integers (with or without a preceding +/-), null,
and void--when used as sources of for loops resulted in illegal
JavaScript being generated. While (42).length is a silly but legal thing
to do, 42.length is an actual SyntaxError in JS. This commit adds checks
to wrap potentially problematic expressions in parentheses.

@rhendric rhendric changed the title from catch integer literal loop sources to add robustness to generated loop code Feb 7, 2016

@rhendric

This comment has been minimized.

Collaborator

rhendric commented Feb 7, 2016

(For why-the-heck-does-he-want-this background: I was working on #162 and kept getting very annoying-to-diagnose errors from the test suite. I had to binary search the files with comments, because of course the JavaScript SyntaxErrors didn't give me a line number. Not fun. I'm just trying to help out the next guy who goes experimenting with loop, slice, or spread syntax.)

gkz added a commit that referenced this pull request Feb 8, 2016

Merge pull request #841 from rhendric/for-literal-error
add robustness to generated loop code

@gkz gkz merged commit fbc1502 into gkz:master Feb 8, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment