You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@groovy.apache.org by pa...@apache.org on 2017/12/24 06:33:28 UTC
[2/2] groovy git commit: GROOVY-8386/GROOVY-8094: Final variable
analysis broken with try/catch/finally and if/then/else (closes #646)
GROOVY-8386/GROOVY-8094: Final variable analysis broken with try/catch/finally and if/then/else (closes #646)
Project: http://git-wip-us.apache.org/repos/asf/groovy/repo
Commit: http://git-wip-us.apache.org/repos/asf/groovy/commit/319deda4
Tree: http://git-wip-us.apache.org/repos/asf/groovy/tree/319deda4
Diff: http://git-wip-us.apache.org/repos/asf/groovy/diff/319deda4
Branch: refs/heads/GROOVY_2_6_X
Commit: 319deda42e333c4921f7ed804252ce9726e200ff
Parents: cbc7e31
Author: paulk <pa...@asert.com.au>
Authored: Fri Dec 22 17:11:51 2017 +1000
Committer: paulk <pa...@asert.com.au>
Committed: Sun Dec 24 16:33:14 2017 +1000
----------------------------------------------------------------------
.../groovy/classgen/FinalVariableAnalyzer.java | 113 ++++++++++++-------
.../groovy/transform/stc/BugsSTCTest.groovy | 16 ---
.../groovy/transform/stc/GenericsSTCTest.groovy | 6 +-
.../classgen/FinalVariableAnalyzerTest.groovy | 53 ++++++---
.../groovy/runtime/NioGroovyMethodsTest.groovy | 2 +-
5 files changed, 113 insertions(+), 77 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/groovy/blob/319deda4/src/main/java/org/codehaus/groovy/classgen/FinalVariableAnalyzer.java
----------------------------------------------------------------------
diff --git a/src/main/java/org/codehaus/groovy/classgen/FinalVariableAnalyzer.java b/src/main/java/org/codehaus/groovy/classgen/FinalVariableAnalyzer.java
index 76c4e1f..141daa6 100644
--- a/src/main/java/org/codehaus/groovy/classgen/FinalVariableAnalyzer.java
+++ b/src/main/java/org/codehaus/groovy/classgen/FinalVariableAnalyzer.java
@@ -52,7 +52,7 @@ public class FinalVariableAnalyzer extends ClassCodeVisitorSupport {
private final SourceUnit sourceUnit;
private final VariableNotFinalCallback callback;
- private Set<VariableExpression> declaredFinalVariables = null;
+ private Set<Variable> declaredFinalVariables = null;
private boolean inAssignment = false;
private enum VariableState {
@@ -89,11 +89,12 @@ public class FinalVariableAnalyzer extends ClassCodeVisitorSupport {
public FinalVariableAnalyzer(final SourceUnit sourceUnit, final VariableNotFinalCallback callback) {
this.callback = callback;
this.sourceUnit = sourceUnit;
- pushState();
+ assignmentTracker.add(new StateMap());
}
private Map<Variable, VariableState> pushState() {
Map<Variable, VariableState> state = new StateMap();
+ state.putAll(getState());
assignmentTracker.add(state);
return state;
}
@@ -128,18 +129,9 @@ public class FinalVariableAnalyzer extends ClassCodeVisitorSupport {
@Override
public void visitBlockStatement(final BlockStatement block) {
- Set<VariableExpression> old = declaredFinalVariables;
- declaredFinalVariables = new HashSet<VariableExpression>();
+ Set<Variable> old = declaredFinalVariables;
+ declaredFinalVariables = new HashSet<Variable>();
super.visitBlockStatement(block);
- if (callback != null) {
- Map<Variable, VariableState> state = getState();
- for (VariableExpression declaredFinalVariable : declaredFinalVariables) {
- VariableState variableState = state.get(declaredFinalVariable.getAccessedVariable());
- if (variableState == null || variableState != VariableState.is_final) {
- callback.variableNotAlwaysInitialized(declaredFinalVariable);
- }
- }
- }
declaredFinalVariables = old;
}
@@ -149,27 +141,45 @@ public class FinalVariableAnalyzer extends ClassCodeVisitorSupport {
boolean isDeclaration = expression instanceof DeclarationExpression;
Expression leftExpression = expression.getLeftExpression();
Expression rightExpression = expression.getRightExpression();
- if (isDeclaration && leftExpression instanceof VariableExpression) {
- VariableExpression var = (VariableExpression) leftExpression;
- if (Modifier.isFinal(var.getModifiers())) {
- declaredFinalVariables.add(var);
- }
+ if (isDeclaration) {
+ recordFinalVars(leftExpression);
}
- leftExpression.visit(this);
+ // visit RHS first for expressions like a = b = 0
inAssignment = assignment;
rightExpression.visit(this);
inAssignment = false;
+ leftExpression.visit(this);
if (assignment) {
- if (leftExpression instanceof Variable) {
- boolean uninitialized =
- isDeclaration && rightExpression == EmptyExpression.INSTANCE;
- recordAssignment((Variable) leftExpression, isDeclaration, uninitialized, false, expression);
- } else if (leftExpression instanceof TupleExpression) {
- TupleExpression te = (TupleExpression) leftExpression;
- for (Expression next : te.getExpressions()) {
- if (next instanceof Variable) {
- recordAssignment((Variable) next, isDeclaration, false, false, next);
- }
+ recordAssignments(expression, isDeclaration, leftExpression, rightExpression);
+ }
+ }
+
+ private void recordAssignments(BinaryExpression expression, boolean isDeclaration, Expression leftExpression, Expression rightExpression) {
+ if (leftExpression instanceof Variable) {
+ boolean uninitialized =
+ isDeclaration && rightExpression == EmptyExpression.INSTANCE;
+ recordAssignment((Variable) leftExpression, isDeclaration, uninitialized, false, expression);
+ } else if (leftExpression instanceof TupleExpression) {
+ TupleExpression te = (TupleExpression) leftExpression;
+ for (Expression next : te.getExpressions()) {
+ if (next instanceof Variable) {
+ recordAssignment((Variable) next, isDeclaration, false, false, next);
+ }
+ }
+ }
+ }
+
+ private void recordFinalVars(Expression leftExpression) {
+ if (leftExpression instanceof VariableExpression) {
+ VariableExpression var = (VariableExpression) leftExpression;
+ if (Modifier.isFinal(var.getModifiers())) {
+ declaredFinalVariables.add(var);
+ }
+ } else if (leftExpression instanceof TupleExpression) {
+ TupleExpression te = (TupleExpression) leftExpression;
+ for (Expression next : te.getExpressions()) {
+ if (next instanceof Variable) {
+ declaredFinalVariables.add((Variable) next);
}
}
}
@@ -214,13 +224,16 @@ public class FinalVariableAnalyzer extends ClassCodeVisitorSupport {
@Override
public void visitVariableExpression(final VariableExpression expression) {
super.visitVariableExpression(expression);
- if (inAssignment) {
- Map<Variable, VariableState> state = getState();
- Variable key = expression.getAccessedVariable();
+ Map<Variable, VariableState> state = getState();
+ Variable key = expression.getAccessedVariable();
+ if (key == null) {
+ fixVar(expression);
+ key = expression.getAccessedVariable();
+ }
+ if (key != null && !key.isClosureSharedVariable() && callback != null) {
VariableState variableState = state.get(key);
- if (variableState == VariableState.is_uninitialized) {
- variableState = VariableState.is_var;
- state.put(key, variableState);
+ if (inAssignment && variableState == VariableState.is_uninitialized) {
+ callback.variableNotAlwaysInitialized(expression);
}
}
}
@@ -257,14 +270,8 @@ public class FinalVariableAnalyzer extends ClassCodeVisitorSupport {
VariableState mergedIfElse;
mergedIfElse = ifValue != null && ifValue.isFinal
&& elseValue != null && elseValue.isFinal ? VariableState.is_final : VariableState.is_var;
- if (beforeValue == null) {
+ if (beforeValue != null) {
curState.put(var, mergedIfElse);
- } else {
- if (beforeValue == VariableState.is_uninitialized) {
- curState.put(var, mergedIfElse);
- } else if (ifValue != null || elseValue != null) {
- curState.put(var, VariableState.is_var);
- }
}
}
}
@@ -310,6 +317,14 @@ public class FinalVariableAnalyzer extends ClassCodeVisitorSupport {
if (var == null) {
return;
}
+
+ // getTarget(var) can be null in buggy xform code, e.g. Spock
+ if (getTarget(var) == null) {
+ fixVar(var);
+ // we maybe can't fix a synthetic field
+ if (getTarget(var) == null) return;
+ }
+
if (!isDeclaration && var.isClosureSharedVariable()) {
getState().put(var, VariableState.is_var);
}
@@ -331,6 +346,20 @@ public class FinalVariableAnalyzer extends ClassCodeVisitorSupport {
}
}
+ // getTarget(var) can be null in buggy xform code, e.g. Spock <= 1.1
+ // TODO consider removing fixVar once Spock 1.2 is released - replace with informational exception?
+ // This fixes xform declaration expressions but not other synthetic fields which aren't set up correctly
+ private void fixVar(Variable var) {
+ if (getTarget(var) == null && var instanceof VariableExpression && getState() != null && var.getName() != null) {
+ for (Variable v: getState().keySet()) {
+ if (var.getName().equals(v.getName())) {
+ ((VariableExpression)var).setAccessedVariable(v);
+ break;
+ }
+ }
+ }
+ }
+
public interface VariableNotFinalCallback {
/**
* Callback called whenever an assignment transforms an effectively final variable into a non final variable
http://git-wip-us.apache.org/repos/asf/groovy/blob/319deda4/src/test/groovy/transform/stc/BugsSTCTest.groovy
----------------------------------------------------------------------
diff --git a/src/test/groovy/transform/stc/BugsSTCTest.groovy b/src/test/groovy/transform/stc/BugsSTCTest.groovy
index 721f11d..3a3df35 100644
--- a/src/test/groovy/transform/stc/BugsSTCTest.groovy
+++ b/src/test/groovy/transform/stc/BugsSTCTest.groovy
@@ -426,22 +426,6 @@ class BugsSTCTest extends StaticTypeCheckingTestCase {
}
'''
}
- void testFlowTypingErrorWithIfElseAndNoInitialInferredType() {
- assertScript '''
- def o
- boolean b = true
- if (b) {
- o = 1
- } else {
- @ASTTest(phase=INSTRUCTION_SELECTION, value={
- assert node.getNodeMetaData(INFERRED_TYPE) == OBJECT_TYPE
- })
- def o2 = o
- o2 = 'foo'
- println (o2.toString())
- }
- '''
- }
// GROOVY-6104
void testShouldResolveConstantFromInterfaceImplementedInSuperClass() {
http://git-wip-us.apache.org/repos/asf/groovy/blob/319deda4/src/test/groovy/transform/stc/GenericsSTCTest.groovy
----------------------------------------------------------------------
diff --git a/src/test/groovy/transform/stc/GenericsSTCTest.groovy b/src/test/groovy/transform/stc/GenericsSTCTest.groovy
index b1d9fd7..a35506c 100644
--- a/src/test/groovy/transform/stc/GenericsSTCTest.groovy
+++ b/src/test/groovy/transform/stc/GenericsSTCTest.groovy
@@ -582,7 +582,7 @@ class GenericsSTCTest extends StaticTypeCheckingTestCase {
// GROOVY-5594
void testMapEntryUsingPropertyNotation() {
assertScript '''
- Map.Entry<Date, Integer> entry
+ Map.Entry<Date, Integer> entry = null
@ASTTest(phase=INSTRUCTION_SELECTION, value={
assert node.getNodeMetaData(INFERRED_TYPE) == make(Date)
@@ -598,7 +598,7 @@ class GenericsSTCTest extends StaticTypeCheckingTestCase {
void testInferenceFromMap() {
assertScript '''
- Map<Date, Integer> map
+ Map<Date, Integer> map = [:]
@ASTTest(phase=INSTRUCTION_SELECTION, value={
def infType = node.getNodeMetaData(INFERRED_TYPE)
@@ -614,7 +614,7 @@ class GenericsSTCTest extends StaticTypeCheckingTestCase {
void testInferenceFromListOfMaps() {
assertScript '''
- List<Map<Date, Integer>> maps
+ List<Map<Date, Integer>> maps = []
@ASTTest(phase=INSTRUCTION_SELECTION, value={
def listType = node.getNodeMetaData(INFERRED_TYPE)
http://git-wip-us.apache.org/repos/asf/groovy/blob/319deda4/src/test/org/codehaus/groovy/classgen/FinalVariableAnalyzerTest.groovy
----------------------------------------------------------------------
diff --git a/src/test/org/codehaus/groovy/classgen/FinalVariableAnalyzerTest.groovy b/src/test/org/codehaus/groovy/classgen/FinalVariableAnalyzerTest.groovy
index bffda28..c5a8cd7 100644
--- a/src/test/org/codehaus/groovy/classgen/FinalVariableAnalyzerTest.groovy
+++ b/src/test/org/codehaus/groovy/classgen/FinalVariableAnalyzerTest.groovy
@@ -215,6 +215,30 @@ class FinalVariableAnalyzerTest extends GroovyTestCase {
'''
}
+ // GROOVY-8386
+ void testPotentiallyUninitializedFinalVarOkayIfNotUsed() {
+ assertFinals begin: false, '''
+ final begin
+ try {
+ begin = new Date()
+ } finally {
+ println 'done'
+ }
+ '''
+ }
+
+ // GROOVY-8094
+ void testFinalVarSetWithinIf() {
+ assertFinalCompilationErrors(['z'], '''
+ def method() {
+ final z = null
+ if (z != null) {
+ z = 3
+ }
+ }
+ ''')
+ }
+
void testPrePostfixShouldMakeVarNotFinal() {
assertFinals x: false, y: false, z: false, o: false, '''
def x = 0
@@ -228,17 +252,15 @@ class FinalVariableAnalyzerTest extends GroovyTestCase {
'''
}
- void testPrePostfixShouldMakeUninitializedVarNotFinal() {
- assertFinals x: false, y: false, z: false, o: false, '''
+ void testPrePostfixShouldNotCompileWithUninitializedVar() {
+ assertFinalCompilationErrors(['x'], '''
def x
- def y
- def z
- def o
x++
- ++y
- z--
- --o
- '''
+ ''', true)
+ assertFinalCompilationErrors(['y'], '''
+ def y
+ --y
+ ''', true)
}
void testAssignmentInIfBooleanExpressionShouldFailCompilation() {
@@ -308,7 +330,7 @@ class FinalVariableAnalyzerTest extends GroovyTestCase {
final x
def y = x
x = 1
- ''')
+ ''', true)
}
void testShouldConsiderThatXIsEffectivelyFinalWithIfElse() {
@@ -335,10 +357,10 @@ class FinalVariableAnalyzerTest extends GroovyTestCase {
'''
}
- void testShouldThrowCompileTimeErrorBecauseXIsNotInitialized() {
- assertFinalCompilationErrors(['x'], '''
+ void testShouldConsiderFinalVarOkayIfNotAccessedButShouldNotBeDeemedFinal() {
+ assertFinals x: false, '''
final x
- ''', true)
+ '''
}
void testShouldThrowCompileTimeErrorBecauseXIsNotEffectivelyFinalWithSubsequentIfs() {
@@ -348,9 +370,10 @@ class FinalVariableAnalyzerTest extends GroovyTestCase {
x=1
}
if (!foo) {
- x=2
+ x=2 // follow Java here, don't try to interpret boolean's across different if statements
+ // so consider this as might have been already initialized at this point
}
- ''', true)
+ ''')
}
void testShouldNotSayThatVarIsNotInitialized() {
http://git-wip-us.apache.org/repos/asf/groovy/blob/319deda4/subprojects/groovy-nio/src/test/groovy/org/codehaus/groovy/runtime/NioGroovyMethodsTest.groovy
----------------------------------------------------------------------
diff --git a/subprojects/groovy-nio/src/test/groovy/org/codehaus/groovy/runtime/NioGroovyMethodsTest.groovy b/subprojects/groovy-nio/src/test/groovy/org/codehaus/groovy/runtime/NioGroovyMethodsTest.groovy
index 01fc88e..e751ed5 100644
--- a/subprojects/groovy-nio/src/test/groovy/org/codehaus/groovy/runtime/NioGroovyMethodsTest.groovy
+++ b/subprojects/groovy-nio/src/test/groovy/org/codehaus/groovy/runtime/NioGroovyMethodsTest.groovy
@@ -192,7 +192,7 @@ class NioGroovyMethodsTest extends Specification {
inputStream.readObject() == str
cleanup:
- inputStream.close()
+ inputStream?.close()
}
def testNewObjectInputStream() {