From 9feb558e9ea490386c59e2d59b813702bdf2523f Mon Sep 17 00:00:00 2001 From: Frotty Date: Fri, 4 Sep 2026 17:56:05 +0200 Subject: [PATCH 1/3] Enable Error Prone, fix two dead compiler checks, cut hot-path allocations, and stop parallel test forks racing on a shared output file. --- de.peeeq.wurstscript/build.gradle | 41 +++++++++++++- .../wurstio/CompiletimeFunctionRunner.java | 2 +- .../wurstscript/attributes/AttrExprType.java | 7 ++- .../wurstscript/attributes/AttrFuncDef.java | 3 +- .../attributes/OverloadingResolver.java | 5 -- .../attributes/names/NameLinks.java | 4 +- .../attributes/prettyPrint/PrettyUtils.java | 5 +- .../interpreter/TimerMockHandler.java | 2 +- .../imtojass/ImToJassTranslator.java | 4 +- .../imtranslation/EliminateGenerics.java | 3 -- .../lua/translation/LuaTranslator.java | 2 - .../tests/CompilerFuzzTestsSC.java | 16 ++++-- .../tests/wurstscript/tests/ScopingTests.java | 26 +++++++++ .../wurstscript/tests/WurstScriptTest.java | 54 +++++++++++++++++++ 14 files changed, 150 insertions(+), 24 deletions(-) diff --git a/de.peeeq.wurstscript/build.gradle b/de.peeeq.wurstscript/build.gradle index 07246f7fe..891981f9f 100644 --- a/de.peeeq.wurstscript/build.gradle +++ b/de.peeeq.wurstscript/build.gradle @@ -13,6 +13,7 @@ plugins { id 'eclipse' id 'idea' id 'jacoco' + id 'net.ltgt.errorprone' version '5.1.1' id 'maven-publish' id 'com.gradleup.shadow' version '9.2.2' id 'de.undercouch.download' version '5.6.0' @@ -41,7 +42,43 @@ tasks.withType(JavaExec).configureEach { jvmArgs('-XX:+UnlockExperimentalVMOptions', '-XX:+UseCompactObjectHeaders') } -tasks.withType(JavaCompile).configureEach { options.release = 25 } +/** -------- Compilation and static analysis -------- + * Both javac lint and Error Prone are ADVISORY: they report at warning severity and + * never fail the build. Unit tests and coverage remain the primary quality gate; this + * is a supplementary signal, not a merge blocker. + * Skip Error Prone entirely with: ./gradlew -PskipErrorProne + */ +def errorProneEnabled = !project.hasProperty('skipErrorProne') + +tasks.withType(JavaCompile).configureEach { + options.release = 25 + // 'serial'/'this-escape' are noisy against this codebase's AST and visitor style. + // 'auxiliaryclass' fires ~86 times purely on the multi-class-per-file layout - structural, not a defect. + options.compilerArgs += ['-Xlint:all', '-Xlint:-serial', '-Xlint:-this-escape', + '-Xlint:-processing', '-Xlint:-auxiliaryclass'] + // javac silently caps reporting at 100 warnings. Without this, lint noise exhausts the + // budget and every Error Prone finding is dropped as overflow - the analysis looks clean + // while reporting nothing. Do not remove. + options.compilerArgs += ['-Xmaxwarns', '10000'] + + options.errorprone { + enabled = errorProneEnabled + // src-gen holds ~700 generated AST classes - analysing them is pure noise. + excludedPaths = '.*src-gen.*' + disableWarningsInGeneratedCode = true + // Advisory only: demote every check to a warning so the build never fails on it. + // Requires the -Xmaxwarns bump above to actually be visible. + allErrorsAsWarnings = true + // Upstream bug: NonCanonicalType NPEs in SuggestedFixes.qualifyType on anonymous + // subclasses of nested types, which this codebase's visitors use everywhere + // (e.g. `new LuaModel.DefaultVisitor() {}`). A plugin crash is a hard javac error, + // so allErrorsAsWarnings cannot absorb it. Purely stylistic check; safe to drop. + disable('NonCanonicalType') + } +} + +// Keep the test compile loop fast - tests are the primary gate, not the audit target. +tasks.named('compileTestJava') { options.errorprone.enabled = false } jacoco { toolVersion = "0.8.13" @@ -90,6 +127,8 @@ configurations { } dependencies { + errorprone 'com.google.errorprone:error_prone_core:2.50.0' + implementation 'org.jetbrains:annotations:23.0.0' // Antlr diff --git a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstio/CompiletimeFunctionRunner.java b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstio/CompiletimeFunctionRunner.java index 359c625ea..09f9c849b 100644 --- a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstio/CompiletimeFunctionRunner.java +++ b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstio/CompiletimeFunctionRunner.java @@ -120,7 +120,7 @@ public void run() { collectCompiletimeFunctions(toExecute); long tCollected = System.nanoTime(); - toExecute.sort(Comparator.comparing(this::getOrderIndex)); + toExecute.sort(Comparator.comparingInt(this::getOrderIndex)); long tSorted = System.nanoTime(); execute(toExecute); diff --git a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/attributes/AttrExprType.java b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/attributes/AttrExprType.java index d0d065e87..a1e608502 100644 --- a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/attributes/AttrExprType.java +++ b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/attributes/AttrExprType.java @@ -74,7 +74,12 @@ public static WurstType calculate(ExprVarAccess term) { return WurstTypeUnknown.instance(); } if (!(varDef instanceof OtherLink) && varDef.getDef() instanceof VarDef) { - if (Utils.getParentVarDef(Optional.of(term)) == Optional.of((VarDef) varDef.getDef())) { + // Compare the enclosing VarDef to the one this access resolves to. Both sides used + // to be wrapped in fresh Optionals and compared with ==, which is never true, so + // this check silently did nothing. getParentVarDef returns null (not empty) when + // there is no enclosing VarDef, hence the explicit null guard. + Optional enclosingVarDef = Utils.getParentVarDef(Optional.of(term)); + if (enclosingVarDef != null && enclosingVarDef.orElse(null) == varDef.getDef()) { term.addError("Recursive variable definition is not allowed."); return WurstTypeUnknown.instance(); } diff --git a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/attributes/AttrFuncDef.java b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/attributes/AttrFuncDef.java index 69960e244..623b43b2a 100644 --- a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/attributes/AttrFuncDef.java +++ b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/attributes/AttrFuncDef.java @@ -197,9 +197,8 @@ private ToStringConversionResolution(@Nullable FuncLink conversion, @Nullable St var raw = NameResolution.lookupMemberFuncs(node, recvT, node.getFuncName(), /*showErrors=*/false); java.util.ArrayList visible = new java.util.ArrayList<>(raw.size()); - java.util.ArrayList hidden = new java.util.ArrayList<>(raw.size()); for (var f : raw) { - if (isVisible(f)) visible.add(f); else hidden.add(f); + if (isVisible(f)) visible.add(f); } if (!raw.isEmpty() && visible.isEmpty()) { diff --git a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/attributes/OverloadingResolver.java b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/attributes/OverloadingResolver.java index 6b96df6be..c2d96d957 100644 --- a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/attributes/OverloadingResolver.java +++ b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/attributes/OverloadingResolver.java @@ -5,7 +5,6 @@ import de.peeeq.wurstscript.types.WurstType; import de.peeeq.wurstscript.types.WurstTypeTypeParam; import de.peeeq.wurstscript.types.WurstTypeVararg; -import de.peeeq.wurstscript.utils.NotNullList; import de.peeeq.wurstscript.utils.Utils; import org.eclipse.jdt.annotation.Nullable; @@ -53,8 +52,6 @@ Optional resolve(Iterable alternativeFunctions, C caller) { if (size == 1) { return Optional.of(Utils.getFirst(alternativeFunctions)); } - List hints = new NotNullList<>(); - Map numMatches = new HashMap<>(); for (F f : alternativeFunctions) { if (!hasValidParameterCount(f, caller)) { @@ -68,8 +65,6 @@ Optional resolve(Iterable alternativeFunctions, C caller) { && expectedParamType instanceof WurstTypeTypeParam) { // should be ok! } else if (!getArgumentType(caller, i).isSubtypeOf(expectedParamType, f)) { - hints.add("Expected " + expectedParamType - + " as parameter " + i + " ,but found " + getArgumentType(caller, i) + "."); continue; } matches++; diff --git a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/attributes/names/NameLinks.java b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/attributes/names/NameLinks.java index 4793e0d22..78ce6bfcf 100644 --- a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/attributes/names/NameLinks.java +++ b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/attributes/names/NameLinks.java @@ -57,7 +57,9 @@ public static ImmutableMultimap calculate(ClassOrModuleOrModule @NotNull private static Map> initOverrideMap(Multimap result) { - Map> overrideCheckResults = new Hashtable<>(); + // LinkedHashMap, not Hashtable: nothing here is concurrent, and reportOverrideErrors + // iterates this map, so insertion order beats hash order for reproducible diagnostics. + Map> overrideCheckResults = new LinkedHashMap<>(); for (DefLink link : result.values()) { if (link instanceof FuncLink) { Map map = overrideCheckResults.computeIfAbsent(link.getName(), diff --git a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/attributes/prettyPrint/PrettyUtils.java b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/attributes/prettyPrint/PrettyUtils.java index 3cb3883bd..ce210c815 100644 --- a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/attributes/prettyPrint/PrettyUtils.java +++ b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/attributes/prettyPrint/PrettyUtils.java @@ -27,8 +27,11 @@ public static void pretty(List args) throws IOException { return; } String arg = args.get(0); - if (args.equals("...")) { + // Was args.equals("...") - comparing the List to a String, which is never true, so + // the "..." argument silently fell through to being treated as a file name below. + if (arg.equals("...")) { prettyAll("."); + return; } if (arg.equals("tree") && args.size() >= 2) { debug(args.get(1)); diff --git a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/intermediatelang/interpreter/TimerMockHandler.java b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/intermediatelang/interpreter/TimerMockHandler.java index 8e3619af4..a64c67aca 100644 --- a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/intermediatelang/interpreter/TimerMockHandler.java +++ b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/intermediatelang/interpreter/TimerMockHandler.java @@ -10,7 +10,7 @@ */ public class TimerMockHandler { private float virtualTime = 0; - private final PriorityQueue nextRunnable = new PriorityQueue<>(Comparator.comparing(r -> r.time)); + private final PriorityQueue nextRunnable = new PriorityQueue<>(Comparator.comparingDouble(r -> r.time)); public void cancelTask(RunTask runTask) { nextRunnable.remove(runTask); diff --git a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtojass/ImToJassTranslator.java b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtojass/ImToJassTranslator.java index 9247745d0..5e51b2caf 100644 --- a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtojass/ImToJassTranslator.java +++ b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtojass/ImToJassTranslator.java @@ -67,8 +67,8 @@ private void makeNamesUnique(List list) { List sorted = new ArrayList<>(list); sorted.sort(Comparator.comparing(JassImElementWithName::getName) .thenComparing(v -> v.getTrace().attrSource().getFile()) - .thenComparing(v -> v.getTrace().attrSource().getLine()) - .thenComparing(v -> v.getTrace().attrSource().getStartColumn())); + .thenComparingInt(v -> v.getTrace().attrSource().getLine()) + .thenComparingInt(v -> v.getTrace().attrSource().getStartColumn())); Set used = new HashSet<>(sorted.size() * 2); Map nextSuffix = new HashMap<>(); diff --git a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/EliminateGenerics.java b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/EliminateGenerics.java index e32390131..90c232a74 100644 --- a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/EliminateGenerics.java +++ b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/EliminateGenerics.java @@ -1162,9 +1162,6 @@ private void dbgMethodsByName(String phase) { } private String checkDanglingMethodRefs(String phase) { - IdentityHashMap inProg = new IdentityHashMap<>(); - for (ImMethod m : prog.getMethods()) inProg.put(m, Boolean.TRUE); - final int[] dangling = {0}; prog.accept(new Element.DefaultVisitor() { diff --git a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/lua/translation/LuaTranslator.java b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/lua/translation/LuaTranslator.java index 29188d783..35ebbf2fa 100644 --- a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/lua/translation/LuaTranslator.java +++ b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/lua/translation/LuaTranslator.java @@ -812,12 +812,10 @@ private void translateFunc(ImFunction f) { } // translate local variables - List functionLocals = new ArrayList<>(); for (ImVar local : f.getLocals()) { LuaVariable luaLocal = luaVar.getFor(local); luaLocal.setInitialValue(defaultValue(local.getType())); lf.getBody().add(luaLocal); - functionLocals.add(luaLocal); } // translate body: diff --git a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/CompilerFuzzTestsSC.java b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/CompilerFuzzTestsSC.java index 37b994aa1..09f5b280a 100644 --- a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/CompilerFuzzTestsSC.java +++ b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/CompilerFuzzTestsSC.java @@ -27,16 +27,24 @@ public void generatedProgramsAreCrashFree(@From(RandomProgram.class) Program pro @Property(maxInvocations = 64) public void generatedProgramsCompileForBothBackends(@From(RandomProgram.class) Program program) { - assertCompilesForBothBackends(program); + assertCompilesForBothBackends(program, "generatedProgramsCompileForBothBackends"); } @Test public void generatedCorpusCompilesForBothBackends() { - new RandomProgram().generate(0).forEach(this::assertCompilesForBothBackends); + new RandomProgram().generate(0) + .forEach(p -> assertCompilesForBothBackends(p, "generatedCorpusCompilesForBothBackends")); } - private void assertCompilesForBothBackends(Program program) { - CompilationResult result = test() + /** + * Both callers used to let test() name the output after this helper, so they shared one file + * under ./test-output/. They run in different Gradle forks - the @Test under TestNG, the + * @Property under SmallCheckViaJUnitCoreTestNG - so the two JVMs raced on that file and pjass + * intermittently parsed a spliced result, reporting word fragments as undefined types. Naming + * the output after the calling test keeps them apart. + */ + private void assertCompilesForBothBackends(Program program, String testName) { + CompilationResult result = testNamed(testName) .setStopOnFirstError(false) .executeProg(false) .testLua(true) diff --git a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/ScopingTests.java b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/ScopingTests.java index c951e8dcf..29093532f 100644 --- a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/ScopingTests.java +++ b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/ScopingTests.java @@ -95,5 +95,31 @@ public void privateCode() { "endpackage"); } + /** + * A class field whose initializer refers to itself must be rejected. + * Globals are caught earlier by "must be declared before it is used" and locals by + * flow analysis, so class fields are the only shape that reaches the check in + * AttrExprType.calculate(ExprVarAccess). That check compared two freshly allocated + * Optionals with ==, so it was unreachable and this compiled silently. + */ + @Test + public void test_recursive_class_field_def() { + testAssertErrorsLines(false, "Recursive variable definition is not allowed", + "package test", + " class C", + " int x = x + 1", + "endpackage"); + } + + /** Guard against the check over-triggering: a field initialized from another field is fine. */ + @Test + public void test_non_recursive_class_field_def_ok() { + testAssertOkLines(false, + "package test", + " class C", + " int a = 1", + " int b = a + 1", + "endpackage"); + } } diff --git a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/WurstScriptTest.java b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/WurstScriptTest.java index f0c2fe1cd..4e77f892e 100644 --- a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/WurstScriptTest.java +++ b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/WurstScriptTest.java @@ -40,6 +40,9 @@ import java.nio.charset.StandardCharsets; import java.util.*; import java.util.Map.Entry; +import java.security.MessageDigest; +import java.security.NoSuchAlgorithmException; +import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.TimeUnit; import java.util.stream.Collectors; @@ -432,6 +435,24 @@ public TestConfig test() { return new TestConfig(name); } + /** + * Like {@link #test()} but with an explicit name for the files written under + * {@link #TEST_OUTPUT_PATH}. + * + *

{@link #test()} names those files after the first frame outside WurstScriptTest, which is + * the *helper* whenever a test reaches it through one of its own methods. That is deliberate - + * DeterministicChecks relies on one test method producing several differently named outputs - + * but it means two test methods sharing a helper get the same name, and therefore the same + * file. Gradle runs test classes in parallel forks, so when those two methods live in + * different suites the JVMs race on one .j file and pjass parses a spliced result. + * + *

Pass an explicit name in that situation. {@code name} is prefixed with the test class, + * exactly as {@link #test()} does. + */ + public TestConfig testNamed(String name) { + return new TestConfig(this.getClass().getSimpleName() + "_" + name); + } + void testAssertOk(boolean excuteProg, boolean withStdLib, CU... units) { test().executeProg(excuteProg).withStdLib(withStdLib).compilationUnits(units); } @@ -1122,12 +1143,45 @@ WurstModel parseFiles(Iterable inputFiles, } + /** + * Scripts already checked by pjass in this JVM, by content hash. + * + *

pjass parses every file it is given as one program, so independent test scripts cannot be + * batched into a single invocation - each one costs a process spawn plus a re-parse of + * common.j and blizzard.j (~15k lines). About a third of the scripts this suite emits are + * byte-identical to one already checked (the same source compiled under several optimisation + * levels, and near-identical fixtures within a test class), and pjass is a pure function of its + * input, so checking those again cannot tell us anything new. Spawning is the dominant cost on + * Windows, where CreateProcess is an order of magnitude dearer than fork/exec. + * + *

Only successes are recorded: a failure re-runs so the reported message names the file the + * caller actually passed. + */ + private static final Set pjassCheckedScripts = ConcurrentHashMap.newKeySet(); + private void runPjass(File outputFile) throws Error { + String digest = scriptDigest(outputFile); + if (digest != null && pjassCheckedScripts.contains(digest)) { + return; + } Result pJassResult = Pjass.runPjass(outputFile); WLogger.info(pJassResult.getMessage()); if (!pJassResult.isOk() && !pJassResult.getMessage().equals("IO Exception")) { throw new Error(pJassResult.getMessage() + pJassResult.getErrors()); } + if (digest != null) { + pjassCheckedScripts.add(digest); + } + } + + /** Content hash of a generated script, or null if it cannot be read - in which case pjass runs. */ + private static String scriptDigest(File file) { + try { + MessageDigest md = MessageDigest.getInstance("SHA-256"); + return HexFormat.of().formatHex(md.digest(java.nio.file.Files.readAllBytes(file.toPath()))); + } catch (IOException | NoSuchAlgorithmException e) { + return null; + } } private static String currentTestEnv = ""; From 4bcd0269107d059b6fa1d03556cb6afe2ce8a7ce Mon Sep 17 00:00:00 2001 From: Frotty Date: Fri, 4 Sep 2026 18:17:52 +0200 Subject: [PATCH 2/3] Match compiler fuzz budgets to the generators' distinct shapes and assert newline and recompile invariance instead of non-null. --- .../tests/CompilerFuzzTestsSC.java | 73 +++++++++++++++++-- 1 file changed, 65 insertions(+), 8 deletions(-) diff --git a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/CompilerFuzzTestsSC.java b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/CompilerFuzzTestsSC.java index 09f5b280a..66352a0d6 100644 --- a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/CompilerFuzzTestsSC.java +++ b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/CompilerFuzzTestsSC.java @@ -8,6 +8,10 @@ import smallcheck.annotations.Property; import smallcheck.generators.SeriesGen; +import java.io.File; +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; import java.util.ArrayList; import java.util.Arrays; import java.util.List; @@ -17,7 +21,18 @@ @RunWith(SmallCheckRunner.class) public class CompilerFuzzTestsSC extends WurstScriptTest { - @Property(maxInvocations = 320) + /** + * How many structurally distinct programs each generator can actually produce. + * + *

buildRandomSingleProgram varies its shape with six bits of the seed - indent style, + * tuple, interface, module, loop, callback - so there are 2^6 shapes; the rest of the seed + * only renames packages and changes integer literals, which reaches no new compiler path. + * Budgets above these counts recompile the same shapes under different names. + */ + private static final int SINGLE_PROGRAM_SHAPES = 64; + private static final int CROSS_PACKAGE_SHAPES = 24; + + @Property(maxInvocations = SINGLE_PROGRAM_SHAPES) public void generatedProgramsAreCrashFree(@From(RandomProgram.class) Program program) { CompilationResult result = runProgram(program); @@ -25,7 +40,7 @@ public void generatedProgramsAreCrashFree(@From(RandomProgram.class) Program pro Assert.assertNotNull(result.getGui()); } - @Property(maxInvocations = 64) + @Property(maxInvocations = SINGLE_PROGRAM_SHAPES) public void generatedProgramsCompileForBothBackends(@From(RandomProgram.class) Program program) { assertCompilesForBothBackends(program, "generatedProgramsCompileForBothBackends"); } @@ -56,16 +71,58 @@ private void assertCompilesForBothBackends(Program program, String testName) { + "\nsource:\n" + String.join("\n---\n", program.sources)); } - @Property(maxInvocations = 180) - public void mixedNewlineStylesAreCrashFree(@From(RandomProgram.class) Program program) { + /** + * Line endings are a lexer detail: the same source written with LF and with CRLF has to emit + * byte-identical Jass. This previously only asserted that compiling the CRLF variant returned + * non-null, which no realistic bug would violate. + */ + @Property(maxInvocations = SINGLE_PROGRAM_SHAPES) + public void newlineStyleDoesNotAffectEmittedCode(@From(RandomProgram.class) Program program) { String alternateNewline = "\n".equals(program.newline) ? "\r\n" : "\n"; - CompilationResult result = runProgram(program.withNewline(alternateNewline)); - Assert.assertNotNull(result); - Assert.assertNotNull(result.getGui()); + String fromOriginal = compileAndReadJass(program, "newlineOriginal"); + String fromAlternate = compileAndReadJass(program.withNewline(alternateNewline), "newlineAlternate"); + + Assert.assertEquals(fromAlternate, fromOriginal, + "line ending style changed the emitted Jass\nsource:\n" + String.join("\n---\n", program.sources)); + } + + /** + * The same source compiled twice in one process has to emit the same script. DeterministicChecks + * pins this for a few hand-written programs; this runs it across every generated shape. + */ + @Property(maxInvocations = SINGLE_PROGRAM_SHAPES) + public void compilingTwiceEmitsIdenticalCode(@From(RandomProgram.class) Program program) { + String first = compileAndReadJass(program, "determinismFirst"); + String second = compileAndReadJass(program, "determinismSecond"); + + Assert.assertEquals(second, first, + "recompiling the same source emitted different Jass\nsource:\n" + String.join("\n---\n", program.sources)); + } + + /** + * Compiles {@code program} and returns the unoptimised Jass it emitted. The output name is + * explicit so two compilations inside one property do not overwrite each other's file. + */ + private String compileAndReadJass(Program program, String outputName) { + CompilationResult result = testNamed(outputName) + .setStopOnFirstError(false) + .executeProg(false) + .compilationUnits(asCompilationUnits(program)); + + Assert.assertTrue(result.getGui().getErrorList().isEmpty(), + "generated program produced compiler diagnostics: " + result.getGui().getErrorList() + + "\nsource:\n" + String.join("\n---\n", program.sources)); + + File emitted = new File(TEST_OUTPUT_PATH + getClass().getSimpleName() + "_" + outputName + "_no_opts.j"); + try { + return Files.readString(emitted.toPath(), StandardCharsets.UTF_8); + } catch (IOException e) { + throw new AssertionError("could not read emitted script " + emitted, e); + } } - @Property(maxInvocations = 120) + @Property(maxInvocations = CROSS_PACKAGE_SHAPES) public void crossPackageProgramsAreCrashFree(@From(CrossPackageProgram.class) Program program) { CompilationResult result = runProgram(program); From 5d8fda84ef7fecfbb610f0dce9dbe3a3bc593124 Mon Sep 17 00:00:00 2001 From: Frotty Date: Fri, 4 Sep 2026 19:53:29 +0200 Subject: [PATCH 3/3] Record only validated pjass results and cover the pretty-print argument dispatch. --- .../attributes/prettyPrint/PrettyUtils.java | 46 ++++++++++++----- .../prettyprint/PrettyUtilsArgsTest.java | 49 +++++++++++++++++++ .../wurstscript/tests/WurstScriptTest.java | 5 +- 3 files changed, 88 insertions(+), 12 deletions(-) create mode 100644 de.peeeq.wurstscript/src/test/java/tests/prettyprint/PrettyUtilsArgsTest.java diff --git a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/attributes/prettyPrint/PrettyUtils.java b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/attributes/prettyPrint/PrettyUtils.java index ce210c815..76bff0ee6 100644 --- a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/attributes/prettyPrint/PrettyUtils.java +++ b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/attributes/prettyPrint/PrettyUtils.java @@ -22,24 +22,48 @@ public class PrettyUtils { /** * @param args */ - public static void pretty(List args) throws IOException { - if (args.size() == 0) { - return; + /** + * What {@link #pretty(List)} does with a given argument list. + * + *

Split out from the dispatch so it can be asserted directly: the alternative is running the + * real thing, and both the directory walk and the single-file branch print to stdout while + * readFile swallows its own exceptions, so neither outcome is distinguishable from the other. + */ + public enum PrettyAction { + /** No arguments; nothing to do. */ + NONE, + /** "..." - format every .wurst file below the root. */ + ALL, + /** "tree " - dump the parse tree. */ + TREE, + /** Anything else is taken as a file name. */ + SINGLE_FILE + } + + public static PrettyAction selectAction(List args) { + if (args.isEmpty()) { + return PrettyAction.NONE; } String arg = args.get(0); - // Was args.equals("...") - comparing the List to a String, which is never true, so - // the "..." argument silently fell through to being treated as a file name below. + // This used to read args.equals("..."), comparing the List itself to a String, which is + // never true - so "..." fell through and was treated as a file name. if (arg.equals("...")) { - prettyAll("."); - return; + return PrettyAction.ALL; } if (arg.equals("tree") && args.size() >= 2) { - debug(args.get(1)); - return; + return PrettyAction.TREE; } + return PrettyAction.SINGLE_FILE; + } - String clean = pretty(new File(arg)); - System.out.println(clean); + public static void pretty(List args) throws IOException { + switch (selectAction(args)) { + case NONE -> { + } + case ALL -> prettyAll("."); + case TREE -> debug(args.get(1)); + case SINGLE_FILE -> System.out.println(pretty(new File(args.get(0)))); + } } public static String pretty(String source, String ending) { diff --git a/de.peeeq.wurstscript/src/test/java/tests/prettyprint/PrettyUtilsArgsTest.java b/de.peeeq.wurstscript/src/test/java/tests/prettyprint/PrettyUtilsArgsTest.java new file mode 100644 index 000000000..0dc48725a --- /dev/null +++ b/de.peeeq.wurstscript/src/test/java/tests/prettyprint/PrettyUtilsArgsTest.java @@ -0,0 +1,49 @@ +package tests.prettyprint; + +import de.peeeq.wurstscript.attributes.prettyPrint.PrettyUtils; +import org.testng.annotations.Test; + +import java.util.Arrays; +import java.util.Collections; + +import static org.testng.AssertJUnit.assertEquals; + +/** + * Argument dispatch for the -prettyPrint CLI entry point. + * + *

The "..." branch used to compare the argument List itself to a String, so it was never taken + * and "..." was treated as a file name instead; readFile then swallowed the resulting + * FileNotFoundException, so the mistake produced no visible failure. + */ +public class PrettyUtilsArgsTest { + + @Test + public void tripleDotSelectsDirectoryFormatting() { + assertEquals(PrettyUtils.PrettyAction.ALL, + PrettyUtils.selectAction(Collections.singletonList("..."))); + } + + @Test + public void aFileNameIsNotTreatedAsDirectoryFormatting() { + assertEquals(PrettyUtils.PrettyAction.SINGLE_FILE, + PrettyUtils.selectAction(Collections.singletonList("some/file.wurst"))); + } + + @Test + public void treeWithAFileSelectsTreeDump() { + assertEquals(PrettyUtils.PrettyAction.TREE, + PrettyUtils.selectAction(Arrays.asList("tree", "some/file.wurst"))); + } + + @Test + public void treeWithoutAFileIsTreatedAsAFileName() { + assertEquals(PrettyUtils.PrettyAction.SINGLE_FILE, + PrettyUtils.selectAction(Collections.singletonList("tree"))); + } + + @Test + public void noArgumentsDoesNothing() { + assertEquals(PrettyUtils.PrettyAction.NONE, + PrettyUtils.selectAction(Collections.emptyList())); + } +} diff --git a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/WurstScriptTest.java b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/WurstScriptTest.java index 4e77f892e..ba2e5fc9f 100644 --- a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/WurstScriptTest.java +++ b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/WurstScriptTest.java @@ -1169,7 +1169,10 @@ private void runPjass(File outputFile) throws Error { if (!pJassResult.isOk() && !pJassResult.getMessage().equals("IO Exception")) { throw new Error(pJassResult.getMessage() + pJassResult.getErrors()); } - if (digest != null) { + // Only a real pass may be recorded. An "IO Exception" result is deliberately not fatal, but + // it means pjass never validated this script - caching it would make every later identical + // script skip validation too, after a failure that may well have been transient. + if (digest != null && pJassResult.isOk()) { pjassCheckedScripts.add(digest); } }