Add JS 1.5 strict mode.

RESOLVED FIXED in 1.6R6

Status

Rhino
Core
--
enhancement
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: Bob Jervis, Assigned: Roshan James)

Tracking

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

11 years ago
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.
(Reporter)

Comment 1

11 years ago
Created attachment 262804 [details] [diff] [review]
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

11 years ago
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?
Assignee: nobody → bjervis
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 3

11 years ago
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

11 years ago
Created attachment 262834 [details] [diff] [review]
strictMode2.patch
Attachment #262804 - Attachment is obsolete: true

Comment 5

11 years ago
David, 

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

Comment 6

11 years ago
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

11 years ago
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

11 years ago
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

10 years ago
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

10 years ago
I committed the patch.

Updated

10 years ago
Target Milestone: --- → 1.6R6

Comment 11

10 years ago
Reassign to Roshan to finish up the strict mode work.
Assignee: bjervis → roshanj
(Assignee)

Comment 12

10 years ago
Created attachment 265858 [details] [diff] [review]
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
(Assignee)

Updated

10 years ago
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Comment 13

10 years ago
Created attachment 266826 [details] [diff] [review]
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: }
Attachment #265858 - Attachment is obsolete: true

Comment 14

10 years ago
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
You need to log in before you can comment on or make changes to this bug.