I haven't tested the patch, looks reasonable.
That said, the reason I tried hacking the AST in ASCompilationUnit was
because I thought the "best" way would be to effectively fix up the source
code as if it had been written properly in the first place. IOW, this
code effectively looks for source code that does:
[Bindable]
SomeClass extends Object
{
}
And makes it look like:
[Bindable]
SomeClass extends EventDispatcher
{
}
As you know, there are other patterns for static binding. Or another way
to think of it: if you do take the time to re-base your [Bindable] source
code on EventDispatcher, none of this code in the compiler executes. It
is a convenience feature: the compiler could just report an error saying
you can't use [Bindable] on Objects.
I'm not a compiler expert, but back when Falcon only produced SWFs, I
guess it seemed reasonable to leave the AST alone and generate modified
output, but with FalconJX leveraging the AST, I'm concerned about work
that means that future compiler developers will have to know that the AST
doesn't go straight to JS or whatever the output is. As we've seen, the
JSDoc annotation was driven off the AST. What else might we someday want
to change in the AST "conversion" that will require us knowing about this
code in ClassDirectiveProcessor?
Anyway, again, I don't see any hurry to go back and look into
re-implementing this [Bindable] handling in ASCompilationUnit. At least
we've recorded in the archives that there might be other ways to solve
this problem.
Thanks,
-Alex
On 9/5/16, 6:44 PM, "Greg Dove" <greg.d...@gmail.com> wrote:
>I think I found a solution for the dependency issue I mentioned.
>The inclusion of "ValueChangeEvent" when not otherwise specified in the
>project provided a useful clue here! (Otherwise I suspect I would not have
>found this)
>I need to do a bit more testing, but I expect to issue a related PR for a
>change in ClassDirectiveProcessor before too long.
>
>On Mon, Sep 5, 2016 at 10:05 PM, Greg Dove <greg.d...@gmail.com> wrote:
>
>> Alex, I did some testing with a minimal project, I did not even have to
>> try as-only, as SimpleApplication was sufficient. And yes, there is an
>> issue if the dependency is not explicitly added somewhere. I will try to
>> find a way to resolve this, otherwise the ASCompilationUnit code will
>>need
>> to go back.
>> I had thought it might be useful to have the bindable implementation be
>> more of any output variation than a manipulation of the AST. I had even
>> wondered about longer term possibilities with such an approach - e.g.
>> perhaps different binding implementation variations (e.g. as3 Signals
>> instead of events or something like that). I realize that this is not
>> something to think about seriously now. Plenty to do with things as they
>> are... was just thinking of possibilities for the future.
>>
>> Anyhow, if I can't find a quick solution to add a specific dependency
>>for
>> output in the swf without the ASCompilationUnit AST manipulation
>>approach,
>> then that will need to go back (at least for now, anyway).
>> Perhaps this was the reason you did this in the first place? Was it when
>> the EventDispatcher base class for binding became non-native?
>> If you think its best to try and fix the original problems with the
>> original code in ASCompilationUnit , then perhaps that is the simplest
>> route for now. If so, I'd be keen to leave the other stuff I added for a
>> short time, but will happily remove the unnecessary parts once we can be
>> sure they never get branched into as a fallback (i.e. we can be sure the
>> ASCompilationUnit code catches all the 'extends' candidates). Let me
>>know
>> what you think.
>>
>> fyi I will be focusing on reflection work in whatever spare time I can
>> find this week, following on from where you got to earlier this year
>>with
>> that. I will let you know where I get to later this week, I'm not sure
>>if I
>> will have it finished this week or not.
>>
>> cheers
>> Greg
>>
>>
>> On Sat, Sep 3, 2016 at 1:41 PM, Greg Dove <greg.d...@gmail.com> wrote:
>>
>>> Sound great. Yes, switching it back on at any point should remain a
>>>quick
>>> fix option because it would simply make the other code act as a
>>>backstop.
>>> I will take a quick look on Monday to see if I can find any problems in
>>> small as-only projects where maybe the order of class definitions in
>>>the
>>> swf could be important with the current approach. Sounds a bit like a
>>> theoretical edge case but you made me think about it.
>>> Have a nice weekend.
>>>
>>> -Greg
>>> [sent from my phone]
>>>
>>> On 3/09/2016 12:41 PM, "Alex Harui" <aha...@adobe.com> wrote:
>>>
>>>>
>>>>
>>>> On 9/2/16, 5:10 PM, "Greg Dove" <greg.d...@gmail.com> wrote:
>>>>
>>>> >Yes, that was one of the major changes. I moved it alongside the
>>>>other
>>>> >implementation inside ClassDirectiveProcessor and was able to get
>>>>more
>>>> >consistent results.
>>>> >I initially commented it out so I could get everything working using
>>>>the
>>>> >implements IEventDispatcher approach - which is the more general
>>>> approach
>>>> >that would work for all cases- in javascript, then added it back for
>>>> both
>>>> >targets as a 2nd output variation in a later commit, and it went to
>>>> >ClassDirectorProcessor for swf.
>>>> >
>>>> >I was seeing 2 issues from the ASCompilationUnit implementation:
>>>> >1. It was not always applying where it should have been (so in cases
>>>> where
>>>> >it could have been 'extends EventDispatcher' it would sometimes fall
>>>> >through to the implements IEventDispatcher approach in
>>>> >ClassDIrectiveProcessor anyway). I suspect this was
>>>>threading-related,
>>>> but
>>>> >I am not so familiar with threads.
>>>> >
>>>> >2. Less of an issue, it could also apply an EventDispatcher base
>>>>class
>>>> in
>>>> >static-only bindable cases, which is unnecessary, this I expect could
>>>> have
>>>> >been more easily fixed. I tried checking with hasModifier in the
>>>> original
>>>> >code, but I think that might not be available yet, iirc that caused
>>>>an
>>>> >error.
>>>> >
>>>> >So I moved everything to the ClassDirectiveProcessor, which had the
>>>> other
>>>> >implementation of bindable support in any case. Sorry, I thought I
>>>>had
>>>> >been
>>>> >clear about that.
>>>>
>>>> Well, you probably did explain it, but it probably didn't stick in my
>>>> head
>>>> until we hit this last issue and I went looking for the code where I
>>>>had
>>>> a
>>>> vague memory of hacking a base class. I still don't know the compiler
>>>> code so well that it is clear in my head what work should be done
>>>>where.
>>>> It looks like the original attempt to deal with [Bindable] for SWF was
>>>> done in ClassDirectiveProcessor, but when I wanted to fix a bug in
>>>> FalconJX I decided to fix it by hacking the AST since that drives
>>>> everything in FalconJX. The right answer may have been for me to move
>>>> the
>>>> code from ClassDirectiveProcessor into ASCompilationUnit and use the
>>>> existing tests for needsEventDispatcher, then maybe we wouldn't have
>>>>had
>>>> the two issues above.
>>>>
>>>> Anyway, I think we have recorded the possibility that AST hacking
>>>>might
>>>> be
>>>> a solution if we hit other problems. I'm ok with leaving the code as
>>>>is
>>>> until we hit another problem but feel free to keep digging if you
>>>>want.
>>>>
>>>> Thanks,
>>>> -Alex
>>>>
>>>>
>>
