Issue 6648 in pharo: Renaming class refactoring breaks the system

Status: Accepted
Owner: [hidden email]
Labels: Type-Bug Milestone-2.0 Target-Nautilus Importance-High

New issue 6648 by [hidden email]: Renaming class refactoring breaks the  
system
http://code.google.com/p/pharo/issues/detail?id=6648

DON'T DO THIS IN AN IMAGE CONTAINING VALUABLE CHANGES!

Steps to reproduce:
1. Select any class in the Nautilus browser.
2. Right-click -> Refactoring -> Class refactoring -> Rename.. (If you can,  
menu selection using a mouse is somewhat iffy if submenus have to be  
displayed on left )
3. Chose any name
4. Click accept in changes browser
5) Select any Author name if needed, and proceed

Congrats, you now get to see the System error handling failed overlay!



_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #1 on issue 6648 by [hidden email]: Renaming class  
refactoring breaks the system
http://code.google.com/p/pharo/issues/detail?id=6648

Can you be more specific about the error message that appears?
I think we fixed this in the latest version.


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #2 on issue 6648 by [hidden email]: Renaming class refactoring  
breaks the system
http://code.google.com/p/pharo/issues/detail?id=6648

Not in 20278 on Jenkins at least.
NautilusUU >> buildPackageHierarcyFor:
eventually leading to RPackage classesForClassTag: and a SystemDictionary  
errorKeyNotFound: (of the old class name)

In other words, it finds the class name in RPackage, then fails to find it  
in the environment, probably because the RB rewriting ends up in a place  
that modifies the system without proper announcements.

BTW; It's still easily reproducable with the above 5 steps.


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #3 on issue 6648 by [hidden email]: Renaming class refactoring  
breaks the system
http://code.google.com/p/pharo/issues/detail?id=6648

I mean, observing the effects and checking if the issue is fixed, that is :)


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #4 on issue 6648 by [hidden email]: Renaming class  
refactoring breaks the system
http://code.google.com/p/pharo/issues/detail?id=6648

thanks for the details. looks like yet another rpackage-tags bug.


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Updates:
        Status: FixReviewNeeded

Comment #5 on issue 6648 by [hidden email]: Renaming class  
refactoring breaks the system
http://code.google.com/p/pharo/issues/detail?id=6648

Name:  
SLICE-Issue-6648-Renaming-class-refactoring-breaks-the-system-CamilloBruni.1
Author: CamilloBruni
Time: 11 September 2012, 1:19:00.419 pm
UUID: e0489f91-5919-44c6-aea4-5823199b4709
Ancestors:
Dependencies: Monticello-CamilloBruni.690, RPackage-CamilloBruni.21,  
RPackage-Core-CamilloBruni.167

- properly clear classes from tags
- refactored #classes instvar access in RPackage to go only via methods
- refactored some RPackage methods to reduce code duplication


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Updates:
        Status: MonkeyIsChecking

Comment #6 on issue 6648 by [hidden email]: Renaming class  
refactoring breaks the system
http://code.google.com/p/pharo/issues/detail?id=6648#c6

The Monkey is currently checking this issue. Please don't change it!


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Updates:
        Status: WorkNeeded

Comment #7 on issue 6648 by [hidden email]: Renaming class  
refactoring breaks the system
http://code.google.com/p/pharo/issues/detail?id=6648#c7

Monkey went bananas:
--------------------
Error while loading  
SLICE-Issue-6648-Renaming-class-refactoring-breaks-the-system-CamilloBruni.1  
from http://ss3.gemstone.com/ss/PharoInbox:
        Error: Could not find version 'RPackage-EstebanLorenzano.20'. Maybe you  
need to add a repository?
  1: MCRepositoryGroup(Object)>>error:
  2: [self error: 'Could not find version ' , aVersionInfo name  
printString , '. Maybe you need to add a repository?'] in  
MCRepositoryGroup>>versionWithInfo:
  3: MCRepositoryGroup>>versionWithInfo:ifNone:
  4: MCRepositoryGroup>>versionWithInfo:
  5: MCWorkingCopy>>findSnapshotWithVersionInfo:
  6: MCMergeRecord>>ancestorSnapshot
  7: MCMergeRecord>>mergePatch
  8: [:ea | merger applyPatch: ea mergePatch] in  
MCVersionMerger>>gatherChanges
  9: OrderedCollection>>do:
10: MCVersionMerger>>gatherChanges
        ...
----------------------------------------------------------
Loaded Source:  
SLICE-Issue-6648-Renaming-class-refactoring-breaks-the-system-CamilloBruni.1  
from http://ss3.gemstone.com/ss/PharoInbox
Tested using Pharo-2.0-20278-a on CoInterpreter  
VMMaker-oscog-IgorStasenko.162 uuid: e4554f9a-cc90-4826-a807-ac282b782fe4  
Sep  7 2012
StackToRegisterMappingCogit VMMaker-oscog-IgorStasenko.162 uuid:  
e4554f9a-cc90-4826-a807-ac282b782fe4 Sep  7 2012
https://git.gitorious.org/cogvm/blessed.git Commit:  
aeb0705cde4b8fc57cb262dc051c5ee6dfa72a14 Date: 2012-07-26 16:38:48 +0200  
By: Igor Stasenko <[hidden email]>


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Updates:
        Status: FixReviewNeeded

Comment #8 on issue 6648 by [hidden email]: Renaming class  
refactoring breaks the system
http://code.google.com/p/pharo/issues/detail?id=6648

(No comment was entered for this change.)


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #9 on issue 6648 by [hidden email]: Renaming class  
refactoring breaks the system
http://code.google.com/p/pharo/issues/detail?id=6648

recommitted again...


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #10 on issue 6648 by [hidden email]: Renaming class refactoring  
breaks the system
http://code.google.com/p/pharo/issues/detail?id=6648

This is kinda weird, I tried the merge behaviour manually and got the same  
message.
The funny thing is, RPackage-EstebanLorenzano.20 was the current version in  
the image... So, rather than checking against the current installed  
(unmodified) version, it would rather download and compare against  
another .mcz?

Also a bit of code review:
failIfMetaclassName:
- Should give a better error message
- When the thing it's being used for is that what is passed is in fact a  
valid class name and not a metaclass name, why not rewrite it as, say  
assertValidClassName: ?
and return something like self error: aClassName, 'is not a valid class  
name!' if not?

Makes more sense to me than testing specifically against a single known  
failure case at least...

Also, the split between definedSelectors/extensionDefinedSelectors  
instvars...
It seems to introduce a lot of complexity, is the case where you ask for  
extended selectors really that common that it needs a O(1) rather than O(n)  
(where n is number of defined classes) lookup you'd have with single  
definedSelectors/definedClasses dictionary/Set?

(Related; Why isn't classes a Set to begin with, with conversion to an  
ordered collection suitable for display being explicit, instead of the  
other way around?)

Would f.ex. remove the split between removeClassDefinitionName: and  
removeClassDefinitionNamed: (which, I guess the renaming to  
removeClassDefinitionWithoutCheckingMethods: is at least a step in a better  
direction)


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Updates:
        Status: WorkNeeded

Comment #11 on issue 6648 by [hidden email]: Renaming class refactoring  
breaks the system
http://code.google.com/p/pharo/issues/detail?id=6648

After loading the slice in 20278, and performing steps 1-5, I still get an  
emergency evaluater with the same error messages as before btw.


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #12 on issue 6648 by [hidden email]: Renaming class  
refactoring breaks the system
http://code.google.com/p/pharo/issues/detail?id=6648

- there are only two possible selectors coming in, either class names or  
metaclass names.
   so there is no need to test for something else than a space in the name  
(I know beautiful, but the fastet way to it)
- sure why not, feel free to change the name of that method
-  changing definedSelectors or any of that is tricky, since esteban is  
working on another fix which will most probably fail with too much  
refactorings...
- #classes is a Set, just check the #initialize method
- removeClassDefinitionName/d I don't like them either, though no clue why  
we have 2 of them, so I leave them for now...


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Updates:
        Status: FixReviewNeeded

Comment #13 on issue 6648 by [hidden email]: Renaming class  
refactoring breaks the system
http://code.google.com/p/pharo/issues/detail?id=6648

In top-level packages it works
Classes in packages in sublevels don't work, though I don't get an  
emergency evaluator anymore...


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #14 on issue 6648 by [hidden email]: Renaming class  
refactoring breaks the system
http://code.google.com/p/pharo/issues/detail?id=6648

ah yes i do :P



_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #15 on issue 6648 by [hidden email]: Renaming class refactoring  
breaks the system
http://code.google.com/p/pharo/issues/detail?id=6648

"There are only two possible selectors coming in"
As one of those is an error, only one should really be coming in :)

My point was, isn't it then better to check that the known-to-be correct  
version is what you have, and raise an error if that's not the case, rather  
than detect the one (known at this point) error case and then raise an  
error?


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #16 on issue 6648 by [hidden email]: Renaming class  
refactoring breaks the system
http://code.google.com/p/pharo/issues/detail?id=6648

Name:  
SLICE-Issue-6648-Renaming-class-refactoring-breaks-the-system-CamilloBruni.3
Author: CamilloBruni
Time: 11 September 2012, 3:49:32.817 pm
UUID: e856e5c6-282a-430f-a46c-bac782f6149a
Ancestors:  
SLICE-Issue-6648-Renaming-class-refactoring-breaks-the-system-CamilloBruni.2
Dependencies: Monticello-CamilloBruni.690, RPackage-CamilloBruni.22,  
RPackage-Core-CamilloBruni.168

more changes
- added hierarchical RPackage lookup


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #17 on issue 6648 by [hidden email]: Renaming class  
refactoring breaks the system
http://code.google.com/p/pharo/issues/detail?id=6648

"Also, the split between definedSelectors/extensionDefinedSelectors  
instvars...
It seems to introduce a lot of complexity, is the case where you ask for  
extended selectors really that common that it needs a O(1) rather than O(n)  
(where n is number of defined classes) lookup you'd have with single  
definedSelectors/definedClasses dictionary/Set?"

this part of RPackage is stable, covered by tests and it is not complex so  
I would not change it.


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Updates:
        Blockedon: pharo:6654

Comment #18 on issue 6648 by [hidden email]: Renaming class  
refactoring breaks the system
http://code.google.com/p/pharo/issues/detail?id=6648

(No comment was entered for this change.)


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Updates:
        Status: MonkeyIsChecking

Comment #19 on issue 6648 by [hidden email]: Renaming class  
refactoring breaks the system
http://code.google.com/p/pharo/issues/detail?id=6648#c19

The Monkey is currently checking this issue. Please don't change it!


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker