I've updated the change set, and just sent out the updated review. But I also
wanted to specifically address each issue raised.
On Mar 8, 2011, at 3:35 PM, P T Withington wrote:
> Comments:
>
> 1. ClassModel.java: Style nit:
>
> if (! modelOnly || isbuiltin) {
>
> I like to fully parenthesize expressions like this, to a. call attention to
> the nearly invisible `not` operator, and b. make it so the human reading the
> code does not have to remember the associativity rules for the language to
> read the code.
Done.
> 2. SchemaBuilder#757: We have a /de facto/ situation where attribute types
> are XML (or LZX) types, for the most part, with a magic mapping from XML type
> to Javascript type, but method argument types are always just Javascript
> types. I don't have any good proposals on how to fix that, but maybe what is
> really important here is that if the LFC is advertising a public attribute
> with a type that cannot be represented in the schema, perhaps it should not
> be a public attribute after all? Maybe we should be issuing a warning for
> these attributes (and clean them up).
It is not the attribute types, it is the method parameter types
that are causing trouble. Yes, they *should* be the Javascript types, but they
aren't in many
places in the javadoc. We don't use the method parameters now, so this can be
attacked
whenever we need them. (I don't want to make this change set bigger, or with
more dependencies, than it needs to be)
>
> 3. CommonGenerator: "predefinedGlobals" are actually properties of the
> ECMAScript global object, so perhaps should have a name that conveys that.
Done - renamed to globalProperties.
>
> 4. CommonGenerator: I wonder if it will be important to know the types of
> built-in properties eventually?
Probably eventually, we can add that later. If a @type appears, it is copied
through to lfc.lzx; it is not retained/emitted by the tag compiler now. Again,
keeping the change set to a reasonable size.
> 5. JavascriptGenerator#302: I wonder if it would be better to warn (or err)
> when Debug is referenced in non-debug code? That would seem to go along with
> the goal of improved performance. In fact, rather than warnWithThis, I think
> I would rather have the default to be if the compiler cannot resolve a
> reference that it issue a warning (or error) and we provide a pragma to
> declare a reference as being global (if it is an instance reference, we can
> silence the warning with an explicit `this.`). We can't just do this
> unilaterally, and it probably should not go in this change, but could you
> file an improvement to make this happen? It will mean fixing up all
> demos/examples/test to compile without error by either fixing the compiler to
> know about missing globals, or by adding pragmas for globals we can't know
> about.
Done (filed as http://jira.openlaszlo.org/jira/browse/LPP-9814, P0 assigned to
me).
> 6. JavascriptGenerator#1390: We might want to emit a warning when we
> discover and explicit `with` statement to say that it is deprecated (it is
> not permitted in ES5 strict, and will probably go away completely in ES6)
>
Done - added warning.
> Issues:
>
> 1. ClassModel.java#1010: We've tried pretty hard not to have any
> platform-specific code in the tag compiler (the LZX-to-Javascript phase).
> Your introducing an env.isDHTML here, and unless there are strong reasons for
> doing so, I'd rather not see that.
Done - removed env.isDHTML, and a fix in SWF9Generator was needed to elide it
properly.
> 2. NodeModel#838, #865, Seems to me extends, jsname, jsextends, should all
> fall under the metamodel scheme and be skipped there, rather than called out
> as literal constants.
I tried this, the metamodel schema code later applies when
metamodel.getAttribute(name, ALLOCATION_INSTANCE) != null, which is not the
case most/all of the time: (not the case for LzView, LzCanvas, LzText, LzState,
...) I don't think we can get around looking at the literal constants. BTW,
the "extends" check was in place before I added the "jsname" and "jsextends"
checks.
>
> 3. SchemaBuilder/addEvents: This is a mess that I created that needs to be
> cleaned up. See André's comment on
> http://jira.openlaszlo.org/jira/browse/LPP-9675. At the very least, my idea
> to auto-emit event declarations seems broken. Perhaps that should just be
> backed out? I can't tell if your change relies on auto-event declaration,
> hopefully not.
We do not rely on auto-event declaration, and events are not included in the
reference classes. Cleanup should happen separately as part of LPP-9675.
>
> 4. CommonGenerator: "prototype" is _not_ a property of Object instances, only
> of Function instances (and hence of our Classes, which are implemented as
> ECMAScript Functions. Function instances also have length, apply, and call
> as properties.
Done - removed prototype from the list.
> 5. CommonGenerator: We should probably add to the ECMA global properties the
> non-standard (but nearly always present in our target runtimes) global object
> properties (which can be found in ECMA-262-5 B.2: escape and unescape. This
> would solve some of the issues you report in your test comments.
Done - added escape, unescape
> 6. CommonGenerator: Similarly, we include in pre-defined globals `lz`, which
> is defined early on by the runtime. It seems you should add `canvas` to this
> list because it is an implicitly-defined global in all user programs, bound
> to the single instance of the <canvas> tag.
Done - added lz, canvas.
> 7. JavascriptGenerator#1451: Would it be useful here, rather than clearing
> possibleInstance, just remove the ones you have bound and then assert that it
> is empty?
(See 'More issues' #1 below) If the classdescription is complete, what we do is
remove the ones we have bound, and if there are any remaining issue a warning
if option 'warnUnknownGlobals' is on:
'Warning: <functionname> uses unknown variables, treated as globals:
<sym1>, <sym2>....
Since the class hierarchy is known, any unknown (and there may be a few) are
globals -- we shouldn't assert they are empty, since they may be simply globals
we don't know about (we don't have the complete picture of names of all class
names in lfc, for instance, which may be referenced via new LzXXX() or
LzXXX.staticMethod).
> 8. ScriptClass: I don't understand what is going on here. If you are
> renaming these variables only here, it seems they must not be referenced in
> the first place, so I wonder why we are emitting these. It seems there must
> be a deeper issue that this is papering over?
We are accounting for 'metasymbols' that use JS reserved words, like <attribute
name="with"/> that are put into the schema (via lfc-undeclared.lzx), also
interfaces named 'interface' and 'class'. I tried, and it was difficult to
introduce a 'metasymbol="true"' into the schema (I successfully got them to get
elided, but introduced some other errors that I have not tracked down). Per
your suggestion, I changed ScriptClass to elide attributes and classes that
conflict with reserved words. Since we are not emitting all classes (only ones
in the hierarchy of user's classes), we don't need to worry about 'interface'
and 'class', but 'with' is on LzNode, so that will get elided.
>
> 9. Can we add some test cases to test/smoke/compiler.lzl? A particular test
> I would like to see is a method with an inner function that references a
> global, does/doesn't shadow an instance variable, etc.
There was already one test (closureOverShadowedValue), I added 3 more, testing
all combinations of {shadowed, not shadowed}, {with free reference, without}
Also added a set of 10 tests 'testWithThisAlias' in smoketest to cover the
situation you raised about globals, and variations thereof.
Also added Andre's test5, test11, test12, test14 to lztest which pointed out
flaws in the earlier change set.
====
More issues:
> 1. I missed a glaring logic error in JavascriptGenerator in my previous
> review:
>
> JavascriptGenerator#1419:
>
> The global table is a red herring. It started out as an attempted solution
> to this problem, but it was an incorrect one. I'm sorry that it is still
> there, because I think it led you down the garden path...
>
> We _can't_ remove globals from possibleInstance if the classdesc is not
> complete. If it's not complete, we don't know that there might not be
> instance vars that alias globals. If the classdesc _is_ complete, we don't
> need possibleInstance, because we know the actual instance from the classdesc.
>
> In fact, the compiler's knowledge of globals is really useless for the
> purposes of computing if we need with(this) or not. The only use for globals
> would be to emit a warning when there are free references that the compiler
> cannot identify as a global.
>
> The real logic of this section should be:
>
> if classdesc complete OR if incomplete but all free references are in
> classdesc:
> rewrite all free references that are in classdesc
> otherwise:
> emit a warning and use with(this)
>
Done.
> As a bonus:
>
> if classdesc complete and there are free references _not_ in classdesc:
> compare those free references to known globals and emit a warning for any
> that cannot be identified
Partially done, partially deferred. As indicated about (issues #7), these
could occur for real situations (new LzXXX() or LzXXX.staticMethod). We get
40-60 warnings per each version of lfc built. So for now, this warning occurs
only if the warnUnknownGlobal option is true. I've updated LPP-9814 to talk
about this issue and left a TODO where the warning is issued.
> 2. Reproduce warnings seen with webtop:
> with(this) added in get canvas.gLoginProcess: unknown parts of class
> hierarchy: [$lzc$class_WebtopDataSet->LzDataset] and unaccounted refs:
> [canvas]
> with(this) added in get canvas.gLoginProcess: unknown parts of class
> hierarchy: [$lzc$class_WebtopDataSet->LzDataset] and unaccounted refs:
> [canvas]
>
>>
> Perhaps the key is the method name: 'get canvas.gLoginProcess', which is how
> the compiler labels the (automatically generated) reference method of a
> handler.
>
> The outline of the code that appears to trigger this is:
>
> <class name="WebtopDataSet" extends="dataset">
>
> <handler name="oninited" reference="canvas.gLoginProcess"
> method="_initialize" />
>
> <method name="_initialize" args="ignore" />
>
> </class>
>
> This is going to create a method on the class that has a free reference for
> canvas. What I don't understand is why it thinks it does not know the full
> class hierarchy and just treat it as a free reference?
With this latest change set, I still cannot get this to happen. See <class
name="extdataset" extends="dataset"> in compiler.lzl. I (temporarily) removed
'canvas' from the list of known globals, and then ran:
lzc -DwarnWithThis -DwarnUnknownGlobals --runtime="dhtml" smokecheck.lzx
There are many warnings about unknown globals, canvas among them, but I don't
see 'unknown parts of the class hierarchy' for LzDataset. I do see it for
LzUrl, for a test I intentionally created to see it. Anyway, I think you
should retry with this latest.
--
Don Anderson
Java/C/C++, Berkeley DB, systems consultant
voice: 617-306-2057
email: [email protected]
www: http://www.ddanderson.com
blog: http://libdb.wordpress.com
