[GROOVY-4304] OptimizerVisitor may run twice, corrupting constants

• Type: Bug
• Status: Closed
• Priority: Critical
• Resolution: Fixed
• Affects Version/s: 1.7.3
• Fix Version/s: 1.7.4, 1.8-beta-1
• Component/s: Compiler
• Assignee: Unassigned
• Reporter: Peter Niederwieser
• Created: 14/Jul/10 10:15
• Updated: 22/Jul/10 17:32
• Resolved: 22/Jul/10 07:32

Description

The issue came up when Paul King showed me a Spock spec behaving in strange ways:

TestSpock.groovy:

@Grab('org.spockframework:spock-core:0.4-groovy-1.7')
class TestSpock extends spock.lang.Specification {
  def convert = new Converter()

  def 'important scenarios'() {
    expect:
    c == convert.toCelsius(f)

    where:
    c   | f   | scenario
    0   | 32  | 'Freezing'
    20  | 68  | 'Garden party conditions'
    35  | 95  | 'Beach conditions'
    100 | 212 | 'Boiling'
  }
}

Converter.groovy:

class Converter {
  def toCelsius (fahrenheit) { (fahrenheit - 32) * 5 / 9 }
}

If you run this with 'groovy TestSpock.groovy', you will notice that the values for 'c' and 'f' get mixed up. After debugging groovyc, I came to the following conclusion:

Compilation of SpockTest advances until end of phase "semantic analysis", then CompilationUnit.dequeued() sets the phase back to "initialization". On the second pass, SourceUnitOperation's that have already run are skipped due to this check in CompilationUnit.applyToSourceUnits():

if ((source.phase < phase) || (source.phase == phase &&
!source.phaseComplete)) ...

However, Compilation.applyToPrimaryClassNodes() has a slightly different check:

if (context == null || context.phase <= phase) ...

source.phaseComplete is not checked here, hence OptimizerVisitor (among others) runs a second time. This leads to wrong results, at least in the case where an AST transform has added additional constants. The TODO in OptimizerVisitor.setConstField() is on the spot.

Adding the source.phaseComplete check to Compilation.applyToPrimaryClassNodes() seemed to solve the problem for Paul. I haven't investigated further if this is the correct approach.