thanks a lot for your pointers. I guess this will help us fix the
problem a lot faster. So our rewrite experiment hit a spot ;-) This also
means that the probability of rewrites being broken for multiple
dialects is high...? Where would we send a possible fix to? The
Refactory? You?
Again, thanks a lot for this prompt and precise reaction.
Joachim
Am 09.04.12 23:32, schrieb NiallRoss:
Joachim Tuchel wrote:
> thanks a lot for your pointers. I guess this will help us fix the
> problem a lot faster. So our rewrite experiment hit a spot ;-)
Yes indeed. It was a good case to expose the exact problem.
> This also means that the probability of rewrites being broken for
> multiple dialects is high...? Where would we send a possible fix to?
> The Refactory? You?
Me, definitely: as far as I know, the custom refactoring project is the
one maintaining compatibility of the cross-dialect RB model in
VASmalltalk and VW these days. We also maintain it in Smalltalk/X (so I
will pass the fix to Jan) and in Pharo/Squeak (I'll fix and publish
and/or let them know) - those two are insofar as I have the time (which
is often not very far and/or a bit late).
I will try and fix this in VW and VASmalltalk this weekend but by all
means you and/or Solveig get in ahead of me for VASmalltalk. I think
the most likely fix is that things referred to by an RBMethodNode should
include things defined by its body, but I must think about whether
instead they should be treated as also defined by the method node.
Either is fairly easy to implement - it is merely whether either could
have a side-effect and which is conceptually right. (Also, VW allows
methods to define temps that override names in their scope whereas
VASmalltalk, I think, does not - I'm hoping that will not discriminate
between which of RBMethodNode>>defines: and RBMethodNode>>references:
should be added to in the two dialects.)
I aim to fix this - or test a fix you email me - this weekend. (Being
just back from holiday, I have a full email in-box re VW work, so all
VA must wait till the weekend. :-) )
By all means copy John Brant on any emails if you wish - he'd doubtless
be interested and may have insights. AFAIK, John is not actively
maintaining the RB these days. Travis is the TOOLs guru for VW - I'll
be informing him.
Yours faithfully
Niall Ross
______________________________________________________________________
This email has been scanned by the Symantec Email Security.cloud service.
For more information please visit http://www.symanteccloud.com
______________________________________________________________________
1) The file-out of #verifyForUnknownVariableOf:in: (RBSmallintUIApp), I
recommend as the correct fix for the issue Joachim found - case 49471.
(I am incorporating this into my current work on the RB but the next
version may not be out for a while, so by all means add it right away to
your current development maps / fixes / whatever.) I've made the
checking code handle the case where the oldNode isMethod and so its
body's definitions need also to be checked.
2) I notice that Class>>definitionMessage has changed. Previously it
only added the 'classInstanceVariableNames:' part of the class
definition message if the class had any class instance variables. Now
it always adds it. This interacts with an aspect of the RB framework to
cause
RBNamespaceTest>>testDefineClassAfterDeletedChange
to fail. I attach a fileOut of a change to that test
(RefactoryTestingApp) to make it pass.
(The rest of this email is just remarks about this second case ignore
unless interested.)
I have not seen other issues related to this, but detail the cause below
it in case anyone else has.
a) AddClassChange>>isValidMessageName: in VA (and VW3) only accepts
class creation messages without 'classInstanceVariableNames:'. This
part of the RB framework has separate handling of class and metaclass
(the isMeta variable in the AddClassChange), so class instance variables
are parsed as the instance variables of the metaclass, not in the same
definition string as that of the class. (VW7, by contrast, does have a
single message, so the RB code is slightly different there.)
b) RBNamespaceTest>>testDefineClassAfterDeletedChange sendt
#definitionString to self to get the definition it needs to make an
AddClassChange for the test. This now fails to parse, since
#isValidMessageName: rejects it, so the test fails. The rather trivial
fix is to replace
RBNamespaceTest>>testDefineClassAfterDeletedChange
...
st defineClass: self class definitionString in: RefactoryTestingApp
...
with hardcoding of all but the class name part of the string of the
class' trivial message.
Since there was always the option for this keyword to be there (i.e. if
the class had classInstanceVariables), I don't think is an actual bug
(we must have seen failures on every class with classInstanceVariables
if the code was not robust to it); rather, it is an undesirable feature
which this change reveals to us. In the past, one could create an
AddClassChange in a script via
AddClassChange definition: someClass definitionString
if the class had no classInstanceVariables, but not if it did. Now you
can never use it. Except in this one test, it is not used in the base.
Maybe people who hacked scripts in the RB framework sometimes used it
and were sometimes caught out when a class had classInstanceVariables.
(N.B. it would not be right to change the framework just to add the new
messages to #isValidMessageName:, since in
AddClassChange>>fillOutDefinitions
there is hardcoding of
instancevariableNames := self namesIn: (parseTree arguments at: 2)
value.
... ... arguments at: 3 ...
etc., and this would need to be changed as well to handle both messages
with keyword #classInstanceVariables: and those without.)
Yours faithfully
Niall Ross
Niall Ross wrote: