"It sound to me like you know what you’re talking about"
Haha, Harbs that's probably more reassuring to me than it should be to you.
In terms of anything at all in the compiler, I am still LAYG (learning as
you go) here ;)
(btw, you can never have too many 'as you go' acronyms)
I actually shared Alex's general concerns over making changes in the mxml
parsing, we don't have a large range of tests to cover all the potential
mxml variations (otherwise I expect this would have already been addressed).
Harbs, I understand that you have a (quite possibly) large codebase so you
might provide the best real-world 'litmus test' in you have a large amount
of mxml.
In simple terms the changes will prevent the following 2 examples from
compiling, which currently compile fine in FlexJS, but should not:
<js:initialView>
<local:MyInitialView id="view1" />
<local:MyInitialView id="view2" />
</js:initialView>
in the above example the view2 instance was being created and assigned for
the initialView propertty, the first was ignored
<js:initialView>
<js:SimpleCSSValuesImpl />
</js:initialView>
In this case the types are incompatible.
I actually think the compiler errors for mxml could be more important than
a sesaoned Flex user would assume in terms of how new people will form
early impressions. When people who potentially don't know actionscript
(yet) take some FlexJS examples for a spin, they are probably going to look
at the mxml and think to themselves 'I can do that', so it will be the
first thing they mess with. Mxml-related errors might be the first thing
they see from the compiler after they start messing with stuff.
That's why I spent a bit of time discussing those...I have gone ahead and
gone with the two new ones as I suggested.
On Mon, Mar 6, 2017 at 12:08 AM, Harbs <harbs.li...@gmail.com> wrote:
> 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.SimpleCSS
> ValuesImpl
> >> is not assignable to target type 'org.apache.flex.core.IApplica
> tionView'.
> >>
> >> 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
> >>>
> >>>
> >>
>
>
