Skip to content

Commit 7d5507f

Browse files
nedtwiggnedtwigg
authored
Reproduce bugs related to equo formatters (#1660)
2 parents a9317da + c19d3c0 commit 7d5507f

File tree

11 files changed

+214
-10
lines changed

11 files changed

+214
-10
lines changed

‎lib-extra/src/groovy/java/com/diffplug/spotless/extra/glue/groovy/GrEclipseFormatterStepImpl.java

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -104,14 +104,14 @@ public String format(String raw) throws Exception {
104104
* Eclipse Groovy formatter does not signal problems by its return value, but by logging errors.
105105
*/
106106
private static final class GroovyErrorListener implements ILogListener, IGroovyLogger {
107-
private final List<String> errors;
107+
private final List<Throwable> errors;
108108

109109
public GroovyErrorListener() {
110110
/*
111111
* We need a synchronized list here, in case multiple instantiations
112112
* run in parallel.
113113
*/
114-
errors = Collections.synchronizedList(new ArrayList<String>());
114+
errors = Collections.synchronizedList(new ArrayList<>());
115115
ILog groovyLogger = GroovyCoreActivator.getDefault().getLog();
116116
groovyLogger.addLogListener(this);
117117
synchronized (GroovyLogManager.manager) {
@@ -121,7 +121,7 @@ public GroovyErrorListener() {
121121

122122
@Override
123123
public void logging(final IStatus status, final String plugin) {
124-
errors.add(status.getMessage());
124+
errors.add(status.getException());
125125
}
126126

127127
public boolean errorsDetected() {
@@ -141,9 +141,10 @@ public String toString() {
141141
} else if (0 == errors.size()) {
142142
string.append("Step sucesfully executed.");
143143
}
144-
for (String error : errors) {
144+
for (Throwable error : errors) {
145+
error.printStackTrace();
145146
string.append(System.lineSeparator());
146-
string.append(error);
147+
string.append(error.getMessage());
147148
}
148149

149150
return string.toString();
@@ -162,7 +163,11 @@ public boolean isCategoryEnabled(TraceCategory cat) {
162163

163164
@Override
164165
public void log(TraceCategory arg0, String arg1) {
165-
errors.add(arg1);
166+
try {
167+
throw new RuntimeException(arg1);
168+
} catch (RuntimeException e) {
169+
errors.add(e);
170+
}
166171
}
167172
}
168173

‎lib-extra/src/main/java/com/diffplug/spotless/extra/EquoBasedStepBuilder.java

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,8 @@
2222
import java.util.Map;
2323
import java.util.Properties;
2424

25+
import javax.annotation.Nullable;
26+
2527
import com.diffplug.spotless.FileSignature;
2628
import com.diffplug.spotless.FormatterFunc;
2729
import com.diffplug.spotless.FormatterProperties;
@@ -46,10 +48,17 @@ public abstract class EquoBasedStepBuilder {
4648
private Iterable<File> settingsFiles = new ArrayList<>();
4749
private Map<String, String> p2Mirrors = Map.of();
4850

49-
/** Initialize valid default configuration, taking latest version */
51+
/** @deprecated if you use this constructor you *must* call {@link #setVersion(String)} before calling {@link #build()} */
52+
@Deprecated
5053
public EquoBasedStepBuilder(String formatterName, Provisioner mavenProvisioner, ThrowingEx.Function<State, FormatterFunc> stateToFormatter) {
54+
this(formatterName, mavenProvisioner, null, stateToFormatter);
55+
}
56+
57+
/** Initialize valid default configuration, taking latest version */
58+
public EquoBasedStepBuilder(String formatterName, Provisioner mavenProvisioner, @Nullable String defaultVersion, ThrowingEx.Function<State, FormatterFunc> stateToFormatter) {
5159
this.formatterName = formatterName;
5260
this.mavenProvisioner = mavenProvisioner;
61+
this.formatterVersion = defaultVersion;
5362
this.stateToFormatter = stateToFormatter;
5463
}
5564

‎lib-extra/src/main/java/com/diffplug/spotless/extra/cpp/EclipseCdtFormatterStep.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ public static String defaultVersion() {
4545

4646
/** Provides default configuration */
4747
public static EquoBasedStepBuilder createBuilder(Provisioner provisioner) {
48-
return new EquoBasedStepBuilder(NAME, provisioner, EclipseCdtFormatterStep::apply) {
48+
return new EquoBasedStepBuilder(NAME, provisioner, defaultVersion(), EclipseCdtFormatterStep::apply) {
4949
@Override
5050
protected P2Model model(String version) {
5151
var model = new P2Model();

‎lib-extra/src/main/java/com/diffplug/spotless/extra/groovy/GrEclipseFormatterStep.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ public static String defaultVersion() {
3939
}
4040

4141
public static EquoBasedStepBuilder createBuilder(Provisioner provisioner) {
42-
return new EquoBasedStepBuilder(NAME, provisioner, GrEclipseFormatterStep::apply) {
42+
return new EquoBasedStepBuilder(NAME, provisioner, defaultVersion(), GrEclipseFormatterStep::apply) {
4343
@Override
4444
protected P2Model model(String version) {
4545
if (!version.startsWith("4.")) {

‎lib-extra/src/main/java/com/diffplug/spotless/extra/java/EclipseJdtFormatterStep.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ public static String defaultVersion() {
3838
}
3939

4040
public static EquoBasedStepBuilder createBuilder(Provisioner provisioner) {
41-
return new EquoBasedStepBuilder(NAME, provisioner, EclipseJdtFormatterStep::apply) {
41+
return new EquoBasedStepBuilder(NAME, provisioner, defaultVersion(), EclipseJdtFormatterStep::apply) {
4242
@Override
4343
protected P2Model model(String version) {
4444
var model = new P2Model();

‎lib-extra/src/test/java/com/diffplug/spotless/extra/groovy/GrEclipseFormatterStepSpecialCaseTest.java

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
/*
2+
* Copyright 2023 DiffPlug
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
package com.diffplug.spotless.extra.groovy;
17+
18+
import org.junit.jupiter.api.Assertions;
19+
import org.junit.jupiter.api.Test;
20+
21+
import com.diffplug.spotless.StepHarness;
22+
import com.diffplug.spotless.TestProvisioner;
23+
24+
public class GrEclipseFormatterStepSpecialCaseTest {
25+
/**
26+
* https://github.com/diffplug/spotless/issues/1657
27+
*
28+
* broken: ${parm == null ? "" : "<tag>$parm</tag>"}
29+
* works: ${parm == null ? "" : "<tag>parm</tag>"}
30+
*/
31+
@Test
32+
public void issue_1657() {
33+
Assertions.assertThrows(IllegalArgumentException.class, () -> {
34+
StepHarness.forStep(GrEclipseFormatterStep.createBuilder(TestProvisioner.mavenCentral()).build())
35+
.testResourceUnaffected("groovy/greclipse/format/SomeClass.test");
36+
});
37+
}
38+
39+
@Test
40+
public void issue_1657_fixed() {
41+
StepHarness.forStep(GrEclipseFormatterStep.createBuilder(TestProvisioner.mavenCentral()).build())
42+
.testResourceUnaffected("groovy/greclipse/format/SomeClass.fixed");
43+
}
44+
}

‎lib-extra/src/test/java/com/diffplug/spotless/extra/java/EclipseJdtFormatterStepSpecialCaseTest.java

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
/*
2+
* Copyright 2016-2023 DiffPlug
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
package com.diffplug.spotless.extra.java;
17+
18+
import org.junit.jupiter.api.Test;
19+
20+
import com.diffplug.spotless.StepHarness;
21+
import com.diffplug.spotless.TestProvisioner;
22+
23+
public class EclipseJdtFormatterStepSpecialCaseTest {
24+
/** https://github.com/diffplug/spotless/issues/1638 */
25+
@Test
26+
public void issue_1638() {
27+
StepHarness.forStep(EclipseJdtFormatterStep.createBuilder(TestProvisioner.mavenCentral()).build())
28+
.testResource("java/eclipse/AbstractType.test", "java/eclipse/AbstractType.clean");
29+
}
30+
}

‎testlib/src/main/resources/groovy/greclipse/format/SomeClass.fixed

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
package com.somepackage
2+
3+
class SomeClass {
4+
5+
def func(parm) {
6+
"""<tag>
7+
${parm == null ? "" : "<tag>parm</tag>"}
8+
${parm == null ? "" : "<tag>parm</tag>"}
9+
</tag>
10+
"""
11+
}
12+
}

‎testlib/src/main/resources/groovy/greclipse/format/SomeClass.test

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
package com.somepackage
2+
3+
class SomeClass {
4+
5+
def func(parm) {
6+
"""<tag>
7+
${parm == null ? "" : "<tag>$parm</tag>"}
8+
${parm == null ? "" : "<tag>$parm</tag>"}
9+
</tag>
10+
"""
11+
}
12+
}

‎testlib/src/main/resources/java/eclipse/AbstractType.clean

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
package test;
2+
3+
public abstract class AbstractType {
4+
5+
private String _typeName;
6+
7+
AbstractType(String typeName) {
8+
_typeName = typeName;
9+
}
10+
11+
private String _type() {
12+
String name = getClass().getSimpleName();
13+
return name.endsWith("Type") ? name.substring(0, getClass().getSimpleName().length() - 4) : name;
14+
}
15+
16+
AbstractType argument() {
17+
throw new UnsupportedOperationException(getClass().getSimpleName());
18+
}
19+
20+
@Override
21+
public boolean equals(Object another) {
22+
if (this == another) {
23+
return true;
24+
}
25+
return another instanceof AbstractType t && _typeName.equals(t._typeName);
26+
}
27+
28+
@Override
29+
public int hashCode() {
30+
return _typeName.hashCode();
31+
}
32+
33+
@Override
34+
public String toString() {
35+
return getClass().getSimpleName() + "(typeName)";
36+
}
37+
38+
}

‎testlib/src/main/resources/java/eclipse/AbstractType.test

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
package test;
2+
3+
4+
5+
6+
public abstract class AbstractType {
7+
8+
9+
private String _typeName;
10+
11+
AbstractType(String typeName) {
12+
_typeName = typeName;
13+
}
14+
15+
16+
17+
private String _type() {
18+
String name = getClass().getSimpleName();
19+
return name.endsWith("Type")
20+
? name.substring(0, getClass().getSimpleName().length() - 4)
21+
: name;
22+
}
23+
24+
AbstractType argument() {
25+
throw new UnsupportedOperationException(getClass().getSimpleName());
26+
}
27+
28+
29+
@Override
30+
public boolean equals(Object another) {
31+
if (this == another) {
32+
return true;
33+
}
34+
return another instanceof AbstractType t
35+
&& _typeName.equals(t._typeName);
36+
}
37+
38+
39+
40+
@Override
41+
public int hashCode() {
42+
return _typeName.hashCode();
43+
}
44+
45+
46+
47+
48+
@Override
49+
public String toString() {
50+
return getClass().getSimpleName() + "(typeName)";
51+
}
52+
53+
54+
}

0 commit comments

Comments
 (0)