[Experiment] Automatic rewriting of sends to deprecated methods
Hi, Small experiment: 1) add this method to Object: deprecated: anExplanationString rule: aRule | builder ast rewriteRule method | builder := RBCompositeRefactoryChange named: 'deprecation'. method := thisContext sender sender method. ast := method ast copy. rewriteRule := RBParseTreeRewriter new replace: aRule key with: aRule value. (rewriteRule executeTree: ast) ifTrue: [ builder compile: rewriteRule tree formattedCode in: method methodClass classified: method protocol ]. builder execute. 2) to see it in action, add it to #ifNotNilDo: in UndefinedObject: ifNotNilDo: aBlock "Please use #ifNotNil: instead" self deprecated: 'Please use #ifNotNil: instead' rule: '`@receiver ifNotNilDo: `@statements'-> '`@receiver ifNotNil: `@statements'. ^ self â> open a browser and see how it fixes code magically. The only problem is that it does a whole-method refactoring while it should only affect the node that triggered the deprecation. Else there could be wrong transformations if the same selector is used but only one of the implementations need to be rewritten, while if we are able to rewrite the exact sender, we can do it fully automatically. Marcus
Is this based on Mark's Rewrite Tool? On Sun, Aug 30, 2015 at 10:27 AM, Marcus Denker <marcus.denker@inria.fr> wrote:
Hi,
Small experiment:
1) add this method to Object:
deprecated: anExplanationString rule: aRule | builder ast rewriteRule method | builder := RBCompositeRefactoryChange named: 'deprecation'. method := thisContext sender sender method. ast := method ast copy. rewriteRule := RBParseTreeRewriter new replace: aRule key with: aRule value.
(rewriteRule executeTree: ast) ifTrue: [ builder compile: rewriteRule tree formattedCode in: method methodClass classified: method protocol ]. builder execute.
2) to see it in action, add it to #ifNotNilDo: in UndefinedObject:
ifNotNilDo: aBlock "Please use #ifNotNil: instead" self deprecated: 'Please use #ifNotNil: instead' rule: '`@receiver ifNotNilDo: `@statements'-> '`@receiver ifNotNil: `@statements'. ^ self
â> open a browser and see how it fixes code magically.
The only problem is that it does a whole-method refactoring while it should only affect the node that triggered the deprecation. Else there could be wrong transformations if the same selector is used but only one of the implementations need to be rewritten, while if we are able to rewrite the exact sender, we can do it fully automatically.
Marcus
As far as I'm concerned, it's not based on my tool. 30 ÑеÑп. 2015 3:34 пп "Peter Uhnák" <i.uhnak@gmail.com> пиÑе:
Is this based on Mark's Rewrite Tool?
On Sun, Aug 30, 2015 at 10:27 AM, Marcus Denker <marcus.denker@inria.fr> wrote:
Hi,
Small experiment:
1) add this method to Object:
deprecated: anExplanationString rule: aRule | builder ast rewriteRule method | builder := RBCompositeRefactoryChange named: 'deprecation'. method := thisContext sender sender method. ast := method ast copy. rewriteRule := RBParseTreeRewriter new replace: aRule key with: aRule value.
(rewriteRule executeTree: ast) ifTrue: [ builder compile: rewriteRule tree formattedCode in: method methodClass classified: method protocol ]. builder execute.
2) to see it in action, add it to #ifNotNilDo: in UndefinedObject:
ifNotNilDo: aBlock "Please use #ifNotNil: instead" self deprecated: 'Please use #ifNotNil: instead' rule: '`@receiver ifNotNilDo: `@statements'-> '`@receiver ifNotNil: `@statements'. ^ self
â> open a browser and see how it fixes code magically.
The only problem is that it does a whole-method refactoring while it should only affect the node that triggered the deprecation. Else there could be wrong transformations if the same selector is used but only one of the implementations need to be rewritten, while if we are able to rewrite the exact sender, we can do it fully automatically.
Marcus
But it can be integrated with Rewrite Tool ;), itâs not so easy for everyone to write down a rewrite rule from scratch. Yes, I think that having transformation applied without re-styling will be a huge improvement. Because now there is an âautofixâ button, but I donât use it myself as it restyles all the method. Uko
On 30 Aug 2015, at 16:02, Mark Rizun <mrizun@gmail.com> wrote:
As far as I'm concerned, it's not based on my tool.
30 ÑеÑп. 2015 3:34 пп "Peter Uhnák" <i.uhnak@gmail.com <mailto:i.uhnak@gmail.com>> пиÑе: Is this based on Mark's Rewrite Tool?
On Sun, Aug 30, 2015 at 10:27 AM, Marcus Denker <marcus.denker@inria.fr <mailto:marcus.denker@inria.fr>> wrote: Hi,
Small experiment:
1) add this method to Object:
deprecated: anExplanationString rule: aRule | builder ast rewriteRule method | builder := RBCompositeRefactoryChange named: 'deprecation'. method := thisContext sender sender method. ast := method ast copy. rewriteRule := RBParseTreeRewriter new replace: aRule key with: aRule value.
(rewriteRule executeTree: ast) ifTrue: [ builder compile: rewriteRule tree formattedCode in: method methodClass classified: method protocol ]. builder execute.
2) to see it in action, add it to #ifNotNilDo: in UndefinedObject:
ifNotNilDo: aBlock "Please use #ifNotNil: instead" self deprecated: 'Please use #ifNotNil: instead' rule: '`@receiver ifNotNilDo: `@statements'-> '`@receiver ifNotNil: `@statements'. ^ self
â> open a browser and see how it fixes code magically.
The only problem is that it does a whole-method refactoring while it should only affect the node that triggered the deprecation. Else there could be wrong transformations if the same selector is used but only one of the implementations need to be rewritten, while if we are able to rewrite the exact sender, we can do it fully automatically.
Marcus
No, just plain old ParseTreeRewriter.. but integrating Markâs tool would be very nice as a next step.
On 30 Aug 2015, at 14:34, Peter Uhnák <i.uhnak@gmail.com> wrote:
Is this based on Mark's Rewrite Tool?
On Sun, Aug 30, 2015 at 10:27 AM, Marcus Denker <marcus.denker@inria.fr <mailto:marcus.denker@inria.fr>> wrote: Hi,
Small experiment:
1) add this method to Object:
deprecated: anExplanationString rule: aRule | builder ast rewriteRule method | builder := RBCompositeRefactoryChange named: 'deprecation'. method := thisContext sender sender method. ast := method ast copy. rewriteRule := RBParseTreeRewriter new replace: aRule key with: aRule value.
(rewriteRule executeTree: ast) ifTrue: [ builder compile: rewriteRule tree formattedCode in: method methodClass classified: method protocol ]. builder execute.
2) to see it in action, add it to #ifNotNilDo: in UndefinedObject:
ifNotNilDo: aBlock "Please use #ifNotNil: instead" self deprecated: 'Please use #ifNotNil: instead' rule: '`@receiver ifNotNilDo: `@statements'-> '`@receiver ifNotNil: `@statements'. ^ self
â> open a browser and see how it fixes code magically.
The only problem is that it does a whole-method refactoring while it should only affect the node that triggered the deprecation. Else there could be wrong transformations if the same selector is used but only one of the implementations need to be rewritten, while if we are able to rewrite the exact sender, we can do it fully automatically.
Marcus
But the tool of mark is a ui tool so why would you want to get a tool to define a rule in your case? In the future if mark and camille come up with a better rewrite engine then of course we will use it. Le 30/8/15 18:31, Marcus Denker a écrit :
No, just plain old ParseTreeRewriter.. but integrating Markâs tool would be very nice as a next step.
On 30 Aug 2015, at 14:34, Peter Uhnák <i.uhnak@gmail.com <mailto:i.uhnak@gmail.com>> wrote:
Is this based on Mark's Rewrite Tool?
On Sun, Aug 30, 2015 at 10:27 AM, Marcus Denker <marcus.denker@inria.fr <mailto:marcus.denker@inria.fr>> wrote:
Hi,
Small experiment:
1) add this method to Object:
deprecated: anExplanationString rule: aRule | builder ast rewriteRule method | builder := RBCompositeRefactoryChange named: 'deprecation'. method := thisContext sender sender method. ast := method ast copy. rewriteRule := RBParseTreeRewriter new replace: aRule key with: aRule value.
(rewriteRule executeTree: ast) ifTrue: [ builder compile: rewriteRule tree formattedCode in: method methodClass classified: method protocol ]. builder execute.
2) to see it in action, add it to #ifNotNilDo: in UndefinedObject:
ifNotNilDo: aBlock "Please use #ifNotNil: instead" self deprecated: 'Please use #ifNotNil: instead' rule: '`@receiver ifNotNilDo: `@statements'-> '`@receiver ifNotNil: `@statements'. ^ self
â> open a browser and see how it fixes code magically.
The only problem is that it does a whole-method refactoring while it should only affect the node that triggered the deprecation. Else there could be wrong transformations if the same selector is used but only one of the implementations need to be rewritten, while if we are able to rewrite the exact sender, we can do it fully automatically.
Marcus
On 31 Aug 2015, at 08:41, stepharo <stepharo@free.fr> wrote:
But the tool of mark is a ui tool so why would you want to get a tool to define a rule in your case?
Yes, it would be nice to have a tool for creating the rules. But then, one can use Markâs tool as is for this already now for creating the rewrite rule and then copy-and-paste it. An integration could make that even easier, but for me this is good enough for now.
In the future if mark and camille come up with a better rewrite engine then of course we will use it.
Yes. Marcus
Yes to create rules. I thought that when the rule was executed and I was confused. Le 31/8/15 08:46, Marcus Denker a écrit :
On 31 Aug 2015, at 08:41, stepharo <stepharo@free.fr> wrote:
But the tool of mark is a ui tool so why would you want to get a tool to define a rule in your case?
Yes, it would be nice to have a tool for creating the rules. But then, one can use Markâs tool as is for this already now for creating the rewrite rule and then copy-and-paste it. An integration could make that even easier, but for me this is good enough for now.
In the future if mark and camille come up with a better rewrite engine then of course we will use it.
Yes.
Marcus
The only problem is that it does a whole-method refactoring while it should only affect the node that triggered the deprecation. Else there could be wrong transformations if the same selector is used but only one of the implementations need to be rewritten, while if we are able to rewrite the exact sender, we can do it fully automatically.
I think using #sourceNodeExecuted we could do it like this: deprecated: anExplanationString rule: aRule | builder rewriteRule method context node | builder := RBCompositeRefactoryChange named: 'deprecation'. method := thisContext sender sender method. context := thisContext sender sender. node := context sourceNodeExecuted. rewriteRule := RBParseTreeRewriter new replace: aRule key with: aRule value. (rewriteRule executeTree: node copy) ifTrue: [ node replaceWith: rewriteRule tree. builder compile: method ast formattedCode in: method methodClass classified: method protocol ]. builder execute. And a question is if we need #formattedCode⦠all refactoring normally try to update white-space information. Need to check. Marcus
On 31 Aug 2015, at 08:52, Marcus Denker <marcus.denker@inria.fr> wrote:
The only problem is that it does a whole-method refactoring while it should only affect the node that triggered the deprecation. Else there could be wrong transformations if the same selector is used but only one of the implementations need to be rewritten, while if we are able to rewrite the exact sender, we can do it fully automatically.
I think using #sourceNodeExecuted we could do it like this:
even simpler: deprecated: anExplanationString rule: aRule | rewriteRule method context node | context := thisContext sender sender. method := context method. node := context sourceNodeExecuted. rewriteRule := RBParseTreeRewriter new replace: aRule key with: aRule value. (rewriteRule executeTree: node) ifFalse: [ ^self ]. node replaceWith: rewriteRule tree. method methodClass compile: method ast formattedCode classified: method protocol. It seems to work but I have not tested it too much. Marcus
On Mon, Aug 31, 2015 at 8:41 AM, stepharo <stepharo@free.fr> wrote:
But the tool of mark is a ui tool so why would you want to get a tool to define a rule in your case?
You can use the UI to create the rule, but the application of the rule can be scripted.
Yes, it would be nice to have a tool for creating the rules. But then, one can use Markâs tool as is for this already now for creating the rewrite rule and then copy-and-paste it. An integration could make that even easier, but for me this is good enough for now.
RewriteTool creates classes with the rules, so I don't see the need for copy-pasting. Why not just self deprecated: anExplanationString rule: aRuleClassGeneratedByRewriteTool self
On 31 Aug 2015, at 10:18, Peter Uhnák <i.uhnak@gmail.com> wrote:
On Mon, Aug 31, 2015 at 8:41 AM, stepharo <stepharo@free.fr <mailto:stepharo@free.fr>> wrote: But the tool of mark is a ui tool so why would you want to get a tool to define a rule in your case?
You can use the UI to create the rule, but the application of the rule can be scripted.
Yes, it would be nice to have a tool for creating the rules. But then, one can use Markâs tool as is for this already now for creating the rewrite rule and then copy-and-paste it. An integration could make that even easier, but for me this is good enough for now.
RewriteTool creates classes with the rules, so I don't see the need for copy-pasting. Why not just
self deprecated: anExplanationString rule: aRuleClassGeneratedByRewriteTool
good idea, this way the rule is reusable for a code critique rule (for those cases where it makes sense, e.g. for ifNotNil: yes, as it is likely unique, but for #at: on SmalltalkImage, no ;-) Marcus
I do not get why you want to link the rewrite engine with deprecation at runtime. We should not have such dependencies. To me it is insane. Stef Le 31/8/15 10:18, Peter Uhnák a écrit :
On Mon, Aug 31, 2015 at 8:41 AM, stepharo <stepharo@free.fr <mailto:stepharo@free.fr>> wrote:
But the tool of mark is a ui tool so why would you want to get a tool to define a rule in your case?
You can use the UI to create the rule, but the application of the rule can be scripted.
Yes, it would be nice to have a tool for creating the rules. But then, one can use Markâs tool as is for this already now for creating the rewrite rule and then copy-and-paste it. An integration could make that even easier, but for me this is good enough for now.
RewriteTool creates classes with the rules, so I don't see the need for copy-pasting. Why not just
self deprecated: anExplanationString rule: aRuleClassGeneratedByRewriteTool
self
On 01 Sep 2015, at 08:30, stepharo <stepharo@free.fr> wrote:
I do not get why you want to link the rewrite engine with deprecation at runtime. We should not have such dependencies. To me it is insane.
It can be made pluggable, of course. I donât want to have that there in a way that it requires the refactoring engine to be present all the time. If itâs there â> transform. If not â> do what it does now. But just think how much this simplifies the experience of people loading old code into a new Pharo version: it just keeps running and fixes itself! Itâs that kind of things we need to be able to evolve the system. Imagine, this is a functionality (I think) no other system has! Marcus
Le 1 sept. 2015 à 08:35, Marcus Denker a écrit :
On 01 Sep 2015, at 08:30, stepharo <stepharo@free.fr> wrote:
I do not get why you want to link the rewrite engine with deprecation at runtime. We should not have such dependencies. To me it is insane.
It can be made pluggable, of course. I donât want to have that there in a way that it requires the refactoring engine to be present all the time.
If itâs there â> transform. If not â> do what it does now.
But just think how much this simplifies the experience of people loading old code into a new Pharo version: it just keeps running and fixes itself! Itâs that kind of things we need to be able to evolve the system.
Imagine, this is a functionality (I think) no other system has!
I will not like a feature where it transforms my code without asking. I would prefer that a diff browser pops up and asks me if changes are ok. Automatic rewrite of code often forgot to check some specific cases and you end with bugs in your code if there is no review. The idea is great but the choice of changing the code or not has to be in the user hands.
On 01 Sep 2015, at 09:30, Christophe Demarey <Christophe.Demarey@inria.fr> wrote:
Le 1 sept. 2015 à 08:35, Marcus Denker a écrit :
On 01 Sep 2015, at 08:30, stepharo <stepharo@free.fr> wrote:
I do not get why you want to link the rewrite engine with deprecation at runtime. We should not have such dependencies. To me it is insane.
It can be made pluggable, of course. I donât want to have that there in a way that it requires the refactoring engine to be present all the time.
If itâs there â> transform. If not â> do what it does now.
But just think how much this simplifies the experience of people loading old code into a new Pharo version: it just keeps running and fixes itself! Itâs that kind of things we need to be able to evolve the system.
Imagine, this is a functionality (I think) no other system has!
I will not like a feature where it transforms my code without asking.
Yes, I want to have a dialog as a default, too. It should have the possibility to review, to do your own change *and* to continue and turn off the dialog for future transforms of this session. But it is good to know that people donât think this is that important. The result is that I will deprecate things with far less guilt using the current scheme ;-) Marcus
Le 1 sept. 2015 à 09:37, Marcus Denker a écrit :
On 01 Sep 2015, at 09:30, Christophe Demarey <Christophe.Demarey@inria.fr> wrote:
Le 1 sept. 2015 à 08:35, Marcus Denker a écrit :
On 01 Sep 2015, at 08:30, stepharo <stepharo@free.fr> wrote:
I do not get why you want to link the rewrite engine with deprecation at runtime. We should not have such dependencies. To me it is insane.
It can be made pluggable, of course. I donât want to have that there in a way that it requires the refactoring engine to be present all the time.
If itâs there â> transform. If not â> do what it does now.
But just think how much this simplifies the experience of people loading old code into a new Pharo version: it just keeps running and fixes itself! Itâs that kind of things we need to be able to evolve the system.
Imagine, this is a functionality (I think) no other system has!
I will not like a feature where it transforms my code without asking.
Yes, I want to have a dialog as a default, too. It should have the possibility to review, to do your own change *and* to continue and turn off the dialog for future transforms of this session.
perfect :)
But it is good to know that people donât think this is that important. The result is that I will deprecate things with far less guilt using the current scheme ;-)
with the support you will provide for deprecations, for sure, you will be able to deprecate a lot more things. People will care less because there will be support to convert their code.
Yuriy had an idea that QualityAssistant would offer option to rewrite your code, when you are sending a deprecated message. So you would have MyObject>>myMethod self deprecated: 'whatevs' rule: RewriteToolRule and then in your code which sends this method, the QA would inform you of the deprecation and offer you rewrite. This is safe (it shows Changes dialog first), and practical, because you can publish the rewrite rules as part of your code and users can update easily whenever they are ready. Peter On Tue, Sep 1, 2015 at 9:37 AM, Marcus Denker <marcus.denker@inria.fr> wrote:
On 01 Sep 2015, at 09:30, Christophe Demarey < Christophe.Demarey@inria.fr> wrote:
Le 1 sept. 2015 à 08:35, Marcus Denker a écrit :
On 01 Sep 2015, at 08:30, stepharo <stepharo@free.fr> wrote:
I do not get why you want to link the rewrite engine with deprecation
at runtime.
We should not have such dependencies. To me it is insane.
It can be made pluggable, of course. I donât want to have that there in a way that it requires the refactoring engine to be present all the time.
If itâs there â> transform. If not â> do what it does now.
But just think how much this simplifies the experience of people loading old code into a new Pharo version: it just keeps running and fixes itself! Itâs that kind of things we need to be able to evolve the system.
Imagine, this is a functionality (I think) no other system has!
I will not like a feature where it transforms my code without asking.
Yes, I want to have a dialog as a default, too. It should have the possibility to review, to do your own change *and* to continue and turn off the dialog for future transforms of this session.
But it is good to know that people donât think this is that important. The result is that I will deprecate things with far less guilt using the current scheme ;-)
Marcus
On 01 Sep 2015, at 09:51, Peter Uhnák <i.uhnak@gmail.com> wrote:
Yuriy had an idea that QualityAssistant would offer option to rewrite your code, when you are sending a deprecated message. So you would have
MyObject>>myMethod self deprecated: 'whatevs' rule: RewriteToolRule
and then in your code which sends this method, the QA would inform you of the deprecation and offer you rewrite. This is safe (it shows Changes dialog first), and practical, because you can publish the rewrite rules as part of your code and users can update easily whenever they are ready.
Yes! Marcus
It can be made pluggable, of course. I donât want to have that there in a way that it requires the refactoring engine to be present all the time.
If itâs there â> transform. If not â> do what it does now.
But just think how much this simplifies the experience of people loading old code into a new Pharo version: it just keeps running and fixes itself! Itâs that kind of things we need to be able to evolve the system.
Imagine, this is a functionality (I think) no other system has!
Yes but we should make it pluggable. I would like something that says before all the tests were green and after these automatic changes they are also green :)
I will not like a feature where it transforms my code without asking. I would prefer that a diff browser pops up and asks me if changes are ok. Automatic rewrite of code often forgot to check some specific cases and you end with bugs in your code if there is no review. The idea is great but the choice of changing the code or not has to be in the user hands.
Imagine, this is a functionality (I think) no other system has!
Yes but we should make it pluggable.
Yes!
I would like something that says before all the tests were green and after these automatic changes they are also green :)
Exactly, that's what I implemented: you run the tests and all code covered will be transformed automaticallly *and* with the gurantee that it is correct. There are no false positives as we only transform when the deprecated method is called. It's a "perfect" transformation, but of course only for non-dead code. Marcus
May be in that can we should have a deprecated: andTransform: message Now the question is when do you build a rule. We should discuss with gustavo and his macro recorder. Stef
Imagine, this is a functionality (I think) no other system has!
Yes but we should make it pluggable.
Yes!
I would like something that says before all the tests were green and after these automatic changes they are also green :)
Exactly, that's what I implemented: you run the tests and all code covered will be transformed automaticallly *and* with the gurantee that it is correct. There are no false positives as we only transform when the deprecated method is called. It's a "perfect" transformation, but of course only for non-dead code.
Marcus
On Wed, Sep 2, 2015 at 2:13 PM, Marcus Denker <marcus.denker@inria.fr> wrote:
Imagine, this is a functionality (I think) no other system has!
Yes but we should make it pluggable.
Yes!
I would like something that says before all the tests were green and after these automatic changes they are also green :)
Exactly, that's what I implemented: you run the tests and all code covered will be transformed automaticallly
Ahh. I thought you were thinking of doing it in normally executing code. Doing it while running tests makes more sense. It only occurs on a specific user action. If you start with clean packages, all changes can be reviewed. cheers -ben
On 01-09-15 08:35, Marcus Denker wrote:
But just think how much this simplifies the experience of people loading old code into a new Pharo version: it just keeps running and fixes itself! Itâs that kind of things we need to be able to evolve the system.
This is a crucial step to support continuous improvement indeed. Just-in-time repeatable refactoring, reducing the cost of change Stephan
We could ship a specific package with rules for each version of Pharo. We did it with andre's work and we should continue. Le 1/9/15 10:23, Stephan Eggermont a écrit :
On 01-09-15 08:35, Marcus Denker wrote:
But just think how much this simplifies the experience of people loading old code into a new Pharo version: it just keeps running and fixes itself! Itâs that kind of things we need to be able to evolve the system.
This is a crucial step to support continuous improvement indeed. Just-in-time repeatable refactoring, reducing the cost of change
Stephan
participants (8)
-
Ben Coman -
Christophe Demarey -
Marcus Denker -
Mark Rizun -
Peter Uhnák -
Stephan Eggermont -
stepharo -
Yuriy Tymchuk