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. For real. #2337

Closed
wants to merge 4 commits into from
Closed

Use strict. For real. #2337

wants to merge 4 commits into from

Conversation

paulmillr
Copy link

This will auto-add 'use strict'; to every coffeescript file if —bare wasn't passed to options.

Closes #2335.

@paulmillr
Copy link
Author

Interesting, coffeescript code doesn't pass strict mode checker. Working on this.

failed 2 and passed 428 tests in 1.01 seconds 

  --bare 
  TypeError: Cannot assign to read only property 'length' of function (path) {
          return Module._load(path, _module, true);
        }
    at Object.eval (/Users/paul/Development/coffee-script/lib/coffee-script/coffee-script.js:125:25)
    at Function.<anonymous> (/Users/paul/Development/coffee-script/test/compilation.coffee:23:47)
    at /Users/paul/Development/coffee-script/Cakefile:219:12
    at Object.<anonymous> (/Users/paul/Development/coffee-script/test/compilation.coffee:19:3)
    at Object.<anonymous> (/Users/paul/Development/coffee-script/test/compilation.coffee:99:4)
    at Module._compile (module.js:441:26)
    at Object.run (/Users/paul/Development/coffee-script/lib/coffee-script/coffee-script.js:80:25)
    at /Users/paul/Development/coffee-script/Cakefile:303:22
    at /Users/paul/Development/coffee-script/Cakefile:98:14
    at ChildProcess.<anonymous> (/Users/paul/Development/coffee-script/Cakefile:46:16) 
  test/compilation.js: line unknown, column unknown 
  function () {
    eq(-1, CoffeeScript.compile('x = y', {
      bare: true
    }).indexOf('function'));
    return ok('passed' === CoffeeScript["eval"]('"passed"', {
      bare: true,
      filename: 'test'
    }));
  }

  #855: execution context for `func arr...` should be `null` 
  AssertionError: false == true
    at /Users/paul/Development/coffee-script/Cakefile:259:14
    at /Users/paul/Development/coffee-script/test/function_invocation.coffee:419:14
    at Function.<anonymous> (/Users/paul/Development/coffee-script/test/function_invocation.coffee:422:5)
    at /Users/paul/Development/coffee-script/Cakefile:219:12
    at Object.<anonymous> (/Users/paul/Development/coffee-script/test/function_invocation.coffee:416:3)
    at Object.<anonymous> (/Users/paul/Development/coffee-script/test/function_invocation.coffee:818:4)
    at Module._compile (module.js:441:26)
    at Object.run (/Users/paul/Development/coffee-script/lib/coffee-script/coffee-script.js:80:25)
    at /Users/paul/Development/coffee-script/Cakefile:303:22
    at /Users/paul/Development/coffee-script/Cakefile:98:14 
  test/function_invocation.js: line unknown, column unknown 
  function () {
    var array, contextTest;
    contextTest = function() {
      return eq(this, typeof window !== "undefined" && window !== null ? window : global);
    };
    array = [];
    contextTest(array);
    contextTest.apply(null, array);
    return contextTest.apply(null, array);
  }

@michaelficarra
Copy link
Collaborator

@paulmillr: Both of those are obvious failures. The first is because it's looking for a specific output. The second is because a null context in strict mode actually runs the function with a null context, not the global object.

@paulmillr
Copy link
Author

Yep, fixed.

@paulmillr
Copy link
Author

nope, not really. still fails browser tests because it has different execution context.

@paulmillr
Copy link
Author

@michaelficarra can test for #855 be removed at all? Doesn't seem to be very useful in our context.

@paulmillr
Copy link
Author

contextTest array # this is undefined
contextTest.apply undefined, array # this is undefined
contextTest array... # this is null

Should we pass undefined as execution context to func arr... for consistency with es5?

@@ -305,10 +305,11 @@ test "Prefix unary assignment operators are allowed in parenless calls.", ->
ok (func --val) is 5

test "#855: execution context for `func arr...` should be `null`", ->
contextTest = -> eq @, if window? then window else global
contextTest = ->
eq this, window ? global if this?
Copy link
Collaborator

Choose a reason for hiding this comment

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

This now no longer does the test at all in strict mode. That's not desirable.

@geraldalewis
Copy link
Contributor

Love strict, but I don't think it's a good idea to enable it by default. There are subtle semantic differences in strict mode that will trip people up if they're not watching out/are unaware they're using strict mode. For instance, in non-strict mode, most browsers bind arguments to elements on the arguments object e.g.:

((x) -> x = 2; arguments[0] is 2)(1) #true

and more problematic/far reaching: undefined is no longer coerced to global scope.

Amazon bitten by strict
Intel bitton by strict
Strict Quirks Mode

@paulmillr
Copy link
Author

@geraldalewis any other issues?

These were a long time ago and right now every stable version of every browser (ie isn't one) supports strict mode, so it would be quite hard to run into any of these in production env not spotting them in dev env.

@hikari-no-yume
Copy link

I think it's important to add use strict because forgetting it can make code do things you wouldn't always immediately expect, often erroneous, and worse still, continue plodding on until it hits some other error, making code difficult to debug. For instance, forgetting a new when using a constructor in non-strict mode isn't an error, and the program will often continue executing.

@paulmillr paulmillr mentioned this pull request Jul 3, 2012
@eventualbuddha
Copy link
Contributor

What's the status of this? I agree that it should not be the default, but I think it'd be great to have it as an option. Is that the consensus and we're just waiting for an updated patch, or is there some doubt about whether this should be included even as an option?

@myrne
Copy link

myrne commented May 19, 2013

Having wished for having "use strict" enabled for the first time of my life (chasing a global which turned out to be coming from a missing double arrow and doing @var = "value" somewhere), I would really like to have this as an option.

@codepunkt
Copy link

Whats the current state of this issue?

I want TypeErrors when trying to add properties to sealed objects - without 'use strict' statement, these fail silently.

Therefore i'd like to have an option that generates 'use strict' statements in the top-level function safety wrapper.

@hikari-no-yume
Copy link

Writing 'use strict'; at the top of a CoffeeScript file's really inelegant, and I don't even know if it works properly.

Would be nice if CS had a proper directive for it.

@JaKXz
Copy link

JaKXz commented Feb 8, 2016

so... the merge conflicts on this branch are probably way out of hand to easily resolve at this point... but would it be possible to re-use this work and make this a default thing?

@GeoffreyBooth
Copy link
Collaborator

In ES2015, strict mode is automatically enabled for ES modules or classes, so for many projects this shouldn’t be an issue anymore. In CS2, it is assumed that most people will probably be piping CoffeeScript’s output through Babel or a similar transpiler; if Babel is used, the strict mode transform can be enabled to automatically prepend 'strict mode' when necessary. I’m not sure this needs to be a responsibility of the CoffeeScript compiler.

Closing, but willing to reopen if people can convince the maintainers that this is something CoffeeScript should handle. At this point we have very limited resources for development, so in general if there’s something that Babel can do for us, we want to offload that responsibility onto Babel (hence CS2 outputs ES2015 and leaves the ES2015-to-ES5 conversion to Babel).

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.

Idea: auto-add 'use strict' if compiled with top-level wrapper
9 participants