I have to admit that you lost me pretty quickly… ;-) It sound to me like you know what you’re talking about, so I’m fine with whatever you did/want to do… :-)
Harbs > On Mar 5, 2017, at 9:31 AM, Greg Dove <greg.d...@gmail.com> wrote: > > 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.d...@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 <aha...@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" <gregd...@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 >>> >>> >>
