378790 - Add JS 1.5 strict mode.

Status: RESOLVED FIXED

(Rhino Graveyard :: Core, enhancement)

Description

[Bob Jervis]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1) Gecko/20061010 Firefox/2.0
Build Identifier: 

Rhino should support JS 1.5 strict mode as an option.  (Patch on its way).

Reproducible: Always

Steps to Reproduce:
1.
2.
3.



This is a new feature request that affects a bunch of specific cases.

Comment 1

[Bob Jervis]

Attachment: A patch that implements much of 'strict mode'

This defines two new FEATURE's: FEATURE_STRICT_MODE, which implements most of strict mode.  FEATURE_STRICT_VARS already implements one of the checks (and is separate from the new FEATURE_STRICT_MJODE, so technically you have to set them both to get the full effect).  There is also a FEATURE_WARNING_AS_ERROR that will cause warnings to be treated as errors in all cases.

There is a new command line flag for the Shell: -fatal_warnings to enable FEATURE_WARNING_AS_ERROR.  Also -strict now enables FEATURE_STRICT_MODE as well.

Note that FEATURE_WARNING_AS_ERROR will cause warnings to be treated as errors even if you do not enable warnings (with, for example, the -w flag on the Shell).

The parts of strict mode that are not implemented are: 

1. Some of the deprecated stuff was never supported by Rhino to begin with, so nothing was done to deprecate non-supported things.
2. For the return-without-value warnings, the code to check for whether a function falls through the bottom without a return statement is incomplete.  SpiderMonkey and Rhino have very different representations for switches and catch clauses than does Rhino.  I copied the SpiderMonkey checking code and patched up the differences for everything but those cases.  Thus, some switch statements (those with a default case, for example) that might otherwise warn about falling through will not warn.  Similarly with try/catches that have no finally clause.  The code for checking the individual catch clauses is missing.

Comment 2

[David Caldwell]

Hi Bob.  I was just going through the old E-mails about this earlier today, interestingly enough.  I've been thinking about refactoring the Context "features" into a separate class.  

This was inspired by the E4X implementation factory stuff, which doesn't neatly map onto a true-false model.  Strict mode maps better but it would make sense to have various aggregations that could be handled as a single object.  In another bug (I forget which offhand) there's a discussion about making the property mapping logic pluggable.  The code would not only get pretty ugly, the FEATURE_XXX motif doesn't cover well anything where there are more than two (or a potentially open-ended) number of options.

I know you've just been through this code, and just through doing some implementation based on it.  What are your thoughts?  Backward compatibility would be pretty easy to maintain, in that any Context/ContextFactory that overrode the current feature provision method would end up using a hidden Features implementation that delegated to that method, but everybody else could start implementing the new method.  But if I were to do this, it might at this point make sense to do it now -- before this patch is applied and adds even more features.

Thoughts?

Comment 3

[Norris Boyd]

Bob, I reviewed your patch and had some changes that I made, all relatively 
minor. I attached the changes. 

I haven't yet looked at the more complex parts of the code in detail.

David, I'm just now seeing your post and I'll think about that.

Comment 4

[Norris Boyd]

Attachment: strictMode2.patch

Comment 5

[Norris Boyd]

David, 

Where is the E4X implementation factory so I can take a look? 

Comment 6

[David Caldwell]

It's in Context and ContextFactory; see getE4xImplementationFactory in both of those (supplying a default implementation in ContextFactory).

It introduces a couple of problems with the current layout (and one can think of analogous ones for analogous situations).

First, someone can set FEATURE_E4X to true, but then fail to provide an E4X implementation, or vice-versa.  Especially in the former case, this means we can't trust FEATURE_E4X in our code (and you'll find in the initStandardObjects stuff that we in fact don't).  In the latter case, at least there's a sensible treatment (if FEATURE_E4X is false despite the presence of a factory, just don't use E4X).

Second, there are presently two implementations of E4X but there is at least some indication there may be a third floating around somewhere (some discussion on the newsgroup or in Bugzilla, I forget where, about how to compile without E4X because someone was using a hack to get their own in the classpath) and the intention is to allow people to plug in their own implementations.  Thus, there is no easy binary way to specify the desired implementation factory as a series of boolean features.

Comment 7

[Norris Boyd]

Since we've moved far afield from a discussion of implementing strict mode, I'm going to move my followup questions to an email thread. 

Comment 8

[Norris Boyd]

Checked in Bob's partial implementation of strict mode while we discuss the feature feature :-)

Checking in src/org/mozilla/javascript/CompilerEnvirons.java;
/cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/CompilerEnvirons.java,v  <--  CompilerEnvirons.java
new revision: 1.16; previous revision: 1.15
done
Checking in src/org/mozilla/javascript/Context.java;
/cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/Context.java,v  <--  Context.java
new revision: 1.255; previous revision: 1.254
done
Checking in src/org/mozilla/javascript/IRFactory.java;
/cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/IRFactory.java,v  <--  IRFactory.java
new revision: 1.104; previous revision: 1.103
done
Checking in src/org/mozilla/javascript/Node.java;
/cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/Node.java,v  <--  Node.java
new revision: 1.55; previous revision: 1.54
done
Checking in src/org/mozilla/javascript/Parser.java;
/cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/Parser.java,v  <--  Parser.java
new revision: 1.111; previous revision: 1.110
done
Checking in src/org/mozilla/javascript/ScriptOrFnNode.java;
/cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/ScriptOrFnNode.java,v  <-- ScriptOrFnNode.java
new revision: 1.21; previous revision: 1.20
done
Checking in src/org/mozilla/javascript/ScriptRuntime.java;
/cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/ScriptRuntime.java,v  <--  ScriptRuntime.java
new revision: 1.261; previous revision: 1.260
done
Checking in src/org/mozilla/javascript/regexp/NativeRegExp.java;
/cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/regexp/NativeRegExp.java,v <--  NativeRegExp.java
new revision: 1.101; previous revision: 1.100
done
Checking in src/org/mozilla/javascript/regexp/RegExpImpl.java;
/cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/regexp/RegExpImpl.java,v  <--  RegExpImpl.java
new revision: 1.35; previous revision: 1.34
done
Checking in src/org/mozilla/javascript/resources/Messages.properties;
/cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/resources/Messages.properties,v  <--  Messages.properties
new revision: 1.69; previous revision: 1.68
done
Checking in toolsrc/org/mozilla/javascript/tools/shell/Main.java;
/cvsroot/mozilla/js/rhino/toolsrc/org/mozilla/javascript/tools/shell/Main.java,v  <--  Main.java
new revision: 1.70; previous revision: 1.69
done
Checking in toolsrc/org/mozilla/javascript/tools/shell/ShellContextFactory.java;
/cvsroot/mozilla/js/rhino/toolsrc/org/mozilla/javascript/tools/shell/ShellContextFactory.java,v  <--  ShellContextFactory.java
new revision: 1.10; previous revision: 1.9
done

Comment 9

[Jürg Lehni]

Since I updated my local Rhino from CVS I get these errors:

java.lang.IllegalArgumentException: 11
	at org.mozilla.javascript.ContextFactory.hasFeature(ContextFactory.java:284)
	at org.mozilla.javascript.Context.hasFeature(Context.java:2142)
	at org.mozilla.javascript.CompilerEnvirons.initFromContext(CompilerEnvirons.java:72)
	at org.mozilla.javascript.Context.compileImpl(Context.java:2301)
	at org.mozilla.javascript.Context.compileReader(Context.java:1309)
	at org.mozilla.javascript.Context.compileReader(Context.java:1281)
	at org.mozilla.javascript.Context.evaluateReader(Context.java:1223)
	...

It turns out that CompilerEnvirons.initFromContext asks for both FEATURE_STRICT_MODE and FEATURE_WARNING_AS_ERROR, but ContextFactory.hasFeature does not return a value for these and throws an IllegalArgumentException instead.

I am not sure if this is the right place to report this, or if I should open a new bug.

The patch is simple:

Index: ContextFactory.java
===================================================================
RCS file: /cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/ContextFactory.java,v
retrieving revision 1.24
diff -r1.24 ContextFactory.java
278a279,281
> 
>           case Context.FEATURE_STRICT_MODE:
>               return false;
279a283,285
>           case Context.FEATURE_WARNING_AS_ERROR:
>               return false;
> 

Comment 10

[Attila Szegedi]

I committed the patch.

Comment 11

[Norris Boyd]

Reassign to Roshan to finish up the strict mode work.

Comment 12

[Roshan James]

Attachment: Path for no return value feature

The patch adds the missing features of Strict Mode to the code that is currently in the repository. This patch adds the check for final-return-value on javascript functions.

I have added cases that handle try-catch blocks and switch statements over Bob's previous patch.

Roshan

Comment 13

[Roshan James]

Attachment: Updated patch for return checks

This patch should address most of the requirements for return value checks for strict mode. 

js> function contrived(x) {
  while ( x < 10) {
    if ( x < 5) {
      return;
    }
    x = x + 1;
  }
 
if (x == 100)
  return x;
 else
   return x + 1;
}
js: warning: "<stdin>", line 23: return statement is inconsistent with previous usage
js:   return x;
js: ..........^
js: warning: "<stdin>", line 26: function contrived does not always return a value
js: }
js: ^

There are two possible warnings per function:
 1) The first inconsistent return statement of a function is flagged. Subsequent returns are not flagged. (I can flag all the subsequent returns if you choose)
 2) Overall if the function can have an error if there is any reachable return statement with a value and the function can end in some way (by a return or by dropping off the end) without a value.

Both warnings can be issued independent of each other:

js> function contrived(x) {
  while(true) {
    if (x > 10)
      return;
    x = x + 1
  }
  return 1;
}
js: warning: "<stdin>", line 15: return statement is inconsistent with previous usage
js:   return 1;
js: ..........^

The second warning is not issued here because the reachability analysis sees that the while loop will never break.

Here is the other case:

js> function (x) {
  if (x < 10)
    return 1;
}
js: warning: "<stdin>", line 16: anonymous function does not always return a value
js: }

Comment 14

[Norris Boyd]

Committed Roshan's changes with some modifications. I also updated some other
strict mode checks:

Checking in src/org/mozilla/javascript/Node.java;
/cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/Node.java,v  <--  Node.java
new revision: 1.56; previous revision: 1.55
done
Checking in src/org/mozilla/javascript/Parser.java;
/cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/Parser.java,v  <--  Parser.java
new revision: 1.112; previous revision: 1.111
done
Checking in src/org/mozilla/javascript/ScriptRuntime.java;
/cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/ScriptRuntime.java,v  <--  ScriptRuntime.java
new revision: 1.262; previous revision: 1.261
done
Checking in src/org/mozilla/javascript/resources/Messages.properties;
/cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/resources/Messages.properties,v  <--  Messages.properties
new revision: 1.70; previous revision: 1.69
done
Checking in toolsrc/org/mozilla/javascript/tools/resources/Messages.properties;
/cvsroot/mozilla/js/rhino/toolsrc/org/mozilla/javascript/tools/resources/Messages.properties,v  <--  Messages.properties
new revision: 1.26; previous revision: 1.25
done
Checking in toolsrc/org/mozilla/javascript/tools/shell/Main.java;
/cvsroot/mozilla/js/rhino/toolsrc/org/mozilla/javascript/tools/shell/Main.java,v  <--  Main.java
new revision: 1.71; previous revision: 1.70
done