Skip to content

Commit

Permalink
Fix for #664: No more NPE checking for "Convert local to variable field"
Browse files Browse the repository at this point in the history
  • Loading branch information
@eric-milles
eric-milles committed Aug 3, 2018
1 parent 22c1911 commit 2561fab
Show file tree
Hide file tree
Showing 3 changed files with 140 additions and 37 deletions.
113 changes: 107 additions & 6 deletions .../src/org/codehaus/groovy/eclipse/refactoring/test/extract/ConvertLocalToFieldTests.groovy
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2009-2017 the original author or authors.
* Copyright 2009-2018 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -15,6 +15,8 @@
*/
package org.codehaus.groovy.eclipse.refactoring.test.extract

import groovy.transform.NotYetImplemented

import org.codehaus.groovy.eclipse.refactoring.core.extract.ConvertGroovyLocalToFieldRefactoring
import org.codehaus.groovy.eclipse.refactoring.test.RefactoringTestSuite
import org.codehaus.jdt.groovy.model.GroovyCompilationUnit
Expand All @@ -30,8 +32,11 @@ final class ConvertLocalToFieldTests extends RefactoringTestSuite {
null
}

private void runTest(String testName) {
private void runTest(String testName = test.methodName) {
ConvertLocalToFieldTestsData.TestCase testCase = ConvertLocalToFieldTestsData.getTestCases().get(testName)
}

private void runTest(ConvertLocalToFieldTestsData.TestCase testCase) {
GroovyCompilationUnit cu = (GroovyCompilationUnit) createCU(packageP, 'Test.groovy', testCase.getInput())

ConvertGroovyLocalToFieldRefactoring refactoring =
Expand Down Expand Up @@ -70,6 +75,8 @@ final class ConvertLocalToFieldTests extends RefactoringTestSuite {
assertEqualLines('invalid redo', testCase.getExpected(), cu.getSource())
}

//--------------------------------------------------------------------------

@Test
void testMethodToModule() {
runTest('testMethodToModule')
Expand Down Expand Up @@ -177,21 +184,115 @@ final class ConvertLocalToFieldTests extends RefactoringTestSuite {

@Test
void testClosure() {
runTest('testClosure')
runTest()
}

@Test
void testClosureVariableConflict() {
runTest('testClosureVariableConflict')
runTest()
}

@Test
void testClosureParameterList() {
runTest('testClosureParameterList')
runTest()
}

@Test
void testClosureImplicitIt() {
runTest('testClosureImplicitIt')
runTest()
}

@Test @NotYetImplemented
void testWithinFieldInitializer1() {
def testCase = new ConvertLocalToFieldTestsData.TestCase('''\
class Pogo {
private Object field = { ->
def target/**/ = null
}
}
'''.stripIndent(), '''\
class Pogo {
\tprivate def target
private Object field = { ->
target/**/ = null
}
}
'''.stripIndent())
runTest(testCase)
}

@Test
void testWithinFieldInitializer2() {
def testCase = new ConvertLocalToFieldTestsData.TestCase('''\
@groovy.transform.Field
Object field = { ->
def target/**/ = null
}
'''.stripIndent(), '''\
@groovy.transform.Field def target
@groovy.transform.Field
Object field = { ->
target/**/ = null
}
'''.stripIndent())
runTest(testCase)
}

@Test
void testWithinObjectInitializer() {
def testCase = new ConvertLocalToFieldTestsData.TestCase('''\
class Pogo {
{
def target/**/ = null
}
}
'''.stripIndent(), '''\
class Pogo {
\tprivate def target
{
target/**/ = null
}
}
'''.stripIndent())
runTest(testCase)
}

@Test @NotYetImplemented
void testWithinStaticInitializer() {
def testCase = new ConvertLocalToFieldTestsData.TestCase('''\
class Pogo {
static {
def target/**/ = null
}
}
'''.stripIndent(), '''\
class Pogo {
\tprivate static def target
static {
target/**/ = null
}
}
'''.stripIndent())
runTest(testCase)
}

@Test @NotYetImplemented // not sure if this is legal, but wanted case to test enclosing checks
void testWithinAnnotationClosure() {
createCU(packageP, 'Tag.groovy', '@interface Tag { Class value() }')
def testCase = new ConvertLocalToFieldTestsData.TestCase('''\
@Tag(value = { ->
def target/**/ = null
})
class Pogo {
}
'''.stripIndent(), '''\
@Tag(value = { ->
Pogo.target/**/ = null
})
class Pogo {
\tprivate static def target
}
'''.stripIndent())
runTest(testCase)
}
}
8 changes: 4 additions & 4 deletions .../org/codehaus/groovy/eclipse/refactoring/test/extract/ConvertLocalToFieldTestsData.groovy
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2009-2016 the original author or authors.
* Copyright 2009-2018 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -23,15 +23,15 @@ class ConvertLocalToFieldTestsData {
String fieldName
boolean expectWarning

public TestCase(input, expected = null, fieldName = "target", expectWarning = false) {
public TestCase(String input, String expected = null, String fieldName = "target", boolean expectWarning = false) {
this.input = input
this.expected = expected
this.fieldName = fieldName
this.expectWarning = expectWarning
}

int getSelectionOffset() { return input.indexOf("target/**/") }
int getSelectionLength() { return "target".length() }
int getSelectionOffset() { input.indexOf('target/**/') }
int getSelectionLength() { 'target'.length() }
}

static Map<String, TestCase> testCases = [
Expand Down
56 changes: 29 additions & 27 deletions ...odehaus/groovy/eclipse/refactoring/core/extract/ConvertGroovyLocalToFieldRefactoring.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2009-2017 the original author or authors.
* Copyright 2009-2018 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -50,6 +50,7 @@
import org.codehaus.jdt.groovy.model.GroovyCompilationUnit;
import org.eclipse.core.runtime.CoreException;
import org.eclipse.core.runtime.IProgressMonitor;
import org.eclipse.core.runtime.OperationCanceledException;
import org.eclipse.jdt.core.compiler.CharOperation;
import org.eclipse.jdt.core.refactoring.CompilationUnitChange;
import org.eclipse.jdt.groovy.core.util.DepthFirstVisitor;
Expand Down Expand Up @@ -90,7 +91,6 @@ public class ConvertGroovyLocalToFieldRefactoring extends PromoteTempToFieldRefa
private DeclarationExpression declarationExpression;
private ClassNode containingClassNode;
private ModuleNode moduleNode;

private MethodNode methodNode;

private CompilationUnitChange change;
Expand Down Expand Up @@ -247,12 +247,12 @@ public RefactoringStatus checkInitialConditions(IProgressMonitor monitor) throws

this.variableExpressionInDeclaration = variableExpressionInDeclaration;

ClassNode containingClassNode = getContainingClassNode();
if (containingClassNode == null) {
ClassNode containingClass = getContainingClassNode();
if (containingClass == null) {
result.merge(RefactoringStatus.createFatalErrorStatus("Cannot find enclosing class declaration."));
return result;
}
if (containingClassNode.isInterface() || containingClassNode.isAnnotationDefinition()) {
if (containingClass.isInterface() || containingClass.isAnnotationDefinition()) {
result.merge(RefactoringStatus.createFatalErrorStatus("Cannot add field to an interface or annotation definition."));
return result;
}
Expand Down Expand Up @@ -311,32 +311,36 @@ public RefactoringStatus checkFinalConditions(IProgressMonitor monitor) throws C
this.change = change;

return status;
} catch (BadLocationException e) {
throw new OperationCanceledException(e.getMessage());
} finally {
monitor.done();
}
}

private TextEditGroup createFieldTextEditGroup() {
private TextEditGroup createFieldTextEditGroup() throws BadLocationException {
TextEdit textEdit = null;

ClassNode classNode = getContainingClassNode();
MethodNode methodNode = getContainingMethodNode();
if (methodNode.isScriptBody() && getContainingClosureExpression() == null) {
ClassNode containingClass = getContainingClassNode();
MethodNode containingMethod = getContainingMethodNode();
if (containingMethod != null && containingMethod.isScriptBody() && getContainingClosureExpression() == null) {
textEdit = new InsertEdit(getDeclarationOffset(), "@groovy.transform.Field ");
} else if (containingClass != null && containingClass.isScript()) {
textEdit = new InsertEdit(containingClass.getStart(), createFieldText(0) + ASTTools.getLineDelimeter(unit));
} else {
try {
char[] contents = unit.getContents();
int methodOffset = methodNode.getStart();
int methodLineOffset = methodNode.getStart() - methodNode.getColumnNumber() + 1;
int insertOffset = classNode.isScript() ? classNode.getStart() : CharOperation.indexOf('{', contents, classNode.getStart()) + 1;
char[] contents = unit.getContents();
int indentationLevel = 1;
if (containingMethod != null && containingMethod.getEnd() > 0) {
int methodLineOffset = containingMethod.getStart() - containingMethod.getColumnNumber() + 1;
String methodIndentation = String.valueOf(CharOperation.subarray(contents, methodLineOffset, containingMethod.getStart()));
indentationLevel = ASTTools.getCurrentIntentation(methodIndentation);
}

String methodIndentation = String.valueOf(CharOperation.subarray(contents, methodLineOffset, methodOffset));
String fieldText = createFieldText(ASTTools.getCurrentIntentation(methodIndentation));
String newline = ASTTools.getLineDelimeter(unit);
int insertOffset = CharOperation.indexOf('{', contents, containingClass.getStart()) + 1;
String fieldText = createFieldText(indentationLevel);
String newline = ASTTools.getLineDelimeter(unit);

textEdit = new InsertEdit(insertOffset, classNode.isScript() ? fieldText + newline : newline + fieldText);
} catch (Exception e) {
}
textEdit = new InsertEdit(insertOffset, newline + fieldText);
}

TextEditGroup group = new TextEditGroup("Create field.");
Expand All @@ -356,7 +360,7 @@ private String createFieldText(int indentLevel) throws BadLocationException, Mal
sb.append("@groovy.transform.Field ");
} else {
String visibility = JdtFlags.getVisibilityString(getVisibility());
if (!visibility.equals("")) {
if (!"".equals(visibility)) {
sb.append(visibility).append(' ');
}
}
Expand All @@ -376,7 +380,7 @@ private String createFieldText(int indentLevel) throws BadLocationException, Mal

private TextEditGroup declarationToReferenceTextEditGroup() {
TextEditGroup group = new TextEditGroup("Convert local variable declaration to reference.");
if (!getContainingMethodNode().isScriptBody() || getContainingClosureExpression() != null) {
if ((getContainingMethodNode() != null && !getContainingMethodNode().isScriptBody()) || getContainingClosureExpression() != null) {
int typeOrDefLength = variableExpressionInDeclaration.getStart() - getDeclarationOffset();
group.addTextEdit(new ReplaceEdit(getDeclarationOffset(), typeOrDefLength, ""));
}
Expand All @@ -385,7 +389,7 @@ private TextEditGroup declarationToReferenceTextEditGroup() {

private TextEditGroup renameVariableReferencesTextEditGroup(RefactoringStatus status) {
TextEditGroup group = new TextEditGroup("Update local variables to reference field.");
if (!getContainingMethodNode().isScriptBody() || getContainingClosureExpression() != null) {
if ((getContainingMethodNode() != null && !getContainingMethodNode().isScriptBody() || getContainingClosureExpression() != null)) {
final Set<VariableExpression> references = new HashSet<>();
GroovyClassVisitor referencesVisitor = new DepthFirstVisitor() {
@Override
Expand Down Expand Up @@ -435,14 +439,13 @@ private ModuleNode getModuleNode() {

private ClassNode getContainingClassNode() {
if (containingClassNode == null) {
ModuleNode moduleNode = getModuleNode();
if (moduleNode == null) {
if (getModuleNode() == null) {
return null;
}
if (declarationExpression == null) {
return null;
}
containingClassNode = ASTTools.getContainingClassNode(moduleNode, variableExpressionInDeclaration.getStart());
containingClassNode = ASTTools.getContainingClassNode(getModuleNode(), variableExpressionInDeclaration.getStart());
}
return containingClassNode;
}
Expand Down Expand Up @@ -574,7 +577,6 @@ public void visitVariableExpression(VariableExpression expr) {
try {
visitor.visitModule(getModuleNode());
} catch (VisitCompleteException expected) {
;
}
}
}
Expand Down

0 comments on commit 2561fab

Please sign in to comment.