Need help with Rewrite Rule - Instantiations case 49471

Hi Niall,

I tried your fix for #verifyForUnknownVariableOf:in: and now the above mentioned rewriting works like intended.
Sorry for my late reaction, but you know how life and especially work tends to be from time to time..

So thank you very much for your fix and the explanations! I hope Instantiations will inlcude your fix into the next release of VAST.

Joachim


Am Montag, 16. April 2012 15:58:09 UTC+2 schrieb NiallRoss:

Dear Solveig,
    I attach a zip with two fixes.

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:

>>> this is an effect of how the checking code that Adriaan and I added
>>> to the RB interacts with a lacuna in the RBParser code.
>>>
>>> The checking code is
>>>
>>> RewriteSingleMetaCodeTool>>verifyForUnknownVariableOf: node in:
>>> oldNode
>>>     "A pattern variable in the replace text must correspond to a
>>> pattern
>>> variable in the search text.  Mistyping is a common cause of new user
>>> frustration."
>>>
>>>     (node isPatternNode and: [node isVariable]) ifFalse: [^true].
>>>     ((oldNode references: node name) or: [oldNode defines: node name])
>>> ifTrue: [^true].
>>>     self insertError: 'MetaVar not in search text ' at: node start.
>>>     ^false
>>>
>>> It fails because
>>>     RBMethodNode>>defines:
>>> only accepts the method's arguments as being defined by the method
>>> node, nt its temporaries.  It passes the
>>>     RBMethodNode>>references:
>>> call toits body (which is an RBSequenceNode) and
>>>     RBSequenceNode>>references:
>>> only asks its statements what it references, not its temporaries.
>>> Instead
>>>     RBSequenceNode>>defines:
>>> looks at temporaries.  Thus the test
>>>     (oldNode references: node name) or: [oldNode defines: node name]
>>> almost always works nevertheless because usually either (a) the match
>>> code is
>>>     | `@temps |
>>>     ``@.stats
>>> (i.e. without method name, i.e. with the All Method box unticked) and
>>> then
>>>     RBSequenceNode>>defines:
>>> finds the temp expression, or else (b) if a method defines a temp then
>>> its statements reference it, and if a specific temp is replaced by a
>>> pattern then the same pattern will be in the statements replacing its
>>> reference.
>>>
>>> Joachim's code fails because in
>>>
>>> someMethodName
>>>     | `@temps |
>>>     ``@.stats
>>>
>>> (BTW there is no need for a second ` in `@temps - you can't recurse
>>> into a temp declaration) the checking code is sent to an RBMethodNode
>>> and its #defines: does not look at temps, while its #references: asks
>>> the body which only looks in the statements and `@temps is not
>>> mentioned in ``@.stats (of course).
>>

______________________________________________________________________
This email has been scanned by the Symantec Email Security.cloud service.
For more information please visit http://www.symanteccloud.com
______________________________________________________________________