openrewrite / openrewrite/rewrite

Most language `Assertions` ignore `TypeValidation` entirely, including `unknown()`

Open
#8,568 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

enhancement parser
Dominant language
Java
Stars
3.7k
Forks
570
Avg merge
13h 12m
Merged PRs (30d)
261

Description

Background

TypeValidation reaches a test only through SourceSpec.validateSource, which RewriteTest invokes for the before-source and for the after-recipe result (rewrite-test/src/main/java/org/openrewrite/test/RewriteTest.java:350 and :498). Each language's Assertions chooses what to pass, and most pass SourceSpec.ValidateSource.noop — either explicitly, or implicitly via the 5-arg SourceSpec constructor that defaults to it (rewrite-test/src/main/java/org/openrewrite/test/SourceSpec.java:79).

The result is that in those modules no TypeValidation invariant is checked at all — not unknown(), not erroneous(), not the type-metadata flags — even though every one of their compilation units implements JavaSourceFile, which is the only thing org.openrewrite.java.Assertions.validateTypes requires.

Module validateSource unknown() honored
rewrite-java Assertions::validateTypes yes
  • | rewrite-kotlin | Assertions::validateTypes | yes, since #8557 |
    | rewrite-groovy, rewrite-gradle, rewrite-scala, rewrite-python, rewrite-javascript, rewrite-csharp, rewrite-ruby, rewrite-go | ValidateSource.noop | no |

  • This is not theoretical. ScalaParserVisitor wraps a whole file in a J.Unknown when no statement converts (rewrite-scala/src/main/java/org/openrewrite/scala/ScalaParserVisitor.java:103-120), the TypeScript parser falls back to visitUnknown in a dozen places, and C#/Ruby/Python all carry visitUnknown in their visitors. #8557 is the cautionary tale: a J.Unknown swallowing an entire top-level Kotlin declaration passed CI unnoticed until the check was added.

Corroborating signal: typeValidationOptions(...) appears in zero tests across groovy, scala, python, javascript, csharp, ruby and go. Nothing has ever needed to opt out, because nothing is on.

Out of scope: rewrite-maven and rewrite-docker

Both were checked and deliberately excluded — their trees cannot hold what the remaining flags check:

  • rewrite-maven only builds SourceSpec<Xml.Document>, and Xml.Document implements Xml, SourceFileWithReferences — no J in the tree, so unknown()/erroneous()/type-metadata are unreachable. dependencyModel() is the only meaningful flag and pomResolvedSuccessfully already implements it.
  • rewrite-docker only builds SourceSpec<Docker.File>, and Docker extends Tree with no org.openrewrite.java dependency at all. Its validate handles allowNonWhitespaceInWhitespace() and must stay: rewrite-docker ships no WhitespaceValidationService, so the generic check in RewriteTest.java:383 throws UnsupportedOperationException and is silently skipped, and the Docker visitor additionally strips line continuations.

Everything else (parseAndPrintEquality, cursorAcyclic, immutableExecutionContext, immutableScanning, and allowNonWhitespaceInWhitespace where a WhitespaceValidationService exists) is enforced by RewriteTest generically, independent of validateSource.


A. Honor unknown() / erroneous() per language

  • Each of these replaces ValidateSource.noop with a validator that fails on J.Unknown when typeValidation.unknown() and on J.Erroneous when typeValidation.erroneous(), modeled on org.openrewrite.kotlin.Assertions#assertNoUnknownElements (see #8557). Independent of each other; do B1 first if you want to avoid copy-paste.

Expect existing tests in the module to start failing — that is the point. Each failure is either a parser gap worth fixing or an explicit .typeValidationOptions(TypeValidation.builder().unknown(false).build()) with a linked issue. Do not enable the type-metadata flags as part of these (see C1).

  • A1 — rewrite-groovy (groovy(...))
  • A2 — rewrite-gradle: buildGradle/settingsGradle (G.CompilationUnit) and buildGradleKts/settingsGradleKts (K.CompilationUnit). The kts specs build a Kotlin LST but bypass the check added in #8557
  • A3 — rewrite-scala (scala(...), sbt(...))
  • A4 — rewrite-python (python(...))
  • A5 — rewrite-javascript, Java-side Assertions (javascript/typescript/jsx/tsx)
  • A6 — rewrite-csharp (csharp(...))
  • A7 — rewrite-ruby (ruby(...), gemfile(...), rakefile(...))
  • A8 — rewrite-go (go(...)) https://github.com/openrewrite/rewrite/pull/8573

B. Shared plumbing and small fixes

  • B1 — Extract the J.Unknown/J.Erroneous scan into one reusable helper (rewrite-java and rewrite-kotlin already carry near-identical copies) so A1–A8 delegate instead of duplicating ~20 lines each. The Kotlin variant is the better base: it surfaces the ParseExceptionResult message alongside the offending source text
  • B2org.openrewrite.java.Assertions#validateTypes reports unknowns with the message "LST contains erroneous nodes", identical to the J.Erroneous branch directly above it, and drops the ParseExceptionResult message. Fix the wording and include the parse exception
  • B3 — The TypeScript-side harness (rewrite-javascript/rewrite/src/test/rewrite-test.ts) has no unknown/erroneous validation either, so TS-authored parser tests are blind to it in the same way. Add the equivalent there
  • B4TypeValidation.none() is documented as "Skip all invariant validation checks" but passes allowNonWhitespaceInWhitespace = false, which enables whitespace validation. That flag's polarity is inverted relative to its siblings; decide whether none() should pass true or whether the field should be renamed

C. Needs a decision before implementing

  • C1 — Per language, decide whether the type-metadata flags (identifiers, methodInvocations, methodDeclarations, classDeclarations, constructorInvocations, allowMissingType) can ever be enabled. For the dynamic languages this likely stays off permanently; if so, say so in the Assertions javadoc so the next reader does not assume it is an oversight
  • C2dependencyModel() is implemented only by rewrite-maven. Decide whether Gradle specs should assert the GradleProject/GradleSettings marker the way pomXml asserts MavenResolutionResult

Quick way to see the current state: grep -rn "ValidateSource.noop" --include=Assertions.java ., plus every Assertions that uses the 5-arg SourceSpec constructor.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with the ValidateSource.noop usages found by the issue's grep command, then read RewriteTest.java, SourceSpec.java, and the Kotlin Assertions implementation from #8557. Compare each language's Assertions entry points and tests, and define completion as unknown/erroneous validation being honored with intentional opt-outs documented and tested; the shared helper and decision items require maintainer input.

Written by the indexing model from the issue text.

Assessment

Tech stack
csharp, go, groovy, java, javascript, kotlin, python, ruby, scala, typescript
Domain
developer-experience, testing-qa
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
42/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.