From 64b8183793497522a58d07ee49ac578f930a5c04 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Wed, 5 Apr 2023 15:06:52 -0700 Subject: [PATCH 1/6] The equo-based steps ought to be initialized with a version. --- .../diffplug/spotless/extra/EquoBasedStepBuilder.java | 9 ++++++++- .../spotless/extra/cpp/EclipseCdtFormatterStep.java | 2 +- .../spotless/extra/groovy/GrEclipseFormatterStep.java | 2 +- .../spotless/extra/java/EclipseJdtFormatterStep.java | 2 +- 4 files changed, 11 insertions(+), 4 deletions(-) diff --git a/lib-extra/src/main/java/com/diffplug/spotless/extra/EquoBasedStepBuilder.java b/lib-extra/src/main/java/com/diffplug/spotless/extra/EquoBasedStepBuilder.java index cc12a938ad..66993e2f6c 100644 --- a/lib-extra/src/main/java/com/diffplug/spotless/extra/EquoBasedStepBuilder.java +++ b/lib-extra/src/main/java/com/diffplug/spotless/extra/EquoBasedStepBuilder.java @@ -46,10 +46,17 @@ public abstract class EquoBasedStepBuilder { private Iterable settingsFiles = new ArrayList<>(); private Map p2Mirrors = Map.of(); - /** Initialize valid default configuration, taking latest version */ + /** @deprecated if you use this constructor you *must* call {@link #setVersion(String)} before calling {@link #build()} */ + @Deprecated public EquoBasedStepBuilder(String formatterName, Provisioner mavenProvisioner, ThrowingEx.Function stateToFormatter) { + this(formatterName, mavenProvisioner, null, stateToFormatter); + } + + /** Initialize valid default configuration, taking latest version */ + public EquoBasedStepBuilder(String formatterName, Provisioner mavenProvisioner, String defaultVersion, ThrowingEx.Function stateToFormatter) { this.formatterName = formatterName; this.mavenProvisioner = mavenProvisioner; + this.formatterVersion = defaultVersion; this.stateToFormatter = stateToFormatter; } diff --git a/lib-extra/src/main/java/com/diffplug/spotless/extra/cpp/EclipseCdtFormatterStep.java b/lib-extra/src/main/java/com/diffplug/spotless/extra/cpp/EclipseCdtFormatterStep.java index 1a83389eaa..9f9d591c3c 100644 --- a/lib-extra/src/main/java/com/diffplug/spotless/extra/cpp/EclipseCdtFormatterStep.java +++ b/lib-extra/src/main/java/com/diffplug/spotless/extra/cpp/EclipseCdtFormatterStep.java @@ -45,7 +45,7 @@ public static String defaultVersion() { /** Provides default configuration */ public static EquoBasedStepBuilder createBuilder(Provisioner provisioner) { - return new EquoBasedStepBuilder(NAME, provisioner, EclipseCdtFormatterStep::apply) { + return new EquoBasedStepBuilder(NAME, provisioner, defaultVersion(), EclipseCdtFormatterStep::apply) { @Override protected P2Model model(String version) { var model = new P2Model(); diff --git a/lib-extra/src/main/java/com/diffplug/spotless/extra/groovy/GrEclipseFormatterStep.java b/lib-extra/src/main/java/com/diffplug/spotless/extra/groovy/GrEclipseFormatterStep.java index 08c28889f9..fd3c8b7d8b 100644 --- a/lib-extra/src/main/java/com/diffplug/spotless/extra/groovy/GrEclipseFormatterStep.java +++ b/lib-extra/src/main/java/com/diffplug/spotless/extra/groovy/GrEclipseFormatterStep.java @@ -39,7 +39,7 @@ public static String defaultVersion() { } public static EquoBasedStepBuilder createBuilder(Provisioner provisioner) { - return new EquoBasedStepBuilder(NAME, provisioner, GrEclipseFormatterStep::apply) { + return new EquoBasedStepBuilder(NAME, provisioner, defaultVersion(), GrEclipseFormatterStep::apply) { @Override protected P2Model model(String version) { if (!version.startsWith("4.")) { diff --git a/lib-extra/src/main/java/com/diffplug/spotless/extra/java/EclipseJdtFormatterStep.java b/lib-extra/src/main/java/com/diffplug/spotless/extra/java/EclipseJdtFormatterStep.java index cd9314406f..4a90a40832 100644 --- a/lib-extra/src/main/java/com/diffplug/spotless/extra/java/EclipseJdtFormatterStep.java +++ b/lib-extra/src/main/java/com/diffplug/spotless/extra/java/EclipseJdtFormatterStep.java @@ -38,7 +38,7 @@ public static String defaultVersion() { } public static EquoBasedStepBuilder createBuilder(Provisioner provisioner) { - return new EquoBasedStepBuilder(NAME, provisioner, EclipseJdtFormatterStep::apply) { + return new EquoBasedStepBuilder(NAME, provisioner, defaultVersion(), EclipseJdtFormatterStep::apply) { @Override protected P2Model model(String version) { var model = new P2Model(); From 97b5cc4031eb59134569390ae46644b0bd42820e Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Wed, 5 Apr 2023 16:15:16 -0700 Subject: [PATCH 2/6] Failed attempt to reproduce #1638 (I think we need spotless-format23.xml) --- ...clipseJdtFormatterStepSpecialCaseTest.java | 30 +++++++++++ .../resources/java/eclipse/AbstractType.clean | 38 +++++++++++++ .../resources/java/eclipse/AbstractType.test | 54 +++++++++++++++++++ 3 files changed, 122 insertions(+) create mode 100644 lib-extra/src/test/java/com/diffplug/spotless/extra/java/EclipseJdtFormatterStepSpecialCaseTest.java create mode 100644 testlib/src/main/resources/java/eclipse/AbstractType.clean create mode 100644 testlib/src/main/resources/java/eclipse/AbstractType.test diff --git a/lib-extra/src/test/java/com/diffplug/spotless/extra/java/EclipseJdtFormatterStepSpecialCaseTest.java b/lib-extra/src/test/java/com/diffplug/spotless/extra/java/EclipseJdtFormatterStepSpecialCaseTest.java new file mode 100644 index 0000000000..eb93cf1f09 --- /dev/null +++ b/lib-extra/src/test/java/com/diffplug/spotless/extra/java/EclipseJdtFormatterStepSpecialCaseTest.java @@ -0,0 +1,30 @@ +/* + * Copyright 2016-2023 DiffPlug + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.diffplug.spotless.extra.java; + +import org.junit.jupiter.api.Test; + +import com.diffplug.spotless.StepHarness; +import com.diffplug.spotless.TestProvisioner; + +public class EclipseJdtFormatterStepSpecialCaseTest { + /** https://github.com/diffplug/spotless/issues/1638 */ + @Test + public void issue_1638() { + StepHarness.forStep(EclipseJdtFormatterStep.createBuilder(TestProvisioner.mavenCentral()).build()) + .testResource("java/eclipse/AbstractType.test", "java/eclipse/AbstractType.clean"); + } +} diff --git a/testlib/src/main/resources/java/eclipse/AbstractType.clean b/testlib/src/main/resources/java/eclipse/AbstractType.clean new file mode 100644 index 0000000000..475b1186fd --- /dev/null +++ b/testlib/src/main/resources/java/eclipse/AbstractType.clean @@ -0,0 +1,38 @@ +package test; + +public abstract class AbstractType { + + private String _typeName; + + AbstractType(String typeName) { + _typeName = typeName; + } + + private String _type() { + String name = getClass().getSimpleName(); + return name.endsWith("Type") ? name.substring(0, getClass().getSimpleName().length() - 4) : name; + } + + AbstractType argument() { + throw new UnsupportedOperationException(getClass().getSimpleName()); + } + + @Override + public boolean equals(Object another) { + if (this == another) { + return true; + } + return another instanceof AbstractType t && _typeName.equals(t._typeName); + } + + @Override + public int hashCode() { + return _typeName.hashCode(); + } + + @Override + public String toString() { + return getClass().getSimpleName() + "(typeName)"; + } + +} diff --git a/testlib/src/main/resources/java/eclipse/AbstractType.test b/testlib/src/main/resources/java/eclipse/AbstractType.test new file mode 100644 index 0000000000..d4753b2cfc --- /dev/null +++ b/testlib/src/main/resources/java/eclipse/AbstractType.test @@ -0,0 +1,54 @@ +package test; + + + + +public abstract class AbstractType { + + + private String _typeName; + + AbstractType(String typeName) { + _typeName = typeName; + } + + + + private String _type() { + String name = getClass().getSimpleName(); + return name.endsWith("Type") + ? name.substring(0, getClass().getSimpleName().length() - 4) + : name; + } + + AbstractType argument() { + throw new UnsupportedOperationException(getClass().getSimpleName()); + } + + + @Override + public boolean equals(Object another) { + if (this == another) { + return true; + } + return another instanceof AbstractType t + && _typeName.equals(t._typeName); + } + + + + @Override + public int hashCode() { + return _typeName.hashCode(); + } + + + + + @Override + public String toString() { + return getClass().getSimpleName() + "(typeName)"; + } + + +} From fea35e65b6c49fccdbcb917f12688ec476b8430a Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Wed, 5 Apr 2023 16:52:11 -0700 Subject: [PATCH 3/6] Add a testcase to reproduce #1657. --- ...GrEclipseFormatterStepSpecialCaseTest.java | 30 +++++++++++++++++++ .../groovy/greclipse/format/SomeClass.test | 12 ++++++++ 2 files changed, 42 insertions(+) create mode 100644 lib-extra/src/test/java/com/diffplug/spotless/extra/groovy/GrEclipseFormatterStepSpecialCaseTest.java create mode 100644 testlib/src/main/resources/groovy/greclipse/format/SomeClass.test diff --git a/lib-extra/src/test/java/com/diffplug/spotless/extra/groovy/GrEclipseFormatterStepSpecialCaseTest.java b/lib-extra/src/test/java/com/diffplug/spotless/extra/groovy/GrEclipseFormatterStepSpecialCaseTest.java new file mode 100644 index 0000000000..768e939dfb --- /dev/null +++ b/lib-extra/src/test/java/com/diffplug/spotless/extra/groovy/GrEclipseFormatterStepSpecialCaseTest.java @@ -0,0 +1,30 @@ +/* + * Copyright 2023 DiffPlug + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.diffplug.spotless.extra.groovy; + +import org.junit.jupiter.api.Test; + +import com.diffplug.spotless.StepHarness; +import com.diffplug.spotless.TestProvisioner; + +public class GrEclipseFormatterStepSpecialCaseTest { + /** https://github.com/diffplug/spotless/issues/1657 */ + @Test + public void issue_1657() { + StepHarness.forStep(GrEclipseFormatterStep.createBuilder(TestProvisioner.mavenCentral()).build()) + .testResourceUnaffected("groovy/greclipse/format/SomeClass.test"); + } +} diff --git a/testlib/src/main/resources/groovy/greclipse/format/SomeClass.test b/testlib/src/main/resources/groovy/greclipse/format/SomeClass.test new file mode 100644 index 0000000000..09ea835cca --- /dev/null +++ b/testlib/src/main/resources/groovy/greclipse/format/SomeClass.test @@ -0,0 +1,12 @@ +package com.somepackage + +class SomeClass { + + def func(parm) { + """ + ${parm == null ? "" : "$parm"} + ${parm == null ? "" : "$parm"} + + """ + } +} From 4a9e26984368003a4e50b79684d2e4028d0f58be Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Wed, 5 Apr 2023 16:57:39 -0700 Subject: [PATCH 4/6] Improve error reporting within the GrEclipse step. --- .../glue/groovy/GrEclipseFormatterStepImpl.java | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/lib-extra/src/groovy/java/com/diffplug/spotless/extra/glue/groovy/GrEclipseFormatterStepImpl.java b/lib-extra/src/groovy/java/com/diffplug/spotless/extra/glue/groovy/GrEclipseFormatterStepImpl.java index 9549847d74..7c29209157 100644 --- a/lib-extra/src/groovy/java/com/diffplug/spotless/extra/glue/groovy/GrEclipseFormatterStepImpl.java +++ b/lib-extra/src/groovy/java/com/diffplug/spotless/extra/glue/groovy/GrEclipseFormatterStepImpl.java @@ -104,14 +104,14 @@ public String format(String raw) throws Exception { * Eclipse Groovy formatter does not signal problems by its return value, but by logging errors. */ private static final class GroovyErrorListener implements ILogListener, IGroovyLogger { - private final List errors; + private final List errors; public GroovyErrorListener() { /* * We need a synchronized list here, in case multiple instantiations * run in parallel. */ - errors = Collections.synchronizedList(new ArrayList()); + errors = Collections.synchronizedList(new ArrayList<>()); ILog groovyLogger = GroovyCoreActivator.getDefault().getLog(); groovyLogger.addLogListener(this); synchronized (GroovyLogManager.manager) { @@ -121,7 +121,7 @@ public GroovyErrorListener() { @Override public void logging(final IStatus status, final String plugin) { - errors.add(status.getMessage()); + errors.add(status.getException()); } public boolean errorsDetected() { @@ -141,9 +141,10 @@ public String toString() { } else if (0 == errors.size()) { string.append("Step sucesfully executed."); } - for (String error : errors) { + for (Throwable error : errors) { + error.printStackTrace(); string.append(System.lineSeparator()); - string.append(error); + string.append(error.getMessage()); } return string.toString(); @@ -162,7 +163,11 @@ public boolean isCategoryEnabled(TraceCategory cat) { @Override public void log(TraceCategory arg0, String arg1) { - errors.add(arg1); + try { + throw new RuntimeException(arg1); + } catch (RuntimeException e) { + errors.add(e); + } } } From a8d5db8d3921ff3b9f8863c7f1d48e0c3907ccfc Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Wed, 5 Apr 2023 16:57:54 -0700 Subject: [PATCH 5/6] Use the new error reporting to figure out exactly what was wrong. --- .../GrEclipseFormatterStepSpecialCaseTest.java | 18 ++++++++++++++++-- .../groovy/greclipse/format/SomeClass.fixed | 12 ++++++++++++ .../groovy/greclipse/format/SomeClass.test | 2 +- 3 files changed, 29 insertions(+), 3 deletions(-) create mode 100644 testlib/src/main/resources/groovy/greclipse/format/SomeClass.fixed diff --git a/lib-extra/src/test/java/com/diffplug/spotless/extra/groovy/GrEclipseFormatterStepSpecialCaseTest.java b/lib-extra/src/test/java/com/diffplug/spotless/extra/groovy/GrEclipseFormatterStepSpecialCaseTest.java index 768e939dfb..8f832b89f0 100644 --- a/lib-extra/src/test/java/com/diffplug/spotless/extra/groovy/GrEclipseFormatterStepSpecialCaseTest.java +++ b/lib-extra/src/test/java/com/diffplug/spotless/extra/groovy/GrEclipseFormatterStepSpecialCaseTest.java @@ -15,16 +15,30 @@ */ package com.diffplug.spotless.extra.groovy; +import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; import com.diffplug.spotless.StepHarness; import com.diffplug.spotless.TestProvisioner; public class GrEclipseFormatterStepSpecialCaseTest { - /** https://github.com/diffplug/spotless/issues/1657 */ + /** + * https://github.com/diffplug/spotless/issues/1657 + * + * broken: ${parm == null ? "" : "$parm"} + * works: ${parm == null ? "" : "parm"} + */ @Test public void issue_1657() { + Assertions.assertThrows(IllegalArgumentException.class, () -> { + StepHarness.forStep(GrEclipseFormatterStep.createBuilder(TestProvisioner.mavenCentral()).build()) + .testResourceUnaffected("groovy/greclipse/format/SomeClass.test"); + }); + } + + @Test + public void issue_1657_fixed() { StepHarness.forStep(GrEclipseFormatterStep.createBuilder(TestProvisioner.mavenCentral()).build()) - .testResourceUnaffected("groovy/greclipse/format/SomeClass.test"); + .testResourceUnaffected("groovy/greclipse/format/SomeClass.fixed"); } } diff --git a/testlib/src/main/resources/groovy/greclipse/format/SomeClass.fixed b/testlib/src/main/resources/groovy/greclipse/format/SomeClass.fixed new file mode 100644 index 0000000000..8c93ca4ad0 --- /dev/null +++ b/testlib/src/main/resources/groovy/greclipse/format/SomeClass.fixed @@ -0,0 +1,12 @@ +package com.somepackage + +class SomeClass { + + def func(parm) { + """ + ${parm == null ? "" : "parm"} + ${parm == null ? "" : "parm"} + + """ + } +} diff --git a/testlib/src/main/resources/groovy/greclipse/format/SomeClass.test b/testlib/src/main/resources/groovy/greclipse/format/SomeClass.test index 09ea835cca..62d4df0fc5 100644 --- a/testlib/src/main/resources/groovy/greclipse/format/SomeClass.test +++ b/testlib/src/main/resources/groovy/greclipse/format/SomeClass.test @@ -6,7 +6,7 @@ class SomeClass { """ ${parm == null ? "" : "$parm"} ${parm == null ? "" : "$parm"} - + """ } } From c19d3c06f90e8e3ab1572434bf272cf0777d7208 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Wed, 5 Apr 2023 17:37:20 -0700 Subject: [PATCH 6/6] Fix spotbugs warning. --- .../com/diffplug/spotless/extra/EquoBasedStepBuilder.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib-extra/src/main/java/com/diffplug/spotless/extra/EquoBasedStepBuilder.java b/lib-extra/src/main/java/com/diffplug/spotless/extra/EquoBasedStepBuilder.java index 66993e2f6c..db2e6674db 100644 --- a/lib-extra/src/main/java/com/diffplug/spotless/extra/EquoBasedStepBuilder.java +++ b/lib-extra/src/main/java/com/diffplug/spotless/extra/EquoBasedStepBuilder.java @@ -22,6 +22,8 @@ import java.util.Map; import java.util.Properties; +import javax.annotation.Nullable; + import com.diffplug.spotless.FileSignature; import com.diffplug.spotless.FormatterFunc; import com.diffplug.spotless.FormatterProperties; @@ -53,7 +55,7 @@ public EquoBasedStepBuilder(String formatterName, Provisioner mavenProvisioner, } /** Initialize valid default configuration, taking latest version */ - public EquoBasedStepBuilder(String formatterName, Provisioner mavenProvisioner, String defaultVersion, ThrowingEx.Function stateToFormatter) { + public EquoBasedStepBuilder(String formatterName, Provisioner mavenProvisioner, @Nullable String defaultVersion, ThrowingEx.Function stateToFormatter) { this.formatterName = formatterName; this.mavenProvisioner = mavenProvisioner; this.formatterVersion = defaultVersion;