Status of ihab/sharply
ihab...@gmail.com
This is a summary of the work that MarkM and I did on the "ihab/sharply" branch today. We isolated a bunch of issues and fixed a bunch more. The tests were all green a little earlier, and I will work to make them go back to green asap; and re-snapshot. This should be quite easy.
We now consider the code to be reviewed by MarkM. Mike Samuel agreed to review the tests, which I will entreat him to do as soon as the tests are green again. At this point, I will check in so as to unblock everything else.
The below is pretty much a braindump so as to get this information off of scraps of paper; copying the list fyi.
* * * * *
TODO: For static "warts mode" switch, need to complete the command line flags and hookup to PluginMeta.
TODO: Add a dynamic "warts mode" switch to caja.js. If a module was compiled with warts mode, caja.js alllows it to run iff the warts mode runtime switch is turned on.
TODO: Use warts mode switch to turn POE on or off in caja.js.
TODO: Add explicit xo4a support to caja.js. (Assuming we are keeping it for now, pace David-Sarah Hopwood's arguments.)
TODO: Add support in caja.js for the "caja.manifest(...)" operation.
TODO: Cajoler module format must change to the new JSON-literal format in order to capture warts mode.
TODO: Break up the rules prior to Dave Wagner review in order to foster readability.
BUG: Multiple function declarations with the same name are allowed. The following is not rejected:
function foo() {}
BUG: Assigning to a function name is disallowed, but with an unhelpful error (essentially falls of the end without having matched any rules). This is triggered by, e.g., the following:
foo = 3;
BUG: We are catching the following erroneous structure in the wrong place (but we are catching it, so we are failing safe):
foo++;
TODO: Event handler functions, since they are SYNTHETIC, are not properly traversed in scope analysis -- they are treated as though they were plain blocks -- so the output is not correct. (Specifically: event is treated as though it is a free module variable, not a function parameter, and the translation of arguments to a___ is not done.) We can fix this by specific hacks on the symbols in the function body, but we would prefer to simply make the actual generated FunctionConstructor node non-SYNTHETIC so that scope analysis in the rewriter automatically does the right thing.
TODO: Add a rewriter step where single-statement control flow bodies are normalized. This is a resurrection of Issue 136. The following throws a ReferenceError in the browser (in SquareFree on FF3). When cajoled, the declaration and definition of foo is hoisted to the enclosing block and thus visible throughout, which is wrong. We would like that, at the point of call, foo should at most be visible as undefined:
foo();
BUG: A Caja programmer has no way to declare a "final" constructor that cannot be subclassed.
BUG: We do not enforce that a subclass constructor must call super to ensure that the superclass fields are properly initialized.
Ihab
--
Ihab A.B. Awad, Palo Alto, CA
David-Sarah Hopwood
> *TODO:* Add a rewriter step where single-statement control flow bodies are
> normalized. This is a resurrection of Issue
> The following throws a ReferenceError in the browser (in SquareFree on FF3).
> When cajoled, the declaration and definition of foo is hoisted to the
> enclosing block and thus visible throughout, which is wrong. We would like
> that, at the point of call, foo should at most be visible as undefined:
>
> if (false) function foo() { print('hi'); }
> foo();
What Jacaranda does to avoid having to deal with issues like this, is to
restrict substatements of control flow constructs (if, do, while, for) to
a "simple statement" or block:
<SimpleStatementOrBlock> :
<ExpressionStatement> intersect <AssignmentExpression>
<ContinueStatement>
<BreakStatement>
<ReturnStatement>
<ThrowStatement>
<Block>
<IfOrSimpleStatementOrBlock> :
<IfStatement>
<SimpleStatementOrBlock>
<IfStatement> :
if ( <Expression> ) <SimpleStatementOrBlock> else
<IfOrSimpleStatementOrBlock>
if ( <Expression> ) <SimpleStatementOrBlock>
(and similarly for <IterationStatement>).
Since simple statements cannot declare anything, the effect is that
a substatement can only declare things within a block, and so it is
clear to a human reader what the scope of those things will be.
This is like JSLint's "Required Blocks" restriction documented at
<http://www.jslint.com/lint.html>, but it is more permissive about
substatements that are unproblematic in respect of scoping, and
commonly written without curlies in real code. Whether to include
'<ExpressionStatement> intersect <AssignmentExpression>'
(i.e. function calls, assignments etc.) as a simple statement is
a matter of taste.
--
David-Sarah Hopwood
Mike Samuel
144 // TODO(ihab.awad): Rewrite "golden" or turn into a functional test
145 if (false) {
I don't think you can turn the dynamic style stuff into a functional
test without implementing CSS support in env.js
changes/ihab/sharply/trunk/src/javatests/com/google/caja/opensocial/service/CajolingServiceTest.java@1484
102 "{ var x = y; }",
103 DefaultCajaRewriterTest.weldPrelude("y") +
104 "var x0___;" +
105 "{ x = y; }");
This seems to say
- import y into local scope
- define a temp var
- assign imported variable y to local variable x
Shouldn't this be
- import y into local scope
- define a var x
- assign imported variable y to local variable x
changes/ihab/sharply/trunk/src/javatests/com/google/caja/parser/quasiliteral/DefaultCajaRewriterTest.java@1484
90 // TODO(ihab): SECURITY: Test for whether references to global
'this' are rejected statically.
"for whether" -> "whether"
123 "function foo() {this;}" +
124 "caja.def(foo, Object);" +
125 "function bar() {this;}" +
126 "caja.def(bar, foo);" +
127 "var b=new bar;" +
128 "caja.extend(Object, {x:1});" +
Do we now require that all variables be declared with var?
Indentation on lines 454-461.
750: foo.x = 3;
What does "// iok" mean?
Maybe assertEquals(foo.x, 3);
Are there any tests that assertEquals works cajoled, e.g. that
assertEquals(1, 2) reports an exception visible in java.
954-5: just to clarify, we are not worried about the order in which
properties are read from imports. If an imports object backs some of
the imported values with getters, then it should perform the same
regardless of order of gets.
1079 js(fromString("try {} catch (x__) { x__ = 3; }")),
The exception ending in __ is tested in a number of places, which is
fine, but I don't see anything that tests for exception names ending
in single underscore. Is that ok?
1086 "x = ___.readPub(g, 0);");
Why no fasttracking? Is it because 0 + '_canRead___' is not a valid
identifier? This seems good for debug mode, but it can be replaced
with g[0] in an optimization pass, right?
1090 // TODO(ihab.awad): The below fails for assignment to a free module
1091 // variable. If we declared a 'var', it would fail on the 'var' rather
1092 // than in setReadModifyWriteLocalVar. This test is disabled for now.
1093 // checkFails("x__ *= 2;", "Variables cannot end in \"__\"");
1197 // checkFails("++x__;", "Variables cannot end in \"__\"");
So this does fail, but with the wrong error message? If so, maybe
just change the error message to ""
1156 // checkFails("x__--;", "Variables cannot end in \"__\"");
Need TODO or bobbit error message
1190 public void testFoo() throws Exception {
Test needs proper name
1334 // TODO(ihab.awad): This actually throws an xo4a error!
1335 checkFails(
1543 public void testCallCajaDef2Bad() throws Exception {
1616 public void testCallCajaDef3PlusBad() throws Exception {
All these bad ctor tests are gone. Are those all replaced with bad
function tests?
2070 // Check exceptions on read of uninitialized variables.
Where did this test go, or why is it moot?
changes/ihab/sharply/trunk/src/javatests/com/google/caja/parser/quasiliteral/ScopeTest.java@1484
127 assertFreeVariables("a().b.c.d;", "a", "");
Shouldn't the non-free variables include "b,c,d"
changes/ihab/sharply/trunk/src/javatests/com/google/caja/plugin/CompiledPluginTest.java@1484
71 "java.lang.System.out.println('assertEquals = ' +
assertEquals);");
Please indent properly, and please send error traces to stderr.
changes/ihab/sharply/trunk/src/javatests/com/google/caja/plugin/HtmlCompiledPluginTest.java@1484
95 // TODO(ihab.awad): SECURITY: Re-enable by reading (say) x.foo, and
96 // defining the property IMPORTS___.foo.
441 // TODO(ihab.awad): Disabled until we figure out how to get a
test fixture
442 // that allows us to add stuff to IMPORTS___ before the test is run.
See changes/mikesamuel/fix-debug-mode-ctors/trunk/src/javatests/com/google/caja/plugin/stages/DebuggingSymbolsStageTest.java
for some code which uses caja.result to sneak a value out of
loadModule.
changes/ihab/sharply/trunk/src/js/com/google/caja/caja.js@1484
Please document bind. Should the bound function be callable via a
frozen bound version before it's been frozen?
96 }
100 }
Need semicolons
2008/5/28 <ihab...@gmail.com>:
Mike Stay
> 127 assertFreeVariables("a().b.c.d;", "a", "");
> Shouldn't the non-free variables include "b,c,d"
No, they're properties, not variables.
--
Mike Stay - meta...@gmail.com
http://math.ucr.edu/~mike
http://reperiendi.wordpress.com
Mike Samuel
>
> On Thu, May 29, 2008 at 2:27 PM, Mike Samuel <mikes...@gmail.com> wrote:
>> 127 assertFreeVariables("a().b.c.d;", "a", "");
>> Shouldn't the non-free variables include "b,c,d"
>
> No, they're properties, not variables.
Yep, so they're not free variables. But to be consistent with
102 assertFreeVariables("a.b.c.d;", "a", "b,c,d");
properties should be listed among the non free-variables.
ihab...@gmail.com
2008/5/29 Mike Stay <meta...@gmail.com>:
>Yep, so they're not free variables. But to be consistent with
> On Thu, May 29, 2008 at 2:27 PM, Mike Samuel <mikes...@gmail.com> wrote:
>> 127 assertFreeVariables("a().b.c.d;", "a", "");
>> Shouldn't the non-free variables include "b,c,d"
>
> No, they're properties, not variables.
102 assertFreeVariables("a.b.c.d;", "a", "b,c,d");
properties should be listed among the non free-variables.
That's correct. The test should be testing that they are indeed *not* being treated as free variables. Cheers,
Ihab
Mark Miller
<david....@industrial-designers.co.uk> wrote:
> Since simple statements cannot declare anything, the effect is that
> a substatement can only declare things within a block, and so it is
> clear to a human reader what the scope of those things will be.
Great! This is the direction we independently decided on for ES3.1
strict mode, which we therefore need to propose to the ES4 folks
tomorrow. This decision is not yet reflected in the ES3.1 draft spec
at bottom of <http://wiki.ecmascript.org/doku.php?id=es3.1:es3.1_proposal_working_draft>.
I think it's a fine decision for Caja as well, whether or not we
implement this decision in time for the review.
--
Text by me above is hereby placed in the public domain
Cheers,
--MarkM
ihab...@gmail.com
I think it's a fine decision for Caja as well, whether or not we
implement this decision in time for the review.
I agree but fwiw I think it's probably a hard thing to do in time for the review. Cheers,
Ihab
David-Sarah Hopwood
> On Thu, May 29, 2008 at 9:51 AM, David-Sarah Hopwood
> <david....@industrial-designers.co.uk> wrote:
>> Since simple statements cannot declare anything, the effect is that
>> a substatement can only declare things within a block, and so it is
>> clear to a human reader what the scope of those things will be.
>
> Great! This is the direction we independently decided on for ES3.1
> strict mode, which we therefore need to propose to the ES4 folks
> tomorrow.
Thanks, that gives me some confidence that I'm not making up
restrictions at random :-)
> This decision is not yet reflected in the ES3.1 draft spec
> at bottom of <http://wiki.ecmascript.org/doku.php?id=es3.1:es3.1_proposal_working_draft>.
>
> I think it's a fine decision for Caja as well, whether or not we
> implement this decision in time for the review.
For ES3.1 strict mode, I suggest not including <ExpressionStatement>s as
simple statements.
While we're on the subject of scoping, could you look over the proposed
scoping rules for Jacaranda to see how they compare to Cajita:
A *control block* is a <Block>, <FunctionBody>, or a <CaseClause> or
<DefaultClause> of a switch statement.
The *loose scope* of a declaration is the scope in which occurrences of
the declared identifier would refer to that declaration according to ES3
(this is the whole body of the function that declares the identifier,
including any nested functions in which the identifier is not shadowed).
There MUST NOT be more than one declaration of the same identifier by
a given function (that is, in the function's parameter list, or as
a variable declared by 'var', 'const' or 'catch', or as a declared
function).
The *strict scope* of:
- a variable declared in a <VariableStatement>, is its loose scope
restricted to the textually following section of the immediately
surrounding control block. This includes subsequent, but not prior
initialisers in the statement's <VariableDeclarationList>;
- a variable declared in the <VariableDeclarationListNoIn> part of
a «for (var ...)» form of an <IterationStatement>, is its
loose scope restricted to the textually following section of the
iteration statement. This includes subsequent, but not prior
initialisers in the <VariableDeclarationListNoIn>;
- a variable declared by a <Catch>, is its loose scope restricted to
that catch block;
- a function declared by a <FunctionDeclaration>, is its loose scope.
If an identifier occurrence is not in the loose scope of any
declaration, it is called a *free identifier occurrence*, and
that identifier is a *free identifier* of the module. Otherwise,
the occurrence MUST be in the strict scope of exactly one
declaration.
(I may end up restricting the strict scope of a function declaration
further, to work around the Mozilla bug where it does not "process"
function declarations before executing code, as required by ECMA-262
section 14.)
--
David-Sarah Hopwood
ihab...@gmail.com
changes/ihab/sharply/trunk/src/javatests/com/google/caja/opensocial/DefaultGadgetRewriterTest.java@1484
144 // TODO(ihab.awad): Rewrite "golden" or turn into a functional test
145 if (false) {
I don't think you can turn the dynamic style stuff into a functional
test without implementing CSS support in env.js
Then we should either (a) implement some simple CSS support in env.js or (b) move these tests out of the -.opensocial package and make them more isolated somewhere else. There is no reason to incorporate all the OpenSocial gadget boilerplate: dynamic styles don't require that.
Checking in as-is to unblock the group. TODO remains to remind us.
changes/ihab/sharply/trunk/src/javatests/com/google/caja/opensocial/service/CajolingServiceTest.java@1484
102 "{ var x = y; }",
103 DefaultCajaRewriterTest.weldPrelude("y") +
104 "var x0___;" +
105 "{ x = y; }");
This seems to say ... Shouldn't this be ...
You're correct; it was old leftover stuff. Fixed. That said, (a) the test was missing an assertTrue(...), and (b) the test is broken since it doesn't really cajole. Filed a bug.
changes/ihab/sharply/trunk/src/javatests/com/google/caja/parser/quasiliteral/DefaultCajaRewriterTest.java@1484
90 // TODO(ihab): SECURITY: Test for whether references to global
'this' are rejected statically.
"for whether" -> "whether"
Even better; removed! I already have testBadGlobalThis().
123 "function foo() {this;}" +
124 "caja.def(foo, Object);" +
125 "function bar() {this;}" +
126 "caja.def(bar, foo);" +
127 "var b=new bar;" +
128 "caja.extend(Object, {x:1});" +
Do we now require that all variables be declared with var?
Yepper. Else they are IMPORTS___, in which case they cannot be assigned to.
Indentation on lines 454-461.
Fixed.
750: foo.x = 3;
Maybe assertEquals(foo.x, 3);
Yes, added.
Are there any tests that assertEquals works cajoled, e.g. that
assertEquals(1, 2) reports an exception visible in java.
There is one now. :)
What does "// iok" mean?
Leftover from when I was going through and auditing for coverage. Bobbitted.
954-5: just to clarify, we are not worried about the order in which
properties are read from imports. If an imports object backs some of
the imported values with getters, then it should perform the same
regardless of order of gets.
Indeed ... I seem to have lost my place. Are you saying that the "golden" tests check for *ordered* lookup of properties when in reality we don't care? If so then I agree, and per a TODO elsewhere I need to add more fixturing that allows me to instrument a test with full-authority JS run "before" and "after" the test, which should allow me to do this kind of stuff better.
1079 js(fromString("try {} catch (x__) { x__ = 3; }")),
The exception ending in __ is tested in a number of places, which is
fine, but I don't see anything that tests for exception names ending
in single underscore. Is that ok?
We are uniformly allowing any 'var' to end in a single underscore, including but not limited to exception variables. I agree with your implication that this is a suspect practice (though not overtly insecure afaik). Filed a bug.
1086 "x = ___.readPub(g, 0);");
Why no fasttracking? Is it because 0 + '_canRead___' is not a valid
identifier? This seems good for debug mode, but it can be replaced
with g[0] in an optimization pass, right?
Sure.
1090 // TODO(ihab.awad): The below fails for assignment to a free module
1091 // variable. If we declared a 'var', it would fail on the 'var' rather
1092 // than in setReadModifyWriteLocalVar. This test is disabled for now.
1093 // checkFails("x__ *= 2;", "Variables cannot end in \"__\"");
1197 // checkFails("++x__;", "Variables cannot end in \"__\"");
So this does fail, but with the wrong error message? If so, maybe
just change the error message to ""
Good point. Fixed.
1156 // checkFails("x__--;", "Variables cannot end in \"__\"");
Need TODO or bobbit error message
Now checking "".
1190 public void testFoo() throws Exception {
Test needs proper name
You don't say! :) It was a throwaway debugging schmivit that didn't get removed. Gone.
1334 // TODO(ihab.awad): This actually throws an xo4a error!
1335 checkFails(
1543 public void testCallCajaDef2Bad() throws Exception {
1616 public void testCallCajaDef3PlusBad() throws Exception {
All these bad ctor tests are gone. Are those all replaced with bad
function tests?
All checks are done at runtime instead, per 1st class ctors.
070 // Check exceptions on read of uninitialized variables.
Where did this test go, or why is it moot?
These are now no longer local readPub(...)s on IMPORTS___. They are now module linkage errors that fail early during the generated ___.readImports(...) call.
Once we figure out the proper semantics of ___.readImports(), and instrument our tests so we can have pure-JS "before" and "after" fixtures, we can more easily test for that. Filed a bug.
changes/ihab/sharply/trunk/src/javatests/com/google/caja/parser/quasiliteral/ScopeTest.java@1484
127 assertFreeVariables("a().b.c.d;", "a", "");
Shouldn't the non-free variables include "b,c,d"
Yes, fixed.
changes/ihab/sharply/trunk/src/javatests/com/google/caja/plugin/CompiledPluginTest.java@1484
71 "java.lang.System.out.println('assertEquals = ' +
assertEquals);");
Please indent properly, and please send error traces to stderr.
That was leftover debug dribble drivel. Removed.
changes/ihab/sharply/trunk/src/javatests/com/google/caja/plugin/HtmlCompiledPluginTest.java@1484
95 // TODO(ihab.awad): SECURITY: Re-enable by reading (say) x.foo, and
96 // defining the property IMPORTS___.foo.
441 // TODO(ihab.awad): Disabled until we figure out how to get a
test fixture
442 // that allows us to add stuff to IMPORTS___ before the test is run.
See changes/mikesamuel/fix-debug-mode-ctors/trunk/src/javatests/com/google/caja/plugin/stages/DebuggingSymbolsStageTest.java
for some code which uses caja.result to sneak a value out of
loadModule.
Ok, let's do this in another CL.
changes/ihab/sharply/trunk/src/js/com/google/caja/caja.js@1484
Please document bind. Should the bound function be callable via a
frozen bound version before it's been frozen?
Documented.
As for the rest, I'm afraid I segfaulted again. :(
96 }
100 }
Need semicolons
Fixed. (But JS has semicolon insertion, didn't you hear?) };->
Ihab
Mike Samuel
101 public void
testAssertionsWorkCajoled() throws Exception {
102 try {
103 rewriteAndExecute("assertEquals(1, 2);");
104 fail("Assertions do not work in cajoled mode");
105 } catch (Throwable t) { }
106 }
This will never fail since the AssertionError raised by fail is suppressed.
And the throws Exception is unnecessary.
2008/5/30 <ihab...@gmail.com>:
Mike Samuel
changes/ihab/sharply/trunk/src/javatests/com/google/caja/plugin/asserts.js@1512
160 try {
161 func();
162 fail('Did not throw ' + (msg ? msg : 'an exception'));
163 } catch (ex) {
164 if (msg) { assertEquals(ex, msg); }
165 }
though I don't know quite what assertEquals will do.
And with assertEquals, the expected value should be first.
2008/5/30 Mike Samuel <mikes...@gmail.com>:
Mark S. Miller
> These are now no longer local readPub(...)s on IMPORTS___. They are now
> module linkage errors that fail early during the generated
> ___.readImports(...) call.
>
> Once we figure out the proper semantics of ___.readImports(), and instrument
> our tests so we can have pure-JS "before" and "after" fixtures, we can more
> easily test for that. Filed a bug.
This is the first I've heard of ___.readImports(). I thought we were
just generating ___.readPub()s in the module prelude. Why not?
--
Cheers,
--MarkM
ihab...@gmail.com
This will never fail since the AssertionError raised by fail is suppressed.
Oh oh oh because JUnit tests fail() by throwing. Ok.
And the throws Exception is unnecessary.
Well it's idiomatic, which is good, but ok I removed it.
Ihab
ihab...@gmail.com
Well it's idiomatic, which is good, but ok I removed it.
Actually rewriteAndExecute throws ParseException and stuff. Re-added (given how I'm doing it now).. -- I
ihab...@gmail.com
This is the first I've heard of ___.readImports(). I thought we were
just generating ___.readPub()s in the module prelude. Why not?
Also, maybe we want a different error -- like a "module linkage" error rather than a "not readable".
Ihab
ihab...@gmail.com
Similarly for
changes/ihab/sharply/trunk/src/javatests/com/google/caja/plugin/asserts.js@1512
160 try {
161 func();
162 fail('Did not throw ' + (msg ? msg : 'an exception'));
163 } catch (ex) {
164 if (msg) { assertEquals(ex, msg); }
165 }
though I don't know quite what assertEquals will do.
And with assertEquals, the expected value should be first.
Fixed and fixed. Expect a snapshot in a minute....
Ihab
David-Sarah Hopwood
> On Thu, May 29, 2008 at 2:27 PM, Mike Samuel <mikes...@gmail.com> wrote:
>
> 1079 js(fromString("try {} catch (x__) { x__ = 3; }")),
>> The exception ending in __ is tested in a number of places, which is
>> fine, but I don't see anything that tests for exception names ending
>> in single underscore. Is that ok?
>
> We are uniformly allowing any 'var' to end in a single underscore, including
> but not limited to exception variables. I agree with your implication that
> this is a suspect practice (though not overtly insecure afaik).
Why is it suspect? We're not trying to prevent the programmer from using
silly names -- only insecure ones.
--
David-Sarah Hopwood
ihab...@gmail.com
Why is it suspect? We're not trying to prevent the programmer from using
silly names -- only insecure ones.
Agreed. s/suspect/silly/g in my earlier remark. :) Cheers,
Ihab