Unify Kotlin test suites#22135
Draft
andersfugmann wants to merge 17 commits into
Draft
Conversation
andersfugmann
force-pushed
the
andersfugmann/kotlin-unify-tests
branch
7 times, most recently
from
a0e698f to
ff707b3
Compare
…otlin2 - Port ministdlib from test-kotlin1 to test-kotlin2. The ministdlib test exercises a minimal Kotlin standard library written from scratch. Its options file is updated to include -language-version 2.0 so the test runs in K2 mode when the K2 compiler is active. - Port nested_types from test-kotlin2 to test-kotlin1. The nested_types test exercises type-alias and inner-type queries. Expected output is identical in K1 and K2 modes so no expected-file changes are needed. - Add test-kotlin2/options with codeql-extractor-kotlin-options: -language-version 2.0. The CodeQL CLI adds -language-version 1.9 by default in legacy test extraction mode. Without this override the K2 test suite would run in K1 mode, defeating the purpose of the split. Both ministdlib and nested_types produce byte-identical expected output across K1 (2.3.20, -language-version 1.9) and K2 (2.4.0, default K2). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
andersfugmann
force-pushed
the
andersfugmann/kotlin-unify-tests
branch
2 times, most recently
from
f458cd6 to
0027db7
Compare
In K2 mode the frontend emits `-123L` as IrCall(unaryMinus, IrConst(123L)) rather than IrConst(-123L) as in K1. Queries that search for negative numeric literals therefore need to match both a UnaryMinusExpr wrapping a literal and a plain literal, depending on language mode. Fix: when extractCallExpression encounters an isNumericFunction(unaryMinus) call whose dispatchReceiver is already an IrConst, fold the negation into the constant before extracting. The resulting literal node is identical to what K1 emits. Location: extend the span one character to the left to cover the `-` sign. In K2 the IrCall's startOffset equals the receiver's startOffset, so we recover the minus by subtracting one from the receiver offset. K1 is unaffected: the K1 frontend folds the sign into the constant before IR generation, so this new branch never triggers when compiling with -language 1.9. Expected output changes: - test-kotlin2/library-tests/literals/literals.expected: negative long, float and double literals now appear as plain typed literals instead of as UnaryMinus nodes. The file is now byte-identical to test-kotlin1/library-tests/literals/literals.expected. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…d test-kotlin2 Remove the K1-only comment '// This doesn't generate a throw statement in Kotlin 1 mode' from test-kotlin1/library-tests/generated-throws/generated-throws.kt so the source files are byte-identical between the two suites. The expected outputs still legitimately differ: in K2 mode the compiler generates an implicit throw NoWhenBranchMatchedException for the exhaustive sealed-class when expression, but the K1 frontend does not emit this node. This is a mode-specific behaviour difference that cannot be bridged by an extractor change. Both tests continue to pass: - test-kotlin1 (kotlinc 2.3.20 / -language 1.9): 0 throw results (unchanged) - test-kotlin2 (kotlinc 2.4.0 / -language 2.0): 1 throw result (unchanged) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copy K1's richer C2.kt (which includes test2, test3 and l.get(0) in test) into test-kotlin2. K2 in -language 2.0 mode already tracks dataflow through array.set() and indirect wrapper calls, so the additional tests produce results identical to K1. The expected files for test-kotlin1 and test-kotlin2 are now byte-identical for this test. Verified: - test-kotlin1 (kotlinc 2.3.20 / -language 1.9): all tests pass - test-kotlin2 (kotlinc 2.4.0 / -language 2.0): all tests pass Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
K1 and K2 IR backends compute source locations differently. K1 uses the IR node's synthetic startOffset/endOffset, while K2 reconstructs source positions from PSI. For expression-level nodes this causes location differences across the two language modes. Introduce PSI-backed location lookup as the preferred source for spans wherever PSI is available: getPsiBasedLocation(element) ?: tw.getLocation(element) getPsiBasedLocation() resolves the PSI element for the IR node via psi2Ir.findPsiElement() and builds a location from its startOffset..endOffset. currentIrFile is tracked in extractFileContents so the PSI lookup has the file context it needs. Applied to expression-level nodes (both K1 and K2 modes): - local variable declarations (extractVariable, extractVariableExpr) - IrLocalDelegatedProperty blocks - IrWhen expressions and when-branches - IrGetValue (varaccess) expressions - IrFunctionExpression (lambda) nodes - Block statements (extractBlock) - this/super access expressions (extractThisAccess) - String literals Declaration-level nodes (class, function, property) are guarded with if (usesK2) to avoid a regression in K1 mode where the PSI lookup causes parameterised type instantiations to appear as fromSource(), inflating generic-type query results. The K1 IR frontend does not map all declaration nodes cleanly to source PSI elements; for these nodes we keep the original IR-based location in K1 mode. Expected output changes (both suites): - controlflow/basic/bbStmts, bbStrictDominance, bbSuccessor, getASuccessor, strictDominance: when-branch and varaccess location improvements - java-kotlin-collection-type-generic-methods/test: new stdlib entries from JDK update (AbstractCollection<Runnable> methods) - annotation_classes/PrintAst: variable access location improvement in K1 - classes/genericExprTypes: location improvement in K1 - compilation-units/cus: removed two internal JDK inner-class entries (stdlib version change) - reflection/reflection: removed a few external-class entries (stdlib version) Verified: all 285 tests pass for both test-kotlin1 (kotlinc 2.3.20 / K1) and test-kotlin2 (kotlinc 2.4.0 / K2). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
In K1 mode, the IrCall node for the !! operator stores startOffset at the '!' character rather than at the start of the operand. This means that x!! was previously reported as spanning from '!' to the end of the expression, omitting the operand from the location. Fix this by computing the NotNullExpr location from the value argument's startOffset (the operand) to c.endOffset. This matches the K2 behaviour where the IrCall.startOffset already points at the operand. The fix guards against invalid (< 0) offsets on either side and falls back to the default IrCall location in those cases. Updated expected files in test-kotlin1: - exprs/unaryOp.expected: !! locations now start at operand column - exprs/exprs.expected: same (NotNullExpr entries corrected) - exprs/binop.expected: !! child locations now correct (parent binop location for s!!.plus(5) still differs due to a separate K1 IR limitation where the enclosing call inherits the wrong receiver offset) - controlflow/basic/bbStmts.expected: CFG references to !! now use the improved location - controlflow/basic/getASuccessor.expected: same Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…r to initializer
In K1 mode, IrVariable.startOffset points to the name identifier and
IrVariable.endOffset is also at the name end, giving a location that
covers only the variable name (e.g. "local1") rather than the full
declaration ("val local1 = 2 + 3").
In K2 mode the IR offsets already point from the val/var keyword to the
end of the initializer, matching the intuitive source span.
Fix this for K1 by adding an IrVariable-specific overload of
getPsiBasedLocation that:
1. finds the leaf PSI element at v.startOffset (the name identifier)
2. walks up to the enclosing KtVariableDeclaration (KtProperty)
3. uses the val/var keyword position as the start offset to exclude
any leading annotations from the span
This matches the K2 IR behavior and gives a more complete location for
variable declarations when used in CodeQL queries.
The fix applies to both K1 and K2 since the overload is unconditional;
for K2 the walk-up gives the same result as before.
Expected updates in test-kotlin1:
- variables: local variable locations now span from val/var to initializer
- exprs, stmts, methods, modifiers, reflection: same local variable span
- controlflow/basic: CFG node references updated for new variable spans
(the "var ...;" node now starts at the val/var keyword)
Class member and top-level properties (IrProperty, not IrVariable) are
not affected by this change and retain their existing location behaviour.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
K1 IrProperty.startOffset includes leading modifiers (private, abstract, lateinit, annotations) in the span start; K2 already starts at val/var. Walk the PSI tree from p.startOffset to the enclosing KtProperty, then use valOrVarKeyword.startOffset as the declaration start, giving a consistent start in both K1 and K2. Two related but distinct locations are derived from the KtProperty: - The property itself spans val/var through the end of the full declaration (KtProperty.endOffset), including an explicit getter/setter body on a following line. This is getPsiBasedLocation(IrProperty). - Synthesised accessors (DEFAULT_PROPERTY_ACCESSOR origin) span val/var through the end of the property name (KtProperty.nameIdentifier.endOffset) via getPsiBasedAccessorLocation, applied through accessorOverride(). Explicit getter/setter bodies keep their own independently computed location. This makes K1 accessor locations match K2 and gives each synthesised accessor a precise span, rather than the property's full declaration span. Example (properties.kt line 3, "var modifiableInt = 1"): property modifiableInt -> 3:5:3:25 (val/var .. end of "= 1") accessor getModifiableInt -> 3:5:3:21 (val/var .. end of name) accessor setModifiableInt -> 3:5:3:21 Because accessor locations appear wherever accessors are reported, this refinement updates many expected files (property listings, modifiers, methods, reflection, control-flow and expression dumps). Every change is a location-coordinate change only: no result tuple is added or removed. The PSI-based location is restricted to unspecialised extractions (classTypeArgsIncludingOuterClasses.isNullOrEmpty()). Specialised generic instances (e.g. C<String>.prop) continue to use the binary whole-file location returned by getLocation(p, typeArgs), preserving the existing behaviour that keeps them absent from fromSource() queries. The visibility merge in extractFunction is extended to accept an overriddenAttributes parameter from the caller; the internal fake-override visibility adjustment (DescriptorVisibilities.PUBLIC for Java binary Object methods) is merged with any caller-supplied attributes so that neither overrides the other silently. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
andersfugmann
force-pushed
the
andersfugmann/kotlin-unify-tests
branch
from
06b0b1a to
327eedc
Compare
…guage versions
The local variable declaration expression (extractVariableExpr) computed its
location via getPsiBasedLocation(v as IrElement). The `as IrElement` cast forced
overload resolution to the generic getPsiBasedLocation(IrElement), which resolves
the PSI element straight from the IR element's raw source offsets via
PsiSourceManager.findPsiElement. Those offsets differ between the two frontend
language versions:
fun f(param: Int) {
val local1 = 2 + 3 // line 6: ` val local1 = 2 + 3`
}
- With -language-version 1.9 the IrVariable offsets cover only the name, so
findPsiElement returns the name leaf and the variable is located at 6:13:6:18.
- With -language-version 2.0 the IrVariable offsets cover the whole declaration,
so it is located at 6:9:6:26 (the `val` keyword through the initialiser).
The IrVariable-specific overload getPsiBasedLocation(IrVariable) is frontend
stable: it finds the leaf at the variable's start offset, walks up to the
enclosing KtVariableDeclaration and returns the span from the `val`/`var` keyword
to the end of the declaration. Using it for the entity location makes both
language versions emit the full declaration span 6:9:6:26.
This is the same helper already used for the enclosing localvariabledeclstmt
location; this change applies it to the variable entity (localvars / the
localvariabledeclexpr) as well, so the two are consistent.
Effect: 12 test-kotlin1 expected files move toward the test-kotlin2 output (all
strictly reduce the tk1-vs-tk2 difference; e.g. variables 16->4, reflection
111->67, exprs 1759->1363 diff lines). test-kotlin2 output is unchanged (the
overload produces the same span there as the generic one did). Both suites pass
all 3333 tests with --check-databases.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Also update the enhanced-nullability Kotlin integration test expected: its
local variable declaration expression now spans the val/var keyword to the
initialiser (user.kt:2:7:2:7 -> user.kt:2:3:2:16), consistent with the
library-test convergence.
…rsions Anchor a property backing field's location on its property declaration (the `val`/`var` keyword to the end of the declaration) rather than the raw IR offset. The raw `IrField` offset is inconsistent between frontends: under `-language-version 1.9` it includes leading modifiers in the span start, while under 2.0 it starts at the `val`/`var` keyword. Example: `private val privateProp: Int = 0` before (lang 1.9): properties.kt:35:5:35:32 | int privateProp; (col 5 = `private`) after (lang 1.9): properties.kt:35:13:35:32 | int privateProp; (col 13 = `val`) lang 2.0 (unchanged): properties.kt:35:13:35:32 | int privateProp; The property entity already uses this PSI-based anchor (getPsiBasedLocation), so the field now matches its own property location, which is what the 2.0 frontend already emits. Delegated properties are excluded via `isDelegated`: their field is the `$delegate` storage, whose location is the delegate expression rather than the property declaration, and is converged separately. This is a no-op for `-language-version 2.0` (only test-kotlin1 expected files change); the two suites' backing-field and field-type-access locations now agree. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
andersfugmann
force-pushed
the
andersfugmann/kotlin-unify-tests
branch
from
7e79e69 to
19774cc
Compare
…ve spans
Synthesised and bare `get`/`set` accessors were extracted with different
source locations depending on the frontend:
val typedProp: Int = 3 // getTypedProp
K1: 5:5:5:17 (val..name) K2: 5:5:5:22 (val..type)
val defaultGetter = 7
get // getDefaultGetter
K1: 19:5:19:21 (property head) K2: 20:13:20:15 (`get` keyword)
Under K2 the extractor has no PSI back-mapping for these accessors
(`getKtFile` returns null), so it cannot reproduce K1's property-name-end
span; K2 instead falls back to the raw IR offsets. Rather than converge on a
value K2 cannot produce, K1 is made to match the K2-native spans via the PSI:
* a bare `get`/`set` keyword now points at the keyword token; and
* a fully synthesised accessor now spans the property signature
(`val`/`var` .. type annotation, or .. name when untyped), excluding the
initialiser.
Explicit-body accessors (`get() = 5`) are unaffected: they are located at
their body and never take this override.
Only K1 output changes; the test-kotlin2 (K2) expected files are unchanged.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ative class span
The K1 and K2 frontends record different source locations for the
compiler-synthesised primary constructor of a class that declares no primary
constructor in source. K2 uses the class declaration's raw IR offsets, which
include any leading modifier keywords, while K1's raw offsets start at the
`class` keyword and omit the modifiers.
Since K1 (unlike K2) retains the PSI, we recover the modifier-inclusive span
from the enclosing KtClassOrObject so K1 matches the K2-native span.
Example, for `open class C0<V> {}` on line 11 (the `open` modifier is at
column 1, the `class` keyword at column 6):
before (K1): generics.kt:11:6:11:19 | C0 | C0()
after (K1): generics.kt:11:1:11:19 | C0 | C0() (matches K2)
The fix is deliberately narrow and leaves all other constructors untouched:
- explicit primary constructors (`class C1(val t: T)`, `class C2()`) keep
their own parameter-list location, which both frontends already agree on;
- explicit secondary constructors keep their own location (only the
`isPrimary` constructor is adjusted);
- specialised/parameterised copies of a generic constructor
(`typeSubstitution != null`) are excluded, so they do not gain a spurious
source location and appear in source-filtered queries.
Relearned test-kotlin1 (K1) and test-kotlin2 (K2): all 3333 tests pass with
database-consistency checks. Only test-kotlin1 expected files change (K2 output
is unchanged, as K2 already emits these spans natively). No previously matching
row diverges; net K1-vs-K2 divergence decreases on every affected file, with
generics, generic-inner-classes and modifiers now fully identical.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ty span For local and member delegated properties, the synthesised accessor (`<get-prop>`/`<set-prop>`) and its generated wrapper class/constructor were anchored by the K1 frontend at the delegate expression rather than at the property declaration. K2 (raw IR) anchors them at the property `val`/`var` keyword through the end of the delegate, i.e. the whole `KtProperty` span. Adopt the K2 span for both frontends so the extractor emits identical locations regardless of the supplied language version. Example: <get-prop1> 6:24:9:9 -> 6:9:9:9 Add `getPsiBasedDelegatedAccessorLocation`, which walks from the accessor's PSI up to the enclosing `KtProperty` and returns its span. It returns null when there is no PSI (K2) or no enclosing property, so K2 keeps its native locations and non-delegated callables are unaffected. Wire it into `extractFunction`'s location chain and into `extractGeneratedClass` (guarded on `DELEGATED_PROPERTY_ACCESSOR`) so the generated class/constructor sorts before the method, matching K2. Updates test-kotlin1 expected only (test-kotlin2 unchanged). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…s onto the delegate expression
The synthesised body of a delegated-property accessor (`get`/`getValue`/
`setValue`/`invoke` calls, the `<prop>$delegate` access, associated type
accesses and property-reference classes) carries the source range of the
whole `KtPropertyDelegate` node. The K1 frontend's range starts at the
`by` keyword; K2 starts at the delegate expression itself
(e.g. `lazy { ... }`), three columns later. The `by` keyword is syntactic
glue in the property declaration, not part of the expression being
evaluated, so K2's narrower range is the more intuitive one. Adopt it for
both frontends. Example:
get / getValue / invoke ... 6:24:9:9 -> 6:27:9:9
Add a scoped offset remap on `FileTrapWriter`: while extracting a
`DELEGATED_PROPERTY_ACCESSOR` body, any location whose offsets exactly
equal the `by`-inclusive delegate range is emitted with the delegate
expression's range instead. The range is recovered from the enclosing
`KtProperty`'s PSI (`delegate.expression`), which is available under K1;
under K2 there is no PSI so the remap is inactive and the raw offsets
already exclude `by`. Matching the full delegate range exactly means only
these synthesised body expressions are affected.
Updates test-kotlin1 expected only (test-kotlin2 unchanged).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The K1 frontend lowers `if`/`when` branches with their raw IR offsets collapsed onto the whole enclosing IrWhen expression, so every branch reports the same span (e.g. `stmts.kt:17:26:17:58`). K2 records per-branch spans reconstructed from the branch condition start through the result end (or just the result span for an `else` branch). Reconstruct the per-branch span from the branch's own condition/result offsets, gated on the raw branch span being collapsed onto the enclosing IrWhen (as under K1); under K2 the raw per-branch offsets already differ so the helper is a no-op. `correctedEndOffset` additionally fixes K1 recording a bare assignment's raw end at its left-hand side rather than past its right-hand value. Example (stmts.kt:17): before: 17:26:17:58 (x2, both branches collapsed onto the IrWhen) after: 17:29:17:43 and 17:50:17:58 (per-branch, matching K2) Only the K1 (test-kotlin1) expected files change; the K2 (test-kotlin2) expected files are unchanged. Controlflow expected files converge as a pure cascade of the branch-location shift. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The K1 frontend records an assignment's raw end offset at its left-hand side rather than past its right-hand value, so the ExprStmt wrapper synthesised around a bare assignment statement (`z = 4`) collapses onto the LHS and produces a zero-width span. K2 spans the whole assignment. Widen the ExprStmt wrapper location to end past the assigned value via a new `getExpressionStmtLocation` helper (reusing `correctedEndOffset`). This makes the wrapper match the inner `AssignExpr`, which already uses `e.startOffset .. rhsValue.endOffset`. The helper is a no-op for non-assignments and under K2, where the assignment already spans its value. Example (stmts.kt:17): before: 17:37:17:37 (collapsed onto LHS `z`) after: 17:37:17:41 (spans `z = 4`, matching K2) Only the K1 (test-kotlin1) expected files change; the K2 (test-kotlin2) expected files are unchanged. Controlflow expected files converge as a pure cascade of the location shift (dominator.expected fully converges). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… assigned value)
For an indexed array assignment `a[i] = v` (and its compound forms `a[i] op= v`),
the K1 and K2 frontends disagree on the source location of both the synthesised
array-access node (`...[...]`) and the assignment node (`...=...`/`...op=...`):
- K1 records the end offset at the left-hand side array access, e.g.
`a1[0] = a1[0]` gets `12:3:12:7` (just `a1[0]`).
- K2 spans through the assigned value, e.g. `12:3:12:15` (the whole `a1[0] = a1[0]`).
Decision: adopt the K2 span. An assignment expression should cover its whole
source range (left-hand side through the right-hand value), consistent with how
ordinary (non-indexed) assignments are already located and with the Java
extractor's array-store locations. This makes location-based queries (and any
`...=...`/`...[...]` overlap reasoning) behave the same regardless of frontend.
Implementation: the set operation is desugared to an array `set` call. Two code
paths build these nodes, and both used the raw call/block end offset:
- simple `a[i] = v` -> the `Array.set` IrCall branch in `extractCall`
- compound `a[i] op= v` -> `tryExtractArrayUpdate`
Each now widens the end offset to the assigned value's end via a small offset-based
`correctedEndOffset(rawEnd, valueEnd)` helper (extracted from the existing
`correctedEndOffset(IrExpression)` so the two share one rule). The widening only
fires when the value's end lies past the raw end, so it is a no-op under K2 (where
the raw end already spans the value) and ignores undefined/synthetic offsets. A new
`TrapWriter.getLocation(e, endOffset)` overload preserves the existing IrCall
start-offset adjustment while overriding just the end.
Tradeoff: the change is deliberately scoped to the two array-assignment paths
rather than globally rewriting how set-call locations are derived, to avoid
perturbing unrelated expressions. Only the `test-kotlin1` array expected files
change; `test-kotlin2` is unaffected, and the two suites' `arrayAccesses` and
`assignExprs` expected files are now byte-identical. All 3333 tests pass in both
K1 and K2 modes (with database-consistency checks).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This unifies the Kotlin test suites by moving the test-kotlin1/test-kotlin2 contents into a single test-kotlin suite.
What changed:
java/ql/test-kotlin/and removed the splittest-kotlin1/test-kotlin2layout.Review notes: