RefactoringBrowser (RB) failing to correctly bind "self" in closures?

Hi guys,

I have some code that uses RB and such code works correctly in Pharo but it fails in GemStone. It seems a problem with a closure which in Pharo "self" resolves to the real object while in GemStone resolves to nil. 

The way to reproduce it is:

| tree rewriter |

tree := RBParser parseMethod: 'DoIt ^ [:proxy | proxy at: #oldSelector. ]'.

rewriter := RBParseTreeRewriter new.

rewriter 

replace: ('`#oldSelector `{:node | node value isSymbol and: [node value matchesRegex: ''.*oldSelector.*''] }')

with: ('`{RBLiteralNode value: (`#oldSelector value copyReplaceAll: ''oldSelector'' with: ''newSelector'') asSymbol }').

rewriter executeTree: tree.

rewriter tree newSource

And the problem is the line:

with: ('`{RBLiteralNode value: (`#oldSelector value copyReplaceAll: ''oldSelector'' with: ''newSelector'') asSymbol }').

which gets translated to:

[ :aDictionary | 

RBLiteralNode

  value:

    ((self lookupMatchFor: '`#oldSelector' in: aDictionary) value

      copyReplaceAll: 'oldSelector'

      with: 'newSelector') asSymbol ]

And the self there fails.

Any idea how this is expected to work?

Thanks in advance

--

Mariano
http://marianopeck.wordpress.com

_______________________________________________
Glass mailing list
[hidden email]
http://lists.gemtalksystems.com/mailman/listinfo/glass

OK, I found the place Dale, and now my tests DO work :)

I replaced "source evaluate" with the below lines in bold. 

RBPatternBlockNode >> createBlockFor: aRBBlockNode

  | source |

  self replacePatternNodesIn: aRBBlockNode.

  source := aRBBlockNode formattedCode.

  ^ (source

    _compileInContext: self

    symbolList: GsSession currentSession symbolList

    oldLitVars: nil

    environmentId: 0) _executeInContext: self

I will see if there are tests I can run from RB and next week I will fork and commit fix.

Thanks, 

On Fri, Nov 20, 2015 at 4:54 PM, Dale Henrichs via Glass <[hidden email]> wrote:

In tODE, I've been able to make `self` resolve to a proper object by using the following to evaluate code:

  | meth |
  meth := self
    _compileInContext: aContext
    symbolList: symbolList
    oldLitVars: litVarArray
    environmentId: 0.
  ^ meth _executeInContext: aContext

`self` is bound to aContext ... presumable RB is passing in a `nil` arg there ....

If you have fixes for RB, please fork the RB project[1] and share your fixes via a pull request.

Dale

[1]  https://github.com/dalehenrich/rb

On 11/19/2015 12:17 PM, Mariano Martinez Peck via Glass wrote:

Hi guys,

I have some code that uses RB and such code works correctly in Pharo but it fails in GemStone. It seems a problem with a closure which in Pharo "self" resolves to the real object while in GemStone resolves to nil. 

The way to reproduce it is:

| tree rewriter |

tree := RBParser parseMethod: 'DoIt ^ [:proxy | proxy at: #oldSelector. ]'.

rewriter := RBParseTreeRewriter new.

rewriter 

replace: ('`#oldSelector `{:node | node value isSymbol and: [node value matchesRegex: ''.*oldSelector.*''] }')

with: ('`{RBLiteralNode value: (`#oldSelector value copyReplaceAll: ''oldSelector'' with: ''newSelector'') asSymbol }').

rewriter executeTree: tree.

rewriter tree newSource

And the problem is the line:

with: ('`{RBLiteralNode value: (`#oldSelector value copyReplaceAll: ''oldSelector'' with: ''newSelector'') asSymbol }').

which gets translated to:

[ :aDictionary | 

RBLiteralNode

  value:

    ((self lookupMatchFor: '`#oldSelector' in: aDictionary) value

      copyReplaceAll: 'oldSelector'

      with: 'newSelector') asSymbol ]

And the self there fails.

Any idea how this is expected to work?

Thanks in advance

--

Mariano
http://marianopeck.wordpress.com

_______________________________________________
Glass mailing list
[hidden email]
http://lists.gemtalksystems.com/mailman/listinfo/glass

_______________________________________________
Glass mailing list
[hidden email]
http://lists.gemtalksystems.com/mailman/listinfo/glass

--

Mariano
http://marianopeck.wordpress.com

On Sun, Nov 22, 2015 at 4:17 PM, Dale Henrichs <[hidden email]> wrote:

Cool ....

Now, tODE only loads AST-Core and I just checked and there _was_ only one test error (a test problem that I just fixed), so you must be using other parts of RB ....

Perhaps we could do a mini sprint for RB and get the tests passing for more of the system?

If you are interested perhaps you could let me know what sub-systems you are using and we could divvy up the tests?

I am supposed to be using the AST-Core package. However, it seems that even in Pharo, there is no test that covers my scenario.

Before I fork and commit the fix, let me ask...shouldn't we move RB from dalehenrichs into gsDevKitHome?   even more because it's even used by tODE right?  I guess you will need to update a couple of things of gsDevKit_home scripts in order to get the new repo, but probably worth.

Anyway, let me know what do you think. 

OK Dale, 

Can you check if everything went correct with my PR?

Thanks

On Mon, Nov 23, 2015 at 3:15 PM, Dale Henrichs <[hidden email]> wrote:

On 11/23/2015 05:12 AM, Mariano Martinez Peck wrote:

On Sun, Nov 22, 2015 at 4:17 PM, Dale Henrichs <[hidden email]> wrote:

Cool ....

Now, tODE only loads AST-Core and I just checked and there _was_ only one test error (a test problem that I just fixed), so you must be using other parts of RB ....

Perhaps we could do a mini sprint for RB and get the tests passing for more of the system?

If you are interested perhaps you could let me know what sub-systems you are using and we could divvy up the tests?

I am supposed to be using the AST-Core package. However, it seems that even in Pharo, there is no test that covers my scenario.

haha, well this cuts down the scope of the RB sprint ... the 100's of faiing tests were in the other parts of RB that aren't loaded with AST-Core....

Before I fork and commit the fix, let me ask...shouldn't we move RB from dalehenrichs into gsDevKitHome?   even more because it's even used by tODE right?  I guess you will need to update a couple of things of gsDevKit_home scripts in order to get the new repo, but probably worth.

Anyway, let me know what do you think. 

I would say that you can go ahead and fork my repo ... when It is moved to GsDevKit, ownership will be re-assigned so the repo will move and your fork will point to the GsDevKit version ... to keep old things working, I will make a new fork of rb ....

replacing the cloned repo will be a bit dicier but can be done with a little work .. something that I don't necessarily want to have to deal with at the moment:)

Dale

--

Mariano
http://marianopeck.wordpress.com

Tests were green, so I merged your pull request ... thanks!

Dale

On 11/23/2015 11:47 AM, Mariano Martinez Peck wrote:

OK Dale, 

Can you check if everything went correct with my PR?

Thanks

On Mon, Nov 23, 2015 at 3:15 PM, Dale Henrichs <[hidden email]> wrote:

On 11/23/2015 05:12 AM, Mariano Martinez Peck wrote:

On Sun, Nov 22, 2015 at 4:17 PM, Dale Henrichs <[hidden email]> wrote:

Cool ....

Now, tODE only loads AST-Core and I just checked and there _was_ only one test error (a test problem that I just fixed), so you must be using other parts of RB ....

Perhaps we could do a mini sprint for RB and get the tests passing for more of the system?

If you are interested perhaps you could let me know what sub-systems you are using and we could divvy up the tests?

I am supposed to be using the AST-Core package. However, it seems that even in Pharo, there is no test that covers my scenario.

haha, well this cuts down the scope of the RB sprint ... the 100's of faiing tests were in the other parts of RB that aren't loaded with AST-Core....

Before I fork and commit the fix, let me ask...shouldn't we move RB from dalehenrichs into gsDevKitHome?   even more because it's even used by tODE right?  I guess you will need to update a couple of things of gsDevKit_home scripts in order to get the new repo, but probably worth.

Anyway, let me know what do you think. 

I would say that you can go ahead and fork my repo ... when It is moved to GsDevKit, ownership will be re-assigned so the repo will move and your fork will point to the GsDevKit version ... to keep old things working, I will make a new fork of rb ....

replacing the cloned repo will be a bit dicier but can be done with a little work .. something that I don't necessarily want to have to deal with at the moment:)

Dale

--

Mariano
http://marianopeck.wordpress.com

Dale, now I remembered  a question... 

Why rb project entry is PER STONE (that is, $GS_HOME/sys/local/server/stones/devKit_329/projects/RB.ston) while the other projects loaded by tODE (for example, Ston) are shared (that is, $GS_HOME/GsDevKit_home/sys/default/server/projects/* )  

?

Thanks in advance, 

On Mon, Nov 23, 2015 at 5:47 PM, Dale Henrichs <[hidden email]> wrote:

BTW, if any other GsDevKit_home user wants to pick up the RB bugfixes do the following:

  updateGsDevKit -st

update shared repos and tODE repos ... do not update clients as RB is only used on the server.

Then for each of the tODE stones you are using (and want updated):

  todeUpdate <stone-name>

Dale

On 11/23/2015 12:36 PM, Dale Henrichs wrote:

Tests were green, so I merged your pull request ... thanks!

Dale

On 11/23/2015 11:47 AM, Mariano Martinez Peck wrote:

OK Dale, 

Can you check if everything went correct with my PR?

Thanks

On Mon, Nov 23, 2015 at 3:15 PM, Dale Henrichs <[hidden email]> wrote:

On 11/23/2015 05:12 AM, Mariano Martinez Peck wrote:

On Sun, Nov 22, 2015 at 4:17 PM, Dale Henrichs <[hidden email]> wrote:

Cool ....

Now, tODE only loads AST-Core and I just checked and there _was_ only one test error (a test problem that I just fixed), so you must be using other parts of RB ....

Perhaps we could do a mini sprint for RB and get the tests passing for more of the system?

If you are interested perhaps you could let me know what sub-systems you are using and we could divvy up the tests?

I am supposed to be using the AST-Core package. However, it seems that even in Pharo, there is no test that covers my scenario.

haha, well this cuts down the scope of the RB sprint ... the 100's of faiing tests were in the other parts of RB that aren't loaded with AST-Core....

Before I fork and commit the fix, let me ask...shouldn't we move RB from dalehenrichs into gsDevKitHome?   even more because it's even used by tODE right?  I guess you will need to update a couple of things of gsDevKit_home scripts in order to get the new repo, but probably worth.

Anyway, let me know what do you think. 

I would say that you can go ahead and fork my repo ... when It is moved to GsDevKit, ownership will be re-assigned so the repo will move and your fork will point to the GsDevKit version ... to keep old things working, I will make a new fork of rb ....

replacing the cloned repo will be a bit dicier but can be done with a little work .. something that I don't necessarily want to have to deal with at the moment:)

Dale

--

Mariano
http://marianopeck.wordpress.com

--

Mariano
http://marianopeck.wordpress.com