tests. It does not generate compilation errors, and will thus only result in
a runtime error or corrupted data, which might be hard to detect.
-\section{Refactoring and testing}\label{testing}
+\section{Refactoring and the importance of testing}\label{testing}
\begin{quote}
If you want to refactor, the essential precondition is having solid
tests.\citing{refactoring}
\end{quote}
-When refactoring, there are roughly two kinds of errors that can be made. There
-are errors that make the code unable to compile, and there are the silent
-errors, only popping up at runtime. Compile-time errors are the nice ones. They
-flash up at the moment they are made (at least when using an IDE), and are
-usually easy to fix. The other kind of error is the dangerous one. It is the
-kind of error introduced in the example of \myref{correctness}. It is an error
-sneaking into your code without you noticing, maybe. For discovering those kind
-of errors when refactoring, it is essential to have good test coverage. It is
-not a way to \emph{prove} that the code is correct, but it is a way to make you
-confindent that it \emph{probably} works as desired. In the context of test
-driven development, the tests are even a way to define how the program is
-supposed to work. It is then, by definition, working if the tests are passing.
+When refactoring, there are roughly three classes of errors that can be made.
+The first class of errors are the ones that make the code unable to compile.
+These \emph{compile-time} errors are of the nicer kind. They flash up at the
+moment they are made (at least when using an IDE), and are usually easy to fix.
+The second class are the \emph{runtime} errors. Although they take a bit longer
+to surface, they usually manifest after some time in an illegal argument
+exception, null pointer exception or similar during the program execution.
+These kind of errors are a bit harder to handle, but at least they will show,
+eventually. Then there are the \emph{behavior-changing} errors. These errors are
+of the worst kind. They do not show up during compilation and they do not turn
+on a blinking red light during runtime either. The program can seem to work
+perfectly fine with them in play, but the business logic can be damaged in ways
+that will only show up over time.
+
+For discovering runtime errors and behavior changes when refactoring, it is
+essential to have good test coverage. Testing in this context means writing
+automated tests. Manual testing may have its uses, but when refactoring, it is
+automated unit testing that dominate. For discovering behavior changes it is
+especially important to have tests that cover potential problems, since these
+kind of errors does not reveal themselves.
+
+Unit testing is not a way to \emph{prove} that a program is correct, but it is a
+way to make you confindent that it \emph{probably} works as desired. In the
+context of test driven development (commonly known as TDD), the tests are even a
+way to define how the program is \emph{supposed} to work. It is then, by
+definition, working if the tests are passing.
If the test coverage for a code base is perfect, then it should, theoretically,
-be risk-free to perform refactorings on it. This is why tests and refactoring
-are such a great match.
+be risk-free to perform refactorings on it. This is why automated tests and
+refactoring are such a great match.
\subsection{Testing the code from correctness section}
+The worst thing that can happen when refactoring is to introduce changes to the
+behavior of a program, as in the example on \myref{correctness}. This example
+may be trivial, but the essence is clear. The only problem with the example is
+that it is not clear how to create automated tests for it, without changing it
+in intrusive ways.
+
+Unit tests, as they are known from the different xUnit frameworks around, are
+only suitable to test the \emph{result} of isolated operations. They can not
+easily (if at all) observe the \emph{history} of a program.
+
+
+\todoin{Write \ldots}
+
+Assuming a sequential (non-concurrent) program:
+
+\begin{minted}{java}
+tracematch (C c, X x) {
+ sym m before:
+ call(* X.m(C)) && args(c) && cflow(within(C));
+ sym n before:
+ call(* X.n()) && target(x) && cflow(within(C));
+ sym setCx after:
+ set(C.x) && target(c) && !cflow(m);
+
+ m n
+
+ { assert x == c.x; }
+}
+\end{minted}
+
+%\begin{minted}{java}
+%tracematch (X x1, X x2) {
+% sym m before:
+% call(* X.m(C)) && target(x1);
+% sym n before:
+% call(* X.n()) && target(x2);
+% sym setX after:
+% set(C.x) && !cflow(m) && !cflow(n);
+%
+% m n
+%
+% { assert x1 != x2; }
+%}
+%\end{minted}
\section{The project}
The aim of this project will be to investigate the relationship between a
\See{hacking_undo_history}
\section{Wishful Thinking}
-
+\todoin{???}
\chapter{Composite Refactorings in Eclipse}
As pointed out in \myref{ch:jdt_refactorings}, the Eclipse JDT refactoring model
is not very well suited for making composite refactorings. Therefore a simple
model using changer objects (of type \type{RefaktorChanger}) is used as an
-abstraction layer on top of the existing Eclipse refactorings.
+abstraction layer on top of the existing Eclipse refactorings, instead of
+extending the \typewithref{org.eclipse.ltk.core.refactoring}{Refactoring} class.
+
+The use of an additional abstraction layer is a deliberate choice. It is due to
+the problem of creating a composite
+\typewithref{org.eclipse.ltk.core.refactoring}{Change} that can handle text
+changes that interfere with each other. Thus, a \type{RefaktorChanger} may, or
+may not, take advantage of one or more existing refactorings, but it is always
+intended to make a change to the workspace.
+
+\subsection{A typical \type{RefaktorChanger}}
+The typical refaktor changer class has two responsibilities, checking
+preconditions and executing the requested changes. This is not too different
+from the responsibilities of an LTK refactoring, with the distinction that a
+refaktor changer also executes the change, while an LTK refactoring is only
+responsible for creating the object that can later be used to do the job.
+
+Checking of preconditions is typically done by an
+\typewithref{no.uio.ifi.refaktor.analyze.analyzers}{Analyzer}. If the
+preconditions validate, the upcoming changes are executed by an
+\typewithref{no.uio.ifi.refaktor.change.executors}{Executor}.
\section{The Extract and Move Method Refactoring}
%The Extract and Move Method Refactoring is implemented mainly using these
unit\typeref{org.eclipse.jdt.core.ICompilationUnit}, the offset into the source
code where the extraction shall start, and the length of the source to be
extracted. Then you have to set the method name for the new method together with
-which access modifier that shall be used and some not so interesting parameters.
+its visibility and some not so interesting parameters.
\subsubsection{The MoveInstanceMethodProcessor Class}
-For the Move Method the processor requires a little more advanced input than
+For the Move Method, the processor requires a little more advanced input than
the class for the Extract Method. For construction it requires a method
-handle\typeref{org.eclipse.jdt.core.IMethod} from the Java Model for the method
-that is to be moved. Then the target for the move have to be supplied as the
-variable binding from a chosen variable declaration. In addition to this, one
-have to set some parameters regarding setters/getters and delegation.
+handle\typeref{org.eclipse.jdt.core.IMethod} for the method that is to be moved.
+Then the target for the move have to be supplied as the variable binding from a
+chosen variable declaration. In addition to this, one have to set some
+parameters regarding setters/getters, as well as delegation.
-To make a whole refactoring from the processor, one have to construct a
-\type{MoveRefactoring} from it.
+To make a working refactoring from the processor, one have to create a
+\type{MoveRefactoring} with it.
\subsection{The ExtractAndMoveMethodChanger Class}
+
The \typewithref{no.uio.ifi.refaktor.changers}{ExtractAndMoveMethodChanger}
-class, that is a subclass of the class
-\typewithref{no.uio.ifi.refaktor.changers}{RefaktorChanger}, is the class
-responsible for composing the \type{ExtractMethodRefactoring} and the
-\type{MoveRefactoring}. Its constructor takes a project
-handle\typeref{org.eclipse.core.resources.IProject}, the method name for the new
-method and a \typewithref{no.uio.ifi.refaktor.utils}{SmartTextSelection}.
-
-A \type{SmartTextSelection} is basically a text
-selection\typeref{org.eclipse.jface.text.ITextSelection} object that enforces
-the providing of the underlying document during creation. I.e. its
-\methodwithref{no.uio.ifi.refaktor.utils.SmartTextSelection}{getDocument} method
-will never return \type{null}.
-
-Before extracting the new method, the possible targets for the move operation is
-found with the help of an
-\typewithref{no.uio.ifi.refaktor.extractors}{ExtractAndMoveMethodPrefixesExtractor}.
-The possible targets is computed from the prefixes that the extractor returns
-from its
-\methodwithref{no.uio.ifi.refaktor.extractors.ExtractAndMoveMethodPrefixesExtractor}{getSafePrefixes}
-method. The changer then choose the most suitable target by finding the most
-frequent occurring prefix among the safe ones. The target is the type of the
-first part of the prefix.
-
-After finding a suitable target, the \type{ExtractAndMoveMethodChanger} first
-creates an \type{ExtractMethodRefactoring} and performs it as explained in
-\myref{executing_refactoring} about the execution of refactorings. Then it
-creates and performs the \type{MoveRefactoring} in the same way, based on the
-changes done by the Extract Method refactoring.
-
-\subsection{The ExtractAndMoveMethodPrefixesExtractor Class}
-This extractor extracts properties needed for building the Extract and Move
-Method refactoring. It searches through the given selection to find safe
-prefixes, and those prefixes form a base that can be used to compute possible
-targets for the move part of the refactoring. It finds both the candidates, in
-the form of prefixes, and the non-candidates, called unfixes. All prefixes (and
-unfixes) are represented by a
+class is a subclass of the class
+\typewithref{no.uio.ifi.refaktor.changers}{RefaktorChanger}. It is responsible
+for analyzing and finding the best target for, and also executing, a composition
+of the Extract Method and Move Method refactorings. This particular changer is
+the one of my changers that is closest to being a true LTK refactoring. It can
+be reworked to be one if the problems with overlapping changes are resolved. The
+changer requires a text selection and the name of the new method, or else a
+method name will be generated. The selection has to be of the type
+\typewithref{no.uio.ifi.refaktor.utils}{CompilationUnitTextSelection}. This
+class is a custom extension to
+\typewithref{org.eclipse.jface.text}{TextSelection}, that in addition to the
+basic offset, length and similar methods, also carry an instance of the
+underlying compilation unit handle for the selection.
+
+\subsubsection{The \type{ExtractAndMoveMethodAnalyzer}}
+The analysis and precondition checking is done by the
+\typewithref{no.uio.ifi.refaktor.analyze.analyzers}{ExtractAnd\-MoveMethodAnalyzer}.
+First is check whether the selection is a valid selection or not, with respect
+to statement boundaries and that it actually contains any selections. Then it
+checks the legality of both extracting the selection and also moving it to
+another class. If the selection is approved as legal, it is analyzed to find the
+presumably best target to move the extracted method to.
+
+For finding the best suitable target the analyzer is using a
+\typewithref{no.uio.ifi.refaktor.analyze.collectors}{PrefixesCollector} that
+collects all the possible candidates for the refactoring. All the non-candidates
+is found by an
+\typewithref{no.uio.ifi.refaktor.analyze.collectors}{UnfixesCollector} that
+collects all the targets that will give some kind of error if used. All prefixes
+(and unfixes) are represented by a
\typewithref{no.uio.ifi.refaktor.extractors}{Prefix}, and they are collected
-into prefix sets.\typeref{no.uio.ifi.refaktor.extractors.PrefixSet}.
-
+into sets of prefixes. The safe prefixes is found by subtracting from the set of
+candidate prefixes the prefixes that is enclosing any of the unfixes. A prefix
+is enclosing an unfix if the unfix is in the set of its sub-prefixes. As an
+example, \texttt{``a.b''} is enclosing \texttt{``a''}, as is \texttt{``a''}. The
+safe prefixes is unified in a \type{PrefixSet}. If a prefix has only one
+occurrence, and is a simple expression, it is considered unsuitable as a move
+target. This occurs in statements such as \texttt{``a.foo()''}. For such
+statements it bares no meaning to extract and move them. It only generates an
+extra method and the calling of it.
+
+\todoin{Clean up sections/subsections.}
+
+\subsubsection{The \type{ExtractAndMoveMethodExecutor}}
+If the analysis finds a possible target for the composite refactoring, it is
+executed by an
+\typewithref{no.uio.ifi.refaktor.change.executors}{ExtractAndMoveMethodExecutor}.
+It is composed of the two executors known as
+\typewithref{no.uio.ifi.refaktor.change.executors}{ExtractMethodRefactoringExecutor}
+and
+\typewithref{no.uio.ifi.refaktor.change.executors}{MoveMethodRefactoringExecutor}.
+The \type{ExtractAndMoveMethodExecutor} is responsible for gluing the two
+together by feeding the \type{MoveMethod\-RefactoringExecutor} with the
+resources needed after executing the extract method refactoring.
+\See{postExtractExecution}
+
+\subsubsection{The \type{ExtractMethodRefactoringExecutor}}
+This executor is responsible for creating and executing an instance of the
+\type{ExtractMethodRefactoring} class. It is also responsible for collecting
+some post execution resources that can be used to find the method handle for the
+extracted method, as well as information about its parameters, including the
+variable they originated from.
+
+\subsubsection{The \type{MoveMethodRefactoringExecutor}}
+This executor is responsible for creating and executing an instance of the
+\type{MoveRefactoring}. The move refactoring is a processor-based refactoring,
+and for the Move Method refactoring it is the \type{MoveInstanceMethodProcessor}
+that is used.
+
+The handle for the method to be moved is found on the basis of the information
+gathered after the execution of the Extract Method refactoring. The only
+information the \type{ExtractMethodRefactoring} is sharing after its execution,
+regarding find the method handle, is the textual representation of the new
+method signature. Therefore it must be parsed, the strings for types of the
+parameters must be found and translated to a form that can be used to look up
+the method handle from its type handle. They have to be on the unresolved
+form.\todo{Elaborate?} The name for the type is found from the original
+selection, since an extracted method must end up in the same type as the
+originating method.
+
+When analyzing a selection prior to performing the Extract Method refactoring, a
+target is chosen. It has to be a variable binding, so it is either a field or a
+local variable/parameter. If the target is a field, it can be used with the
+\type{MoveInstanceMethodProcessor} as it is, since the extracted method still is
+in its scope. But if the target is local to the originating method, the target
+that is to be used for the processor must be among its parameters. Thus the
+target must be found among the extracted method's parameters. This is done by
+finding the parameter information object that corresponds to the parameter that
+was declared on basis of the original target's variable when the method was
+extracted. (The extracted method must take one such parameter for each local
+variable that is declared outside the selection that is extracted.) To match the
+original target with the correct parameter information object, the key for the
+information object is compared to the key from the original target's binding.
+The source code must then be parsed to find the method declaration for the
+extracted method. The new target must be found by searching through the
+parameters of the declaration and choose the one that has the same type as the
+old binding from the parameter information object, as well as the same name that
+is provided by the parameter information object.
+
+
+\subsection{Finding the IMethod}\label{postExtractExecution}
+\todoin{Rename section. Write.}
+
+\subsection{Property collectors}
The prefixes and unfixes are found by property
collectors\typeref{no.uio.ifi.refaktor.extractors.collectors.PropertyCollector}.
A property collector follows the visitor pattern\citing{designPatterns} and is
\subsubsection{The PrefixesCollector}
The \typewithref{no.uio.ifi.refaktor.extractors.collectors}{PrefixesCollector}
-is of type \type{PropertyCollector}. It visits expression
+finds prefixes that makes up tha basis for calculating move targets for the
+Extract and Move Method refactoring. It visits expression
statements\typeref{org.eclipse.jdt.core.dom.ExpressionStatement} and creates
prefixes from its expressions in the case of method invocations. The prefixes
found is registered with a prefix set, together with all its sub-prefixes.
\todo{Rewrite in the case of changes to the way prefixes are found}
-\subsubsection{The UnfixesCollector}
+\subsubsection{The UnfixesCollector}\label{unfixes}
The \typewithref{no.uio.ifi.refaktor.extractors.collectors}{UnfixesCollector}
-finds unfixes within the selection. An unfix is a name that is assigned to
-within the selection. The reason that this cannot be allowed, is that the result
-would be an assignment to the \type{this} keyword, which is not valid in Java.
-
-\subsubsection{Computing Safe Prefixes}
-A safe prefix is a prefix that does not enclose an unfix. A prefix is enclosing
-an unfix if the unfix is in the set of its sub-prefixes. As an example,
-\texttt{``a.b''} is enclosing \texttt{``a''}, as is \texttt{``a''}. The safe
-prefixes is unified in a \type{PrefixSet} and can be fetched calling the
-\method{getSafePrefixes} method of the
-\type{ExtractAndMoveMethodPrefixesExtractor}.
+finds unfixes within a selection. That is prefixes that cannot be used as a
+basis for finding a move target in a refactoring.
+
+An unfix can be a name that is assigned to within a selection. The reason that
+this cannot be allowed, is that the result would be an assignment to the
+\type{this} keyword, which is not valid in Java \see{eclipse_bug_420726}.
+
+Prefixes that originates from variable declarations within the same selection
+are also considered unfixes. This is because when a method is moved, it needs to
+be called through a variable. If this variable is also within the method that is
+to be moved, this obviously cannot be done.
+
+Also considered as unfixes are variable references that are of types that is not
+suitable for moving a methods to. This can be either because it is not
+physically possible to move the method to the desired class or that it will
+cause compilation errors by doing so.
+
+If the type binding for a name is not resolved it is considered and unfix. The
+same applies to types that is only found in compiled code, so they have no
+underlying source that is accessible to us. (E.g. the \type{java.lang.String}
+class.)
+
+Interfaces types are not suitable as targets. This is simply because interfaces
+in java cannot contain methods with bodies. (This thesis does not deal with
+features of Java versions later than Java 7. Java 8 has interfaces with default
+implementations of methods.) Neither are local types allowed. This accounts for
+both local and anonymous classes. Anonymous classes are effectively the same as
+interface types with respect to unfixes. Local classes could in theory be used
+as targets, but this is not possible due to limitations of the implementation of
+the Extract and Move Method refactoring. The problem is that the refactoring is
+done in two steps, so the intermediate state between the two refactorings would
+not be legal Java code. In the case of local classes, the problem is that, in
+the intermediate step, a selection referencing a local class would need to take
+the local class as a parameter if it were to be extracted to a new method. This
+new method would need to live in the scope of the declaring class of the
+originating method. The local class would then not be in the scope of the
+extracted method, thus bringing the source code into an illegal state. One could
+imagine that the method was extracted and moved in one operation, without an
+intermediate state. Then it would make sense to include variables with types of
+local classes in the set of legal targets, since the local classes would then be
+in the scopes of the method calls. If this makes any difference for software
+metrics that measure coupling would be a different discussion.
+
+\begin{listing}
+\begin{multicols}{2}
+\begin{minted}[]{java}
+// Before
+void declaresLocalClass() {
+ class LocalClass {
+ void foo() {}
+ void bar() {}
+ }
+
+ LocalClass inst =
+ new LocalClass();
+ inst.foo();
+ inst.bar();
+}
+\end{minted}
+
+\columnbreak
+
+\begin{minted}[]{java}
+// After Extract Method
+void declaresLocalClass() {
+ class LocalClass {
+ void foo() {}
+ void bar() {}
+ }
+
+ LocalClass inst =
+ new LocalClass();
+ fooBar(inst);
+}
+
+// Intermediate step
+void fooBar(LocalClass inst) {
+ inst.foo();
+ inst.bar();
+}
+\end{minted}
+\end{multicols}
+\caption{When Extract and Move Method tries to use a variable with a local type
+as the move target, an intermediate step is taken that is not allowed. Here:
+\type{LocalClass} is not in the scope of \method{fooBar} in its intermediate
+location.}
+\label{lst:extractMethod_LocalClass}
+\end{listing}
+
+The last class of names that are considered unfixes is names used in null tests.
+These are tests that reads like this: if \texttt{<name>} equals \var{null} then
+do something. If allowing variables used in those kinds of expressions as
+targets for moving methods, we would end up with code containing boolean
+expressions like \texttt{this == null}, which would not be meaningful, since
+\var{this} would never be \var{null}.
\subsection{The Prefix Class}
-\todo{?}
+This class exists mainly for holding data about a prefix, such as the expression
+that the prefix represents and the occurrence count of the prefix within a
+selection. In addition to this, it has some functionality such as calculating
+its sub-prefixes and intersecting it with another prefix. The definition of the
+intersection between two prefixes is a prefix representing the longest common
+expression between the two.
+
\subsection{The PrefixSet Class}
+A prefix set holds elements of type \type{Prefix}. It is implemented with the
+help of a \typewithref{java.util}{HashMap} and contains some typical set
+operations, but it does not implement the \typewithref{java.util}{Set}
+interface, since the prefix set does not need all of the functionality a
+\type{Set} requires to be implemented. In addition It needs some other
+functionality not found in the \type{Set} interface. So due to the relatively
+limited use of prefix sets, and that it almost always needs to be referenced as
+such, and not a \type{Set<Prefix>}, it remains as an ad hoc solution to a
+concrete problem.
+
+There are two ways adding prefixes to a \type{PrefixSet}. The first is through
+its \method{add} method. This works like one would expect from a set. It adds
+the prefix to the set if it does not already contain the prefix. The other way
+is to \emph{register} the prefix with the set. When registering a prefix, if the
+set does not contain the prefix, it is just added. If the set contains the
+prefix, its count gets incremented. This is how the occurrence count is handled.
+
+The prefix set also computes the set of prefixes that is not enclosing any
+prefixes of another set. This is kind of a set difference operation only for
+enclosing prefixes.
\subsection{Hacking the Refactoring Undo
History}\label{hacking_undo_history}
-\todo{Where to put this section?}
+\todoin{Where to put this section?}
As an attempt to make multiple subsequent changes to the workspace appear as a
single action (i.e. make the undo changes appear as such), I tried to alter
-\chapter{Eclipse Bugs}
+
+\chapter{Analyzing Code}
+
+\section{AST}
+\todoin{Explain what it is, or just how it is structured in Eclipse and how to
+analyze it?}
+
+\section{Illegal selections}
+
+\subsection{Not all branches end in return}
+
+\subsection{Ambiguous return statement}
+This problem occurs when there is either more than one assignment to a local
+variable that is used outside of the selection, or there is only one, but there
+are also return statements in the selection.
+
+\todoin{Explain why we do not need to consider variables assigned inside
+local/anonymous classes. (The referenced variables need to be final and so
+on\ldots)}
+
+\chapter{Eclipse Bugs Found}
\todoin{Add other things and change headline?}
\section{Eclipse bug 420726: Code is broken when moving a method that is
-assigning to the parameter that is also the move destination}
+assigning to the parameter that is also the move
+destination}\label{eclipse_bug_420726}
This bug\footnote{\url{https://bugs.eclipse.org/bugs/show\_bug.cgi?id=420726}}
was found when analyzing what kinds of names that was to be considered as
-\emph{unfixes}.\todo{refer to unfixes}
+\emph{unfixes} \see{unfixes}.
\subsection{The bug}
The bug emerges when trying to move a method from one class to another, and when
class of the method to move is anonymous, the \var{this} expression in the
parameter list is not qualified with the declaring class' (empty) name.
+\section{Eclipse bug 429954: Extracting statement with reference to local type
+breaks code}\label{eclipse_bug_429954}
+The bug\footnote{\url{https://bugs.eclipse.org/bugs/show\_bug.cgi?id=429954}}
+was discovered when doing some changes to the way unfixes is computed.
+
+\subsection{The bug}
+The problem is that Eclipse is allowing selections that references variables of
+local types to be extracted. When this happens the code is broken, since the
+extracted method must take a parameter of a local type that is not in the
+methods scope. The problem is illustrated in
+\myref{lst:extractMethod_LocalClass}, but there in another setting.
+
+\subsection{Actions taken}
+There are no actions directly springing out of this bug, since the Extract
+Method refactoring cannot be meant to be this way. This is handled on the
+analysis stage of our Extract and Move Method refactoring. So names representing
+variables of local types is considered unfixes \see{unfixes}.
+\todoin{write more when fixing this in legal statements checker}
+
\chapter{Related Work}
\section{The compositional paradigm of refactoring}
git://git.uio.no / ifi-stolz-refaktor.git / blobdiff