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

Use Strict #2021

Merged
merged 10 commits into from
Jan 17, 2012
Merged

Use Strict #2021

merged 10 commits into from
Jan 17, 2012

Conversation

geraldalewis
Copy link
Contributor

I wanted to get the ball rolling on #1547 "CoffeeScript use strict".

This patch does not enable strict mode. However, it does enforce strict mode's "early errors". These are sensible constraints that most JS devs adhere to anyway. As anecdotal evidence, almost no existing CoffeeScript compiler code needed to be rewritten to conform to strict mode.

Each "early error" gets its own discrete patch. Most commits are only a line or two; this isn't as big a change as it may appear. This pull is in the early stages of development, and there are surely some edge cases to address and some code to clean up: my intention is to engender discussion on adopting what we can of strict mode.

The following are now considered errors. Links are provided to the relevant commit.

7.8.3 Numeric Literals

oct = 0121            # octals

Commit 6a44f2c

B.1.2 String Literals

octEscSeq = "\121"    # octal escape sequences

Commit 3fb064f

11.1.5 Object Initialiser

x: 1, x: 1            # duplicate prop names in object literal

Commit b74efbf

11.4.1 The delete Operator

x = 1; delete x       # delete var
delete undefined      # delete undefined 
(x)-> delete x        # delete function argument 

Commit 6bbc5ae

13 Function Definition (13.1 Strict Mode Restrictions)

(x,x)->               # duplicate formal param

Commit 6c9ded8

7.6.1.2 Future Reserved Words

implements = 1        # assigning to future reserved keyword
interface = 1
let = 1
package = 1
private = 1
protected = 1
public = 1
static = 1
yield = 1

Commit 696d873

eval and arguments restrictions

12.14 The try Statement (12.14.1 Strict Mode Restrictions)

try e catch eval      # catch identifier eval
try e catch arguments # catch identifier arguments

13 Function Definition (13.1 Strict Mode Restrictions)

(eval)->              # identifier "eval" in formal params
(arguments)->         # identifier "arguments" in formal params
class eval            # function declaration `function eval(){}`
class arguments       # function declaration `function arguments(){}`

11.13.1 Simple Assignment

eval = 1              # assigning to eval
arguments = 1         # assigning to arguments

11.3.1 Postfix Increment Operator

eval++                # incrementing eval
arguments++           # incrementing arguments

Commit 423f54d


Tests a9cfd06


Thanks!

@jashkenas
Copy link
Owner

Gorgeous work -- looking forward to reading through these.

@jashkenas
Copy link
Owner

What's up with the backticks in the Cakefile? Isn't there another backtick-less way to fix 'em?

@jashkenas
Copy link
Owner

Other than that -- looks good to me. The one thing that folks might want to look at is the possibility of cleaning up the simpleNames implementation. There may be some refactors we can do to make that function a bit ... simpler.

@geraldalewis
Copy link
Contributor Author

@jashkenas thanks!

You know, I may not need the backticks. If memory serves, octal escape sequences can be at most 3 chars long (after the \), and though the string contains an octal escape, it also contains other chars. I'll look into it.

@paulmillr
Copy link

With the patch, fs.writeFile file, data, 0755, callback would throw error. What should we use instead of 0755?

@BrendanEich
Copy link

@paulmillr I <3 octal and Unix so I'm with you. with that API can you pass "0755" (quoted string)?

/be

@geraldalewis
Copy link
Contributor Author

@paulmillr I agree octal for file permissions is handy. However, you can use:

fs.writeFile file, data, parseInt("0755"), callback

or, if you know your code isn't going to be used with use strict:

fs.writeFile file, data, `0755`, callback

@geraldalewis
Copy link
Contributor Author

@jashkenas Agreed - I would love to hear feedback on simpleNames in 6c9ded8 . CoffeeScript's special params (@-params, arrays, objects, splats, defaults) made finding each param's actual name difficult. I'm hoping I missed something, and there's a simpler way.

@kriskowal
Copy link

@paulmillr parseInt("755", 8) or parseInt("0755") do the job, albeit verbosely. You might consider aliasing parseInt or wrapping it so you can oct("755"). CoffeeScript might consider preprocessing some radix literals so they can be digested by strict mode, though I would avoid the zero-prefix octal convention. Perhaps o755, b00111011, or arbitrary radix 8x755, 2x110111011, 16xffe.

@michaelficarra
Copy link
Collaborator

@BrendanEich: We're taking most of these rules straight from the ES5 spec. What was the thought process of TC39 when deciding to disallow octal literals in strict mode code? Was this use case never discussed?

@paulmillr: You should use 0b111101101. If we allowed underscores in number literals (#857, #632), it could be made more readable as 0b_111_101_101 (read/write/execute for owner/group/others).

@paulmillr
Copy link

@michaelficarra nice, 0b111_101_101 seems to be more readable to me.

@BrendanEich
Copy link

@michaelficarra TC39 was split when octal was banned, but to be fair, the main problem is people write 08 and 09 and want decimal, more often than you might think. Less fair: buncha Unix haters! :-P

/be

@satyr
Copy link
Collaborator

satyr commented Jan 9, 2012

$ coco -pe 8r755
493

@michaelficarra
Copy link
Collaborator

Other than the issues that have already been discussed, this LGTM. I'm excited to finally get this in.

@jashkenas: since this will introduce breaking changes, we should also bump the minor version to 1.3-pre (indicating the next release version will be 1.3), right?

@jashkenas
Copy link
Owner

Sure.

@geraldalewis
Copy link
Contributor Author

Great -- thanks guys! I'll pull together a revised commit sometime tomorrow.

@geraldalewis
Copy link
Contributor Author

Not to get ahead of myself -- just a heads up -- this branch needs to be rebased before merging :)

Big thanks to everyone for the review, and esp. @michaelficarra for the RegExp help, and @satyr for the tests.

The diff for the change to simpleName (getting the name(s) of a Param) looks much nicer without the diff noise

I think I may propose that octals be allowed with Python-esque 0o755 syntax, but it needs debate, so I'll save that for a new issue.

@michaelficarra
Copy link
Collaborator

LGTM. @jashkenas: I'd be willing to squash/rebase these commits when this pull has your approval.

@geraldalewis
Copy link
Contributor Author

FYI - I tried a rebase dry run earlier and I encountered a bug in git. Rebasing with the interactive flag fixed things, though. Happy to do it and push as a new branch if approved.

@jashkenas
Copy link
Owner

One last small discussion to have before merging it:

How would y'all (@michaelficarra, @geraldalewis, @satyr) feel about adding all of these errors, with the exception of leaving octal literals alone?

Forbidding octal literals doesn't fix their true danger -- which is inside of parseInt -- but does forbid use cases where they're handy, like for file permissions in Node.

It may not align quite as closely with where ES5 and 6 are heading, but there are some APIs designed around octal literals in ES3, which is what we're primarily concerned with here, and we should probably continue to support them.

@michaelficarra
Copy link
Collaborator

@jashkenas: I think that's a good idea. TC39 was split on the decision, but for us it seems more clear that different number representations are an appropriate sugar.

@satyr
Copy link
Collaborator

satyr commented Jan 12, 2012

adding all of these errors, with the exception of leaving octal literals alone?

I don't mind, but it'd be inconsistent to leave strict syntax errors.
Maybe convert them to decimal/hex forms? E.g.:

  • 07763
  • '\77''\x3f'

@geraldalewis
Copy link
Contributor Author

doesn't fix their true danger -- which is inside of parseInt

I totally agree. I like @satyr 's suggestion of simply converting them to decimal. We could also require prepending 0o to an octal just as Python does. This would give us some nice symmetry, keep us in conformance with strict, and impress that octals are sugar:

10    # decimal
0b10  # binary
0o10  # octal
0x10  # hexadecimal

@jashkenas
Copy link
Owner

Whoa, Gerald's idea is pretty neat. It would be quite unexpected for folks coming from JavaScript ... but at the same time, we'd be able to give them a good syntax error, and it would be very clean and consistent.

Perhaps 0b, 0o, 0x is something for TC39 to consider as well.

@michaelficarra
Copy link
Collaborator

I could live with 0o. @geraldalewis: Would unambiguous literals (00 through 07) be allowed? Would 077 produce an error like "decimal literals must not be prefixed with 0; octal literals must be prefixed with 0o" or something?

@geraldalewis
Copy link
Contributor Author

error like ... "octal literals must be prefixed with 0o"

I think that's a great idea --

  • In the unlikely event that a dev believes they're zero-padding a decimal number, they'll get an error
  • Developers who want to use octals for applications such as file permissions are informed that there's an unambiguous means to do so in CoffeeScript

Would unambiguous literals (00 through 07) be allowed?

I would say that for consistency's sake we aught to disallow them entirely in the 0[1-7] form.

@jashkenas
Copy link
Owner

I would say that for consistency's sake we aught to disallow them entirely in the 0[1-7] form.

Agreed.

@geraldalewis
Copy link
Contributor Author

As a proof of concept, I created a new branch for octal literals. You can see the commits here.

bin/coffee -bpe 'octal = 0o777'
var octal;
octal = 511;

...

bin/coffee -bpe 'octal = 0777'

SyntaxError: octal literals "0777" must be prefixed with "0o" on line 1

**Note_: the octals branch is based on the strict branch, so it should not be merged as is -- just pushed it to illustrate how octal literals could work._

Edit: Thanks for the link @michaelficarra :)

@michaelficarra
Copy link
Collaborator

A more helpful link: geraldalewis/coffee-script@strict...octals

@BrendanEich
Copy link

I will mail es-discuss about reviving octal literals. I expect some push-back based on O (uppercase O) being easily mistaken for 0, but it's not an issue if you require only lowercase o -- and anyway, using 007 instead of 0o7 or 0O7 would be caught at compile time with an early error.

Haven't had time to look: does the patch mentioned here support both upper- and lowercase o? Hex supports X and x.

/be

@michaelficarra
Copy link
Collaborator

@BrendanEich: maybe it would help es-discuss if you included a link to that PEP.

Thanks for the link, @showell. The rationale in that PEP is both comprehensive and convincing.

@BrendanEich
Copy link

@michaelficarra already did ;-).

/be

@geraldalewis
Copy link
Contributor Author

A (gentle) ping for @jashkenas and @michaelficarra. Just making sure the most recent patch didn't get lost in Friday's backbone bloodbath ;)

@michaelficarra
Copy link
Collaborator

@geraldalewis: LGTM. 21 commits is a little ridiculous, though. Can you rebase off master, squashing either all of the commits or all the separate features?

@michaelficarra
Copy link
Collaborator

I'm bothered by how none of the compiled JS is recognised as such by Github. @josh: is it because of our new "generated by" comment?

@geraldalewis: LGTM. Thanks a bunch for tackling this issue.

@jashkenas: Are you okay with merging this?

@geraldalewis
Copy link
Contributor Author

@michaelficarra Thanks so much for all the help!

Edit: nice -- thanks @josh!

@josh
Copy link
Contributor

josh commented Jan 16, 2012

@michaelficarra fixed 7974d23

@michaelficarra
Copy link
Collaborator

@josh: thanks

@jashkenas
Copy link
Owner

I can't give it a thorough run through, but I'll take y'alls word for it. Go for the merge.

@michaelficarra michaelficarra merged commit bf8e0aa into jashkenas:master Jan 17, 2012
@geraldalewis
Copy link
Contributor Author

Great! Thanks @jashkenas + @michaelficarra!

@paulmillr
Copy link

fucking nice. can't wait for 1.3.0.

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

Successfully merging this pull request may close these issues.

None yet

9 participants