Re: [Pharo-dev] AST node replacement API

[Denis Kudriashov]

2017-01-23 17:00 GMT+01:00 Yuriy Tymchuk :

> I think that we hacked too much :). I like how ParsetreeRewriter is
> working, but I think that it would be nice to have some API to say: ok, I
> have this node, now I want to replace it (maybe it should generate a new
> changed tree), so the pattern syntax would be more of a nice interface
> instead of a single entry point.
>
> Anyway, I also think that this substitution without reformatting is really
> cool. I need to double check it, because I think that many uses do not use
> auto fix from lint rules because it reformats their code.
>

Yes, I also hate extract temp/method because it reformats original method

Re: [Pharo-dev] AST node replacement API

[John Brant]

On 01/23/2017 10:00 AM, Yuriy Tymchuk wrote:

I think that we hacked too much :). I like how ParsetreeRewriter is working, 
but I think that it would be nice to have some API to say: ok, I have this 
node, now I want to replace it (maybe it should generate a new changed tree), 
so the pattern syntax would be more of a nice interface instead of a single 
entry point.

Anyway, I also think that this substitution without reformatting is really 
cool. I need to double check it, because I think that many uses do not use auto 
fix from lint rules because it reformats their code.


The RB works at the AST level, so you always need to replace a node. If 
you want to specify your own custom source when doing a replacement, you 
could create a method like this:



RBProgramNode>>replaceWith: aNode andSource: aString
| method |
parent ifNil: [ self error: 'This node doesn''t have a parent' ].
method := self methodNode.
method notNil
ifTrue: [ method map: self to: aNode ].
aNode parent: self parent.
self addReplacement: (RBStringReplacement
replaceFrom: self start
to: self stop
with: aString).
parent replaceNode: self withNode: aNode


Then you could use it with something like this:


| source method |
source := 'method
^1'.
method := RBParser parseMethod: source.
method body statements first value replaceWith: (RBParser 
parseExpression: '2 - 1') andSource: '2

-
1'.
method newSource


Here, the "-" and "1" should appear on separate lines. However, if you 
gave invalid source, you get the formatted version:



| source method |
source := 'method
^1'.
method := RBParser parseMethod: source.
method body statements first value replaceWith: (RBParser 
parseExpression: '2 - 1') andSource: '2

-
2'.
method newSource


Here the "2 - 2" expression in the replacement source, isn't equal to 
the "2 - 1" expression node that was provided so it uses the formatted 
code of the parse tree.



John Brant

Re: [Pharo-dev] AST node replacement API

[Yuriy Tymchuk]

I think that we hacked too much :). I like how ParsetreeRewriter is working, 
but I think that it would be nice to have some API to say: ok, I have this 
node, now I want to replace it (maybe it should generate a new changed tree), 
so the pattern syntax would be more of a nice interface instead of a single 
entry point.

Anyway, I also think that this substitution without reformatting is really 
cool. I need to double check it, because I think that many uses do not use auto 
fix from lint rules because it reformats their code.

Cheers.
Uko

> On 23 Jan 2017, at 16:28, John Brant  wrote:
> 
> 
> 
> On 01/23/2017 07:48 AM, Yuriy Tymchuk wrote:
>> Hi everyone,
>> 
>> does anyone have a better knowledge about replacing nodes in AST? Because 
>> what I saw is that there are methods like replaceSourceWith: that can be 
>> sent to an AST node with another AST node as a parameter. But this is not 
>> enough to get a new source code. Because when you ask the AST for newSource 
>> it generates a new source by taking replacements into account, parses the 
>> new source, compares it with itself and in case the new parse tree is equal 
>> it returns the new source. So it’s not enough to just use replaceSourceWith: 
>> but you also have to replace the current node with a new one it AST.
>> 
>> Now the question is: am I doing something wrong, or we have strange API 
>> because we need to replace both source and node, replacing only one of them 
>> does not work…
> 
> 
> This code was created so that some refactoring could be performed without 
> having to format the entire parse tree. For example, converting direct 
> variable access to use accessors should be able to rewrite the code without 
> having to reformat.
> 
> When you perform the replaceWith: on a node or the rewriter performs a node 
> replacement, it calls the replaceMethodSource: method. That method calls the 
> replaceSourceWith: method (replaceSourceWith: is a "private" method). The 
> replaceSourceWith: methods test the source/replacement nodes for certain 
> patterns. If the replacement fits the condition of the pattern, then it adds 
> the appropriate text edit operations that should transform the old source 
> into the new source. If the replaceSourceWith: doesn't recognize what is 
> being done, then it dispatches the replaceSourceFrom: message to the 
> replacement node. Currently, this handles cases were any section of code is 
> being replaced with a variable or literal.
> 
> Anyway, after all replacements are performed, it gets the new source like you 
> explain. It performs a sanity check on the edited source. If it isn't parse 
> tree equivalent to the rewritten tree, then it assumes that something went 
> wrong in a replaceSourceWith: or replaceSourceFrom: method and uses the 
> formatted code of the rewritten tree instead.
> 
> 
> John Brant
> 

Re: [Pharo-dev] AST node replacement API

[John Brant]

On 01/23/2017 07:48 AM, Yuriy Tymchuk wrote:

Hi everyone,

does anyone have a better knowledge about replacing nodes in AST? Because what 
I saw is that there are methods like replaceSourceWith: that can be sent to an 
AST node with another AST node as a parameter. But this is not enough to get a 
new source code. Because when you ask the AST for newSource it generates a new 
source by taking replacements into account, parses the new source, compares it 
with itself and in case the new parse tree is equal it returns the new source. 
So it’s not enough to just use replaceSourceWith: but you also have to replace 
the current node with a new one it AST.

Now the question is: am I doing something wrong, or we have strange API because 
we need to replace both source and node, replacing only one of them does not 
work…



This code was created so that some refactoring could be performed 
without having to format the entire parse tree. For example, converting 
direct variable access to use accessors should be able to rewrite the 
code without having to reformat.


When you perform the replaceWith: on a node or the rewriter performs a 
node replacement, it calls the replaceMethodSource: method. That method 
calls the replaceSourceWith: method (replaceSourceWith: is a "private" 
method). The replaceSourceWith: methods test the source/replacement 
nodes for certain patterns. If the replacement fits the condition of the 
pattern, then it adds the appropriate text edit operations that should 
transform the old source into the new source. If the replaceSourceWith: 
doesn't recognize what is being done, then it dispatches the 
replaceSourceFrom: message to the replacement node. Currently, this 
handles cases were any section of code is being replaced with a variable 
or literal.


Anyway, after all replacements are performed, it gets the new source 
like you explain. It performs a sanity check on the edited source. If it 
isn't parse tree equivalent to the rewritten tree, then it assumes that 
something went wrong in a replaceSourceWith: or replaceSourceFrom: 
method and uses the formatted code of the rewritten tree instead.



John Brant

Re: [Pharo-dev] AST node replacement API

[Nicolai Hess]

2017-01-23 14:48 GMT+01:00 Yuriy Tymchuk :

> Hi everyone,
>
> does anyone have a better knowledge about replacing nodes in AST? Because
> what I saw is that there are methods like replaceSourceWith: that can be
> sent to an AST node with another AST node as a parameter. But this is not
> enough to get a new source code. Because when you ask the AST for newSource
> it generates a new source by taking replacements into account, parses the
> new source, compares it with itself and in case the new parse tree is equal
> it returns the new source. So it’s not enough to just use
> replaceSourceWith: but you also have to replace the current node with a new
> one it AST.
>
> Now the question is: am I doing something wrong, or we have strange API
> because we need to replace both source and node, replacing only one of them
> does not work…
>

The API is strange : )
The nodeMapping and replacements are used by the tree rewriter. And the way
the treerewriter works, it does not rewrite a AST-Tree but generates a new
method by compiling the returned
value, and it works on the returned value, the new/reformatted source text,
not on a changed AST-Tree.
>From the rewriter point of view (and some other parts of RB), the AST is
more like a immutable structure. Only since when we started to use the
RB-AST for the compiler, we started
to use the AST for other purposes

(remember when we got into trouble, because some lint rules from critic
browser were modifying the AST and we used that modified source as the
method source, although the actual
method source didn't changed )


>
> Uko
>