HoriSun / closure-compiler

Automatically exported from code.google.com/p/closure-compiler
0 stars 0 forks source link

Change @nosideeffects to @throws on RegExp constructor extern #1288

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
Observe 
https://code.google.com/p/closure-compiler/source/browse/externs/es3.js#1889

As per http://www.ecma-international.org/ecma-262/5.1/#sec-15.10.4.1 , the 
constructor of the RegExp object may throw TypeError or SyntaxError (and 
therefore should not be @nosideeffects).

Recommend replacing this directive with @throws{(SyntaxError|TypeError)}

Change addresses code-stripping in the following usage pattern (the compiler 
will strip the "var unused" line, even though the RegExp constructor could 
throw as a side-effect):

function verifyOnClientAndSendRegexpToServer(pattern) {
  // Check client-side to save the round-trip. This will throw
  // if the pattern is not a valid RegExp. 
  var unused = new RegExp(pattern);
  sendRegexpToServer(pattern);
}

Original issue reported on code.google.com by mtomc...@google.com on 24 Mar 2014 at 9:05

GoogleCodeExporter commented 9 years ago
We should avoid unsounded optimizations as much as possible so in this case, I 
propose this:

1. Make compiler understand @nosideeffect should be dropped with @throw
2. Make a special pass that runs the regex checker to see if the pattern is 
valid. If so, optimze it like it is sideeffect free

Original comment by acle...@gmail.com on 24 Mar 2014 at 9:07

GoogleCodeExporter commented 9 years ago
As per the directions at 
https://code.google.com/p/closure-compiler/wiki/Contributors, I've attached the 
patch below.

(Note: If there's a unit test to add, I'd be happy to, but I didn't find any 
section of the unit tests that validates the extern type annotations on the 
Javascript core itself).

Diff generated via 'git diff ^HEAD HEAD' on my clone of the project.

Original comment by mtomc...@google.com on 9 Apr 2014 at 2:35

Attachments:

GoogleCodeExporter commented 9 years ago
As a googler you can also prepare a CL internally, which is easier to consume 
as it can be tested against our internal code base.

Original comment by concavel...@gmail.com on 10 Apr 2014 at 12:26

GoogleCodeExporter commented 9 years ago
Issue tracking has been migrated to github. Please make any further comments on 
this issue through https://github.com/google/closure-compiler/issues

Original comment by blic...@google.com on 1 May 2014 at 6:31

GoogleCodeExporter commented 9 years ago

Original comment by blic...@google.com on 1 May 2014 at 6:34