flex-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Dove <greg.d...@gmail.com>
Subject Re: [FlexJS] Working to fix a compiler bug - problem with the Problems
Date Sun, 05 Mar 2017 07:31:46 GMT
OK.... please brace yourself for an onslaught of text.

I tested across Flex 4 and FlexJS through a bunch of variations. This
change passes all tests. In the end this is a very small and isolated code
change (I am almost certain it will not contribute to anyone's merge
conflicts) and I believe it gives greater compile-time type safety to mxml,
so I think it's necessary. But the error descriptions are open for more
input/review... I have favored a strong consistency with the Flex 4
descriptions, which were specific for mxml, over using the related
actionscript descriptions.

BTW I had forgotten, but there was also a comment (it was not marked as a
todo) in the falcon MXMLPropertySpecifierNode class that indicated that
this type of work still needed to be done, so I expect it was an unfinished
implementation.

If I hear nothing conflciting by EOD tomorrow I will commit what I have. It
can easily be reverted if anyone finds issues, or I am happy to address it
further if it is not correct for some cases that I did not anticipate.

That's the end of the preamble.... best of luck getting through what
follows... it took me longer than the original coding part :)

Test setup for Flex 4:
'TestView' below is simply a Flex4 Group mxml component, like so:
<s:Group xmlns:fx="http://ns.adobe.com/mxml/2009"
         xmlns:s="library://ns.adobe.com/flex/spark"
         creationComplete="onCreated()">
    <fx:Script><![CDATA[
        import spark.components.Alert;
        private function onCreated():void{
          //check for actionscript version of error
          //this.number="something";
        }
        public var testArr:Array;
        public var number:Number;
        ]]></fx:Script>
    <s:layout>
        <s:VerticalLayout/>
    </s:layout>
    <s:Button click="test.text='testArr ['+testArr.toString()+'],\nnumber
val:'+number" width="100" height="50"/>
    <s:Label id="test"/>
</s:Group>

Test setup for FlexJS:
'TestView' below is simply a Flex4 Group like so:
<js:View xmlns:fx="http://ns.adobe.com/mxml/2009"
xmlns:js="library://ns.apache.org/flexjs/basic"
initComplete="onCreated()">
   <fx:Script><![CDATA[
import org.apache.flex.html.Alert;
        private function onCreated():void{
          //check for actionscript version of error
          //this.number="something";
        }
        public var testArr:Array;
        public var number:Number;
        ]]></fx:Script>
<js:Button click="Alert.show('testArr ['+testArr.toString()+'],\nnumber
val:'+number, this)" width="100" height="50"/>
</js:View>


Testing:
    <local:TestView id="test">
        <local:testArr>
            <fx:Number>23.56</fx:Number>
            <fx:Number>23.99</fx:Number>
            <fx:String>Something else</fx:String>
        </local:testArr>
        <local:number>
            <fx:Number>23.56</fx:Number>
        </local:number>
    </local:TestView>
The above compiles correctly and because the testArr property has an
'Array' type, the children are inferred as array elements, it is the same
as specifying:
        <local:testArr>
            <fx:Array>
                <fx:Number>23.56</fx:Number>
                <fx:Number>23.99</fx:Number>
                <fx:String>Something else</fx:String>
            </fx:Array>
        </local:testArr>
Note: Using one vs. multiple children: the inference is the same - if the
property being initialized is of type Array, it infers correctly (and
performs the 'Array' wrapping if needed) irrespective of single or multiple
child tags.
NO BUGS FOR ARRAY (inferred or explicit)
This works in both Flex 4 and Falcon (before or after my local changes)
Multiple value children when the property being initialized is not 'Array'
type
---------------------------------------------------------------------
the 'number' property on the other hand is of type Number, and changing
number assignment to multiple values, in this way:
   <local:number>
            <fx:Number>23.56</fx:Number>
<fx:Number>23.99</fx:Number>
        </local:number>
gives Flex 4 compiler errors like so:
Error:(20, 0) [flex4testforFlexJS]: In initializer for 'number', multiple
initializer values for target type Number.
    Error:(21, 0) [flex4testforFlexJS]: In initializer for 'number',
multiple initializer values for target type Number.
(adding more than 2 gives an individual error for each child)
<fx:Number>23.56</fx:Number>
                <fx:Number>23.99</fx:Number>
            <fx:String>Something else</fx:String>
        </local:number>
Swapping to one element with wrong type in multiple child assignment does
not change the error reported (it remains simply 'multiple values'):
Error:(20, 0) [flex4testforFlexJS]: In initializer for 'number', multiple
initializer values for target type Number.
Error:(21, 0) [flex4testforFlexJS]: In initializer for 'number', multiple
initializer values for target type Number.
CURRENT BUG
-----------
Currently in FlexJS, the following happens for either of the above
scenarios:
Only the last child tag is processed in multiple tags like this, the
earlier ones are ignored. It is processed as if it were the only child tag
(and it may be of an incompatible type).
PROPOSED FIX(DONE)
-----------------
Please let me know if you agree, or suggest alternative.
in the local fix for flexjs, I have now created a new Problem,
MXMLMultipleInitializersProblem, which follows the Flex 4 example
with outputs descriptions similar to:
GenericTests.mxml(57): col: 17 Error: In initializer for 'local:number'
multiple initializer values are not permitted for target type 'Number'.
                <fx:Number>23.99</fx:Number>
                ^
GenericTests.mxml(58): col: 13 Error: In initializer for 'local:number'
multiple initializer values are not permitted for target type 'Number'.
            <fx:String>Something else</fx:String>
            ^
Inappropriate single child when property being initialized is not 'Array'
type
----------------------------------------------------------------------------
<local:number>
        <fx:String>Something else</fx:String>
    </local:number>
in Flex 4, this gives:
Error:(22, 0) [flex4testforFlexJS]: In initializer for 'number', type
String is not assignable to target type 'Number'.
The corresponding Flex 4 actionscript error is
Error:(11, 0) [flex4testforFlexJS]: Error code: 1067: Implicit coercion of
a value of type String to an unrelated type Number.
If FlexJS, the actionscript error is the same
CURRENT BUG
-----------
in Falcon this currently does not cause a compiler error for mxml,
compilation completes normally, and in js the value of number will be the
"Something else", in swf it is NaN. IMO this is clearly wrong.


PROPOSED FIX (DONE)
-------------------
Please let me know if you agree, or suggest alternative.
I have added a new Problem to be closer to what was shown before in Flex 4:
This is the new one I mirrored from Flex 4 (with same description) in FlexJS
    GenericTests.mxml(56): col: 4 Error: In initializer for 'local:number',
type String is not assignable to target type 'Number'.
            <fx:String>Something else</fx:String>
^
Multiple initializers
---------------------
   <local:number>
            <fx:Number>23.56</fx:Number>
        </local:number>
        <local:number>
            <fx:Number>23.56</fx:Number>
        </local:number>
this gives the following in Flex 4:
Error:(24, 0) [flex4testforFlexJS]: Multiple initializers for property
'number'.
In FlexJS (with or without the proposed fixes for the other issues above)
GenericTests.mxml(58): col: 9 Error: Child tag 'number' bound to namespace
'*' is already specified for element 'local:TestView'. It will be ignored.
        <local:number>
        ^
(In both Flex 4 and FlexJS the line number reported is for the beginning of
the 2nd initializer tag <local:number>, which is correct)
CURRENT BUG
-----------
I believe the error description should be improved - it currently says "It
will be ignored" and that is not what actually happens (not being ignored
is correct, IMO).
And (unless I am missing something important) the rest seems too verbose.
PROPOSED FIX (NOT DONE):
-----------------------
This is easy to change.... please let me know if you agree.
I suggest changing the FlexJS error description to be similar to Flex 4, to:
Multiple initializers for 'local:number' are not permitted for element
'local:TestView'.
   <local:number>
            ^




On Sat, Mar 4, 2017 at 8:46 PM, Greg Dove <greg.dove@gmail.com> wrote:

>
> I believe MXML is supposed to treat more than one child in a child tag as
> an array.  And thus, the equivalent AS code for:
>
> <js:initialView>
>   <local:MyInitialView id="view1" />
>   <local:MyOtherInitialView id="view2" />
> </js:initialView>
>
> is:
>
>   initialView = [ new MyInitialView, new MyOtherInitialView];
>
>
>
> I understood the same, but I had thought it was supposed to be a bit
> smarter than that and only assume an implicit <fx:Array> wrapper if it is
> assigning to an Array typed property (maybe even 'Arraylike') property. I
> will check the behavior using Flex 4 - I assume we want to keep whatever
> 'smarts' were implemented there, or at least as close as makes sense? I'll
> also double check the compiler error for this same scenario in Flex 4,
> which I did not do yet.
>
> If you code that in AS, you will get an error (I hope), and I think the
> compiler should report the same error for this MXML scenario, since
> initialView is of type IApplicationView or something like that.
>
>
> There might arguably be some justification for having an 'mxml version' of
> an error, describing things more in mxml terms, particularly for new users
> as they come on board and perhaps play with mxml examples first before
> learning actionscript, but I will check both the actionscript error in
> FlexJS and the mxml error in Flex 4 (assuming there is one)  for this
> scenario and report back here for consensus.
>
> <js:initialView>
>   <js:SimpleCSSValuesImpl />
> </js:initialView>
>
> Is the equivalent of:
> var initialView:IApplicationView = new SimpleCSSValuesImpl().
>
>
>
> Exactly, and in the fix, I have this currently being described as:
> In initializer for 'js:initialView', type org.apache.flex.core.SimpleCSSValuesImpl
> is not assignable to target type 'org.apache.flex.core.IApplicationView'.
>
> I will check the FlexJS actionscript error for this also, but I definitely
> pulled that one above as-is from Flex 4 for the same mxml problem (which is
> also correctly treated as a compile time error)
>
>
>
> On Fri, Mar 3, 2017 at 6:33 PM, Alex Harui <aharui@adobe.com> wrote:
>
>> Hi Greg,
>>
>> Thanks for digging into this.
>>
>> Without having dug into it myself, I would like to suggest returning a
>> type mismatch error of some sort.  IIRC, the DuplicateChildTagProblem is
>> for the following:
>>
>> <js:initialView>
>>   <local:MyInitialView id="view1" />
>> </js:initialView>
>> <js:initialView>
>>   <local:MyInitialView id="view2" />
>>
>> </js:initialView>
>>
>> The child tag (js:initialView) is really in there twice.
>>
>>
>> I believe MXML is supposed to treat more than one child in a child tag as
>> an array.  And thus, the equivalent AS code for:
>>
>> <js:initialView>
>>   <local:MyInitialView id="view1" />
>>   <local:MyOtherInitialView id="view2" />
>> </js:initialView>
>>
>> is:
>>
>>   initialView = [ new MyInitialView, new MyOtherInitialView];
>>
>> If you code that in AS, you will get an error (I hope), and I think the
>> compiler should report the same error for this MXML scenario, since
>> initialView is of type IApplicationView or something like that.
>>
>> And same for the second scenario:
>>
>> <js:initialView>
>>   <js:SimpleCSSValuesImpl />
>> </js:initialView>
>>
>> Is the equivalent of:
>>
>>
>>   var initialView:IApplicationView = new SimpleCSSValuesImpl().
>>
>> My 2 cents,
>> -Alex
>>
>> On 3/2/17, 9:09 PM, "Greg Dove" <gregdove@apache.org> wrote:
>>
>> >I have been looking into FLEX-35273 [1].
>> >
>> >This is a compiler bug where it is possible to do things that don't make
>> >sense, like:
>> >
>> ><js:initialView>
>> >       <local:MyInitialView id="view1" />
>> >       <local:MyInitialView id="view2" />
>> ></js:initialView>
>> >
>> >or even this :
>> >
>> ><js:initialView>
>> >       <js:SimpleCSSValuesImpl />
>> ></js:initialView>
>> >
>> >Neither of the above caused compile time errors.
>> >
>> >I have a fix for both the above scenarios.
>> >
>> >For the first one, I used MXMLDuplicateChildTagProblem which seems 'close
>> >enough'
>> >
>> >"Child tag '${childTag}' bound to namespace '${childNamespace}' is
>> >already specified for element '${element}'. It will be ignored.";
>> >
>> >This renders as :
>> >Child tag 'MyInitialView' bound to namespace '*' is already specified for
>> >element 'js:initialView'. It will be ignored.
>> >   <local:MyInitialView id="view2" />
>> >    ^
>> >
>> >in the first example above. The text does not feel entirely right, but
>> >seems close enough. "It will be ignored" sounds more like a warning (as
>> >opposed to an error/failure), which I think a) it should not be and b) it
>> >is not.
>> >
>> >For the second one, I could not find any relevant 'Problem' class (I may
>> >have missed one perhaps?) So I just made a new one. I don't really know
>> >what the rules are here.
>> >These Problems seem to include an error code so I am unclear what to do
>> >if I add a new one. i.e. it is not clear what new error code I should
>> >apply (or even if the error code is important for something).
>> >For now I just added an arbitrary errorCode = 1999 on
>> >MXMLBadChildTagPropertyAssignmentProblem.
>> >
>> >The new problem renders out to :
>> >In initializer for 'js:initialView', type
>> >org.apache.flex.core.SimpleCSSValuesImpl is not assignable to target
>> type
>> >'org.apache.flex.core.IApplicationView'.
>> >
>> >Alex, you may be able to advise here.... are there any rules I need to
>> >follow for the CompilerProblem classes (in particular, when adding a new
>> >class).
>> >
>> >Assuming all is well above, I will commit this fix tomorrow. I was also
>> >trying to see if I could figure out how to get checkintests running, but
>> >I so far I did not. However the regular build tests are all fine with the
>> >changes I made for this.
>> >
>> >
>> >1. https://issues.apache.org/jira/browse/FLEX-35273
>>
>>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message