Unify Kotlin test suites#22135
Draft
andersfugmann wants to merge 47 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>
…onto K2
Follow-up to the member/top-level delegated-accessor convergence, covering the
remaining case: a LOCAL delegated property (declared inside a function body,
e.g. `val x: Int by DelegateProvider()`).
Unlike member/top-level delegated properties, a local delegated property's
forwarding get/set is inlined into the enclosing function rather than
materialised as a `DELEGATED_PROPERTY_ACCESSOR`, so the previous fix
(getDelegatedAccessorSyntheticArgumentLocation, gated on that origin) did not
apply. Because a local property has no dispatch/extension receiver, its
synthetic `thisRef` argument (passed to the delegate's
`provideDelegate`/`getValue`/`setValue`) is always a `null` constant with no
source token.
Under K1 this synthetic `null` was given a bogus location `1:9:1:12`: its IR
offsets (8..11) are not real source offsets and `findPsiElement` resolves them
to the file's line-1 `import`. K2 records it at the whole-file location
`0:0:0:0`, which is the honest representation for an argument with no source
token, and is the canonical target for this unification.
This change adds `getLocalDelegatedPropertySyntheticNullLocation`, gated on:
- a scoped context (`currentLocalDelegatedProperty`) that is set only while
extracting a specific `IrLocalDelegatedProperty`'s generated artifacts
(its delegate initializer and forwarding get/set),
- the element being a `null` `CodeQLIrConst`, and
- the element offsets lying outside the property's `KtProperty` text range
(so a genuine source `null` inside the delegate expression, e.g.
`by foo(null)`, is never relocated).
It returns the whole-file location, mirroring K2. Per the design review, this
is deliberately a distinct, narrowly-scoped path anchored on the local
delegated property being extracted, NOT a relaxation of the accessor-origin
gate to arbitrary enclosing functions (which would risk moving genuine source
`null`/`this` literals). It relies on PSI, so it is a no-op under K2.
Verified via full dual-suite relearn (CI-faithful: 2.3.20/lang-1.9 for tk1,
default/2.4.0 for tk2, database consistency checks): all 3333 tests pass. Only
two tk1 files change (exprs.expected and its PrintAst), each a single row
relocating the synthetic `null` from `1:9:1:12` to `0:0:0:0`; the NullLiteral
rows now match test-kotlin2 exactly, no `1:9:1:12` bogus locations remain
anywhere in test-kotlin1, and test-kotlin2 is byte-for-byte unchanged.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
An explicit property setter written `set(_)` produces a value parameter
whose source spelling is the Kotlin discard placeholder `_`. The two
frontends model this differently:
* K2 preserves the source name and exposes the parameter as `_`.
* K1 additionally flags the parameter with
`IrDeclarationOrigin.UNDERSCORE_PARAMETER`, which the extractor treats
as a synthesized name (no name is written and QL synthesizes `p0`).
This made `test-kotlin1` report `p0` while `test-kotlin2` reported `_`
for the same source (library-tests/underscore-parameters).
The `UNDERSCORE_PARAMETER` origin is *also* set by K1 (and, for the
synthesized cases, by K2) on discarded parameters that do not retain a
`_` source spelling, most importantly:
* lambda placeholder parameters, e.g. `{ index, _ -> ... }`
* the synthesized `invoke` parameters of function types
(funcExprs.kt lines 27/30/31/90/94)
For those, the two frontends assign *different* internal names
(`<anonymous parameter N>` under K1 vs `<unused var>` under K2), so the
synthetic-name treatment is exactly what keeps them converged at `pN`.
Removing the origin check wholesale therefore replaces one small
divergence with a much larger one.
To converge only the case both frontends agree on, we suppress the
synthetic-name treatment only when the parameter's own name is exactly
`_` (the source discard spelling that both K1 and K2 preserve). This
makes K1 emit `_` for `set(_)` (matching K2) while leaving every other
discarded parameter synthetic in both suites.
Scope of the change (verified by a full dual-suite `--learn` with
database-consistency checks, all 3333 tests passing):
* test-kotlin1 library-tests/underscore-parameters: `p0` -> `_`
(now identical to test-kotlin2).
* test-kotlin2: byte-for-byte unchanged.
* No other expected file changes (lambda / function-type discard
parameters remain `pN` in both suites).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
An exhaustive `when` expression (one with no explicit `else` that the
compiler proves total) gets a synthetic `throw NoWhenBranchMatchedException(...)`
as its fallback branch. The two frontends locate that synthetic call
differently:
* K2 gives the call the source offsets of the enclosing `when`
expression, so the throw (and the `new NoWhenBranchMatchedException`)
are located at the `when` block, e.g. `6:3:9:3`.
* K1 leaves the synthetic call with undefined offsets, so the extractor
emits a `0:0:0:0` (no-source) location for both nodes.
A real source location is strictly more useful than none, and anchoring
the implicit fallback to the `when` it belongs to is the intuitive
choice, so we adopt the K2 behaviour for both frontends.
`extractCall` now records the enclosing `when`'s location while
extracting its branches (`currentSyntheticWhenLocation`) and uses it as
the fallback for the `noWhenBranchMatchedException` builtin whenever the
synthetic call itself has undefined offsets. This only changes K1: under
K2 the call already carries valid offsets, so `tw.getLocation(c)` is used
unchanged and K2 output is byte-identical.
Relearned both suites: all 3333 tests pass and the only changed row is
test-kotlin1/library-tests/no-when-branch-found, which now matches
test-kotlin2 exactly (`0:0:0:0` -> `6:3:9:3`).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
A data class's generated `copy(...)` has one value parameter per
primary-constructor property. The K1 frontend records each such
parameter (and its type accesses) at the source location of the
corresponding property; the K2 frontend leaves them with undefined
offsets, which the extractor emits as a `0:0:0:0` location.
This divergence is purely a K2 information regression: the richer K1
location is unambiguously better (it points at the real property in
source, enabling location-based queries), so we converge K2 onto K1
rather than the other way around.
Because K2 exposes no PSI back-mapping, the location cannot be
recomputed from source; instead we recover it from the IR. For a value
parameter of a `GENERATED_DATA_CLASS_MEMBER` function whose own offsets
are undefined, we look up the primary-constructor parameter at the same
index and reuse its location.
Guards keep the change surgical:
- `vp.startOffset >= 0` bails out, so K1 (which already has real
offsets) is untouched.
- the origin must be `GENERATED_DATA_CLASS_MEMBER`.
- the primary-ctor parameter name must match and carry real offsets,
which restricts the remap to `copy`-style parameters and excludes
members such as `equals(other)`.
Relearned both suites: only data-class `copy` parameter rows change
(K2 now matches K1). data-classes/PrintAst.expected becomes byte
-identical across suites; the residual diffs in methods/{exprs,
parameters}.expected are pre-existing, unrelated divergences.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
An explicit property accessor may carry its own annotations, for example:
val x: Int
@JvmName("getX_prop")
get() = 15
The K2 frontend records such an accessor with raw IR offsets that begin
at the leading annotation; the K1 frontend's raw offsets start at the
`get`/`set` keyword and omit the annotation. This is a pure K1
information regression: the annotation is part of the accessor
declaration and the K2 span is the more faithful one, so we converge K1
onto K2.
Because the annotation-inclusive start cannot be reconstructed under K2
(no PSI back-mapping) but is trivially available under K1, we recover it
from the KtPropertyAccessor PSI node, whose text range begins at its
modifier list. A new helper getPsiBasedAnnotatedAccessorLocation returns
this span, and accessorOverride now applies it to explicit accessors (in
addition to the existing synthesised-accessor handling).
Guards keep the change surgical:
- returns null under K2 (getKtFile unavailable; raw offsets already
include the annotation), leaving K2 untouched.
- returns null when the accessor declares no annotations of its own, so
non-annotated explicit accessors (which already converge) are
unaffected.
Relearned both suites: only explicit annotated-accessor declaration rows
change (K1 now matches K2). annotations/jvmName/test.expected becomes
byte-identical across suites; the residual diffs in jvmstatic-annotation
are pre-existing, unrelated divergences (JVM-static proxy forwarder
locations and call-argument spans).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
A primary constructor's call to its superclass is written in source only
as a supertype-list entry (for example `class C : Base()`), so the
synthetic delegating super-constructor call has no `super(...)` token of
its own. The K1 frontend records it at that supertype call expression
(`12:23:12:34`); the K2 frontend records it at the whole class
declaration (`12:1:15:1`).
Neither is a real `super(...)` statement. Consistent with the location
policy adopted for these synthetic constructs (prefer the fuller
whole-construct span), we converge K1 onto the K2 form.
Because the whole-class span cannot be reconstructed under K2 (no PSI
back-mapping) but K2 already emits it from raw IR, the fix is K1-only: a
new helper getPsiBasedPrimaryCtorSuperCallLocation returns the enclosing
KtClassOrObject span (including leading modifiers such as `open`) for the
super call of a primary constructor, and the IrDelegatingConstructorCall
handler uses it for super calls (delegatingClass != currentClass).
Guards keep the change surgical:
- applies to primary constructors only (both explicit `()` and fully
implicit); an explicit `super(...)` in a secondary constructor keeps
its own location, which both frontends already record identically.
- returns null under K2 (getKtFile unavailable; raw offsets already
carry the class span), leaving K2 untouched.
Relearned both suites: only primary-ctor super-call rows change (K1 now
matches K2). classes/ctorCalls.expected becomes byte-identical across
suites; the residual vararg/args diff is a pre-existing, unrelated
`public vararg val` parameter-span divergence.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
When a constructor carries leading modifiers or annotations, for example
`internal constructor(...) { }` or an annotated `@Ann constructor()`,
the K1 frontend's IrBlockBody offsets for the constructor body begin at
the modifier/annotation (`3:3`); the K2 frontend begins at the
`constructor` keyword (`3:12`), consistent with how declarations exclude
leading modifiers from their own span. We converge K1 onto the K2 form.
The K2 span cannot be reconstructed from raw offsets under K1, but the
`constructor` keyword position is available from the PSI. A new helper
getPsiBasedConstructorBodyLocation walks from the block body's start to
the enclosing KtConstructor and returns a location from its
`constructor` keyword through the block body's own end offset;
extractBlockBody uses it in preference to the raw block location.
Guards keep the change surgical:
- returns null under K2 (getKtFile unavailable; raw offsets already
exclude the modifier), leaving K2 untouched.
- returns null for non-constructor bodies and for an implicit primary
constructor with no `constructor` keyword.
- for a constructor without modifiers the keyword coincides with the
block start, so the location is unchanged there.
Relearned both suites: only modifier/annotation-carrying constructor
body-block rows change (K1 now matches K2).
internal-constructor-called-from-java/test.expected becomes
byte-identical across suites; the residual annotation_classes/PrintAst
diffs are pre-existing, unrelated divergences (annotation-argument and
stdlib enum-entry locations).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
A source underscore variable (an unused `_` in a `catch (_: E)` clause or a `val _ = ...` discard) is named differently by the two frontends: - K1 keeps the source spelling `_`. - K2 assigns the synthetic `SpecialNames.UNDERSCORE_FOR_UNUSED_VAR`, which renders as `<unused var>`. Decision (D15): adopt the K1 behaviour. `_` is the actual source token, so it is the more intuitive and source-faithful name; it also keeps the local variable name consistent with the corresponding value-parameter case, where a prior fix already normalises the underscore setter parameter to `_`. `extractVariableExpr` now maps a variable whose IR name is `SpecialNames.UNDERSCORE_FOR_UNUSED_VAR` to `_` before writing `localvars`. The check is on the special name rather than a raw string so it is robust across compiler versions, and it only fires for the frontend-synthesised unused-variable name, leaving all other locals untouched. Full dual-suite relearn: all 3333 tests pass. The only changed expected row is in query-tests/UnderscoreIdentifier, where the K2 catch parameter row converges from `Exception <unused var>` to `Exception _`, matching K1. The remaining divergence in that file (the destructuring container `<destruct>` vs `tmp0_container`) is a separate naming/location issue tracked under C14. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…h frontends A prefix/postfix increment or decrement (`x++`, `--x`, ...) is desugared into a block holding an induction temporary. The two frontends name that temporary differently: - K1 uses a per-scope counter, `tmp0`, `tmp1`, ... `tmpN`. - K2 uses the uniform special name `<unary>`. Decision: adopt the K2 name (`<unary>`) from both frontends. This is the only robust convergence direction: K1's counter-based `tmpN` numbering depends on the frontend's temporary allocation order and cannot be reproduced faithfully under K2 (mapping `<unary>` back to a single `tmp0` would wrongly collapse distinct temporaries such as `tmp0`..`tmp7` in one function). The uniform `<unary>` name is frontend-independent and identifies the construct just as well. (This supersedes the earlier D14 preference for the readable `tmpN` names, which was made before the numbering infeasibility was established.) Implementation: `extractBlock` recognises an `IrContainerExpression` whose origin is one of PREFIX_INCR / PREFIX_DECR / POSTFIX_INCR / POSTFIX_DECR and records its first statement (the induction temporary) together with the canonical name `<unary>`. `extractVariableExpr` emits that name for exactly that variable (matched by identity), leaving all other locals untouched. The recording is saved/restored around the block's statement extraction so nested blocks behave correctly. The detection is origin-based and therefore identical for K1 and K2. Full dual-suite relearn: all 3333 tests pass. The only changed rows are in test-kotlin1 (exprs/exprs, exprs/PrintAst, controlflow/basic/bbStmts and getASuccessor), where the K1 increment temporaries converge from `tmpN` to `<unary>`, matching K2. Remaining exprs.kt differences (unary-operand span, and the delegatedProperties `<set-?>`/`<get-x>` cluster) are separate location/naming items tracked elsewhere. The destructuring container temp (`tmp0_container` vs `<destruct>`) is handled separately in C14b. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…th frontends A destructuring declaration `val (a, b) = subject` introduces a temporary that holds the subject; each component is then read from it via `componentN()`. The two frontends represent that temporary differently: - K1 names it `tmp<N>_container` and locates it at the subject expression only (e.g. `7:26:7:26`, pointing at `p`). - K2 names it `<destruct>` and locates it across the whole destructuring declaration (e.g. `7:9:7:26`, `val (first, _) = p`). Decision: adopt the K2 representation from both frontends. The `<destruct>` name is uniform and frontend-independent (K1's `tmp<N>` counter cannot be reproduced under K2, consistent with C14a), and the full-declaration span is more informative than a bare pointer at the subject. Implementation: `isDestructuringContainerVariable` recognises the temporary by its frontend name (`<destruct>` under K2, `tmp<N>_container` under K1). `extractVariableExpr` then emits the name `<destruct>` and, under K1 only, a PSI-based location spanning the enclosing `KtDestructuringDeclaration` (`getPsiBasedDestructuringContainerLocation`); under K2 the IR offsets are already correct so the helper returns null and the existing location is kept. Full dual-suite relearn: all 3333 tests pass. The only changed row is in query-tests/UnderscoreIdentifier, where the K1 container converges from `7:26:7:26 | tmp0_container` to `7:9:7:26 | <destruct>`, matching K2. With this and C15, query-tests/UnderscoreIdentifier is now byte-identical across both suites. The for-loop destructuring shape (`for ((v, i) in ...)`, where K1 omits the container entirely) is a separate AST-shape difference tracked as C14c. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The Kotlin internal queries job (odasa-buildutils/kotlin_internal_queries)
flagged two code-quality issues introduced by the K1/K2 convergence helpers.
Both are pure source refactors with no effect on extractor output, so no
.expected files change.
1. possiblyThrowingExpressions.ql / notNullExpr: extractVariableExpr used a
`!!` not-null assertion (`currentDesugarTemp!!.second`) guarded by a
separate `currentDesugarTemp?.first === v` check. The extractor must avoid
`!!` (it can throw and lose a source file). Bind `currentDesugarTemp` to a
local val and null-check it in the `when`, which smart-casts the subsequent
`.first`/`.second` accesses. Behaviour is identical.
2. separated_overloads.ql: two overload groups were split by newly added
helpers, which the lint reports as harder-to-read code:
- `getPsiBasedConstructorBodyLocation` sat between the two
`extractBlockBody` overloads; moved it below both.
- the destructuring helpers (`destructuringContainerK1NameRegex`,
`isDestructuringContainerVariable`,
`getPsiBasedDestructuringContainerLocation`) sat between the
`getPsiBasedLocation(IrVariable)` and `getPsiBasedLocation(IrProperty)`
overloads; moved them below the third overload so all three
`getPsiBasedLocation` overloads are adjacent.
Verified the standalone extractor still compiles under both K2 (2.4.0) and
K1 (1.9.20-Beta).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…cation
A primary-constructor property parameter written with leading modifiers,
e.g. `public vararg val s: String`, is located differently by the two
frontend paths:
- K1 (-language-version 1.9) starts the parameter location at the first
modifier token (`public`), giving test.kt:50:5:50:31.
- K2 (default) starts it at the `val`/`var` keyword, giving
test.kt:50:19:50:31.
The K2 span is the more intuitive and consistent one: every other value
parameter is already located from its `val`/`var` keyword (or its name),
so including the leading modifier list here is an outlier that also makes
the property parameter's span inconsistent with the property it backs.
Converge K1 onto the K2 span. `getPsiBasedValueParameterLocation` finds
the enclosing `KtParameter` via PSI back-mapping (available under K1,
where `getKtFile` is non-null; it returns null under K2, which already
emits the desired offsets) and, only when modifiers precede the keyword,
re-anchors the location start at the `val`/`var` keyword while preserving
the parameter's own end offset.
The guard `keyword.startOffset <= vp.startOffset` is essential: when the
`val`/`var` keyword already is the parameter start (no leading modifiers),
the helper must not fire, otherwise it would rewrite the end offset to
`vp.endOffset` and diverge from the raw location for ordinary property
parameters (observed as orphaned `[Parameter]` rows in generics/
reflection/classes PrintAst during development).
After this change test-kotlin1 and test-kotlin2 vararg/args.expected are
byte-identical. All 3333 tests pass in both suites (K1 2.3.20 / lang 1.9
and K2 2.4.0 / lang 2.0).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
For a delegated property `var x: Int by ResourceDelegate()` the two
frontend paths anchor the synthesised delegate locations differently:
- K1 (-language-version 1.9) starts them at the `by` keyword.
- K2 (default) starts them at the delegate expression itself
(e.g. `ResourceDelegate()`), three columns later (`by ` excluded).
The `by` keyword is syntactic glue joining the property to its delegate,
not part of the expression that is evaluated and stored, so K2's narrower
range is the more intuitive one. An existing remap already converged the
delegated-accessor *body* expressions onto the K2 span; this commit
converges the two remaining sites.
1. The `$delegate` backing field. `getDelegateFieldLocation` recovers the
delegate expression's range from the enclosing `KtProperty` via PSI
(available under K1; null under K2, where the raw offsets already
exclude `by`) and is used both for the field's declaration location
(extractField) and for its initializer assignment / lhs access
(extractFieldInitializer's declLocId). It fires only for a delegated
property's own backing field (`isDelegated` and `backingField === f`).
2. Local delegated properties. The `provideDelegate(...)` call and its
`KProperty` argument in the delegate variable's initializer span the
whole `by <expr>` range under K1; the `IrLocalDelegatedProperty`
branch now applies the same `scopedOffsetRemap` used for accessor
bodies while extracting that variable.
The shared PSI walk is factored into `getEnclosingKtProperty` and a
`KtProperty`-keyed `getDelegateExpressionOffsetRemap` overload.
Result: test-kotlin1 and test-kotlin2 exprs/delegatedProperties.expected
and exprs/funcExprs.expected are now byte-identical. exprs/exprs.expected
and methods/exprs.expected have their delegate rows converged (remaining
diffs in those two files are unrelated mechanisms: implicit-cast nodes and
the property-type / setter-parameter spans, addressed separately). All
3333 tests pass in both suites (K1 2.3.20 / lang 1.9 and K2 2.4.0 /
lang 2.0).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
andersfugmann
force-pushed
the
andersfugmann/kotlin-unify-tests
branch
from
915d4d4 to
7b1da21
Compare
…g declaration
Two compiler-synthesised value parameters that carry no source token of their
own were located differently by the K1 and K2 frontends:
- the `<set-?>` parameter of a delegated property's synthesised setter: K1
anchored it at the delegate expression (`delegates.kt:8:32:11:5`) while K2
spans the whole property declaration (`8:5:11:5`); and
- the `value` parameter of the synthetic enum `valueOf` member: K1 has no
offsets and emitted the null `0:0:0:0` location while K2 attributes it to
the enum-class declaration (`1:1:4:1`).
In both cases the parameter is synthesised, so the K2 span (the owning
declaration) is a real, navigable location and strictly more useful than either
the delegate-expression fragment or K1's null `0:0`. We therefore converge K1
onto the K2-native span, consistent with how the other synthesised members
(accessors, implicit constructors, super calls, `$delegate` fields) are already
being anchored on their owning declaration.
`getPsiBasedSyntheticParameterLocation` recovers the span from the PSI under K1
and returns null under K2 (where `getKtFile` is unavailable and the raw offsets
already carry the owning-declaration span) and for every ordinary, source-backed
parameter, so the canonical K2 output is untouched.
Relearn (both suites, CPUS=5): all 3333 tests pass. Only test-kotlin1 changes;
methods/parameters is now byte-identical across suites (6 -> 0 divergent rows),
and the same setter/valueOf parameter rows converge in the PrintAst/exprs
projections that reference them (annotation_classes/PrintAst 88->84,
classes/PrintAst 18->10, exprs/PrintAst 75->67, exprs/exprs 189->171,
exprs_typeaccess/PrintAst 9->5, methods/exprs 36->30). Every touched file's
divergence strictly decreases; no new divergence is introduced.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ocation
For `class C : Intf by <expr>` the compiler synthesises a `$$delegate_0` field
that stores the delegate `<expr>`, so its natural location is that expression.
The two frontends disagreed on the field's own location:
- K1 records it at the delegate expression (`intfDelegate.kt:7:26:9:1`, the
`object : Intf { ... }`); while
- K2 records it from the delegated supertype, so it includes the `Intf by `
glue (`7:18:9:1`).
This is the interface-delegation analogue of the delegated-property `$delegate`
field, where we already adopted the delegate-expression span and excluded the
`by` keyword (it is syntactic glue, not part of the stored expression). We apply
the same principle here and converge K2 onto K1's narrower span.
`getClassDelegateFieldLocation` anchors a class-delegation field
(`IrDeclarationOrigin.DELEGATE`, which has no corresponding property and would
otherwise fall through to the raw IR offset) on its own initialiser expression.
It uses the initialiser's raw offsets, which are available under both frontends
(`7:26:9:1` in each), so it is frontend-stable: under K1 it reproduces the offset
already recorded (no change) and under K2 it trims the leading `Intf by `.
Relearn (both suites, CPUS=5): all 3333 tests pass. Only test-kotlin2 changes;
interface-delegate/test is now byte-identical across suites (2 -> 0 divergent
rows). No other file changes.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The synthetic setter of a *delegated* property has one implicit value
parameter. The K2 frontend names it `<set-?>`
(`SpecialNames.IMPLICIT_SETTER_PARAMETER`), matching what it does for an
ordinary member property's setter. The K1 frontend instead names a
delegated property's setter parameter `value`, diverging from both K2 and
from its own naming of member-property setters.
This produced two divergent rows per delegated `var` between
test-kotlin1 and test-kotlin2 (the `[Parameter]` declaration and the
`[VarAccess]` that reads it back inside the generated setter body):
- [Parameter] value -> [Parameter] <set-?>
- [VarAccess] value -> [VarAccess] <set-?>
Decision: adopt the K2 name. `<set-?>` is the canonical implicit
setter-parameter name the frontend already uses everywhere else, so it is
the more consistent and less surprising choice; a bare `value` is
indistinguishable from a user-written parameter of that name.
`getConvergedValueParameterName` renames the parameter only when its
enclosing function has origin `DELEGATED_PROPERTY_ACCESSOR` *and* its K1
source name is `value`. The origin guard restricts the rename to the fully
synthetic delegated accessors (a hand-written `ReadWriteProperty.setValue`
has origin `DEFINED`, so its `value` parameter is untouched); the name
guard restricts it to the setter value parameter, leaving the accessor's
receiver parameters (`<this>` for an extension property,
`<dispatchReceiver>` for a member property) unchanged. Under K2 the setter
parameter is already `<set-?>` and receivers are `<this>`, so nothing
matches and the canonical K2 output is byte-for-byte unchanged (only the
three K1 setters in test-kotlin1 relearn).
The literal `<set-?>` is used because `SpecialNames.IMPLICIT_SETTER_PARAMETER`
is not resolvable in every compiler version the extractor is built against.
Relearned both suites (all 3333 tests pass); only the three delegated
setters in test-kotlin1 (delegatedProperties.kt lines 19, 34, 82) change.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ure span For a property such as `val prop: Int = 1`, the compiler generates synthetic `this.prop` accesses (inside the default getter and the primary-constructor field initialiser). The K1 and K2 frontends give these synthetic accesses different source spans: * K1 runs the span through the initialiser: `variables.kt:3:5:3:21` * K2 stops at the end of the type signature: `variables.kt:3:5:3:17` The property *declaration* location is `3:21` under both frontends and is left untouched; only these synthetic *access* expressions diverged. The K2 signature span (val/var keyword to the end of the type, or to the end of the name when the type is inferred) is the more intuitive location: it points at the property's signature rather than incidentally swallowing the initialiser expression, so a `this.prop` read is not reported as spanning code it does not evaluate. We adopt the K2 span and reproduce it under K1 from PSI. New helper `getPsiBasedPropertySignatureAccessLocation` returns the signature span only when the access resolves (via `findPsiElement`) to the enclosing `KtProperty`, which is true exactly for these synthetic property-declaration-anchored accesses. It returns null under K2 (no PSI is available there, and the raw IR offset already gives the signature span) and for every ordinary source-written access (whose PSI is the reference expression, not the whole declaration), so canonical K2 output and normal accesses are unaffected. It is wired into the `IrGetField` read path and into `extractThisAccess`. Effect: tk1 `variables/variables.expected` becomes byte-identical to tk2; `variables/variableAccesses.expected` drops to only the separate implicit-`this` call-receiver span divergence; genericExprTypes/exprs/methods synthetic property-access rows all move from the through-initialiser span onto the K2 signature span. All 3333 tests pass in both suites; test-kotlin2 is unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The `enum` library test used two different queries in test-kotlin1 and
test-kotlin2, so their `.expected` files were not comparable (78 divergent lines
that reflected the query difference, not an extractor difference):
* test-kotlin1: `from Method m, RefType t where t = m.getDeclaringType() and
t.getName() = ["Enum", "Enum<?>", "Enum<E>", "EnumUserKt"]
select t.getQualifiedName(), t.getName(), m.getName()`
* test-kotlin2: `from Method m where m.getDeclaringType().getName().matches("Enum%")
select m.getName()`
The K1 query is the more precise of the two: it pins the declaring types by exact
name and reports the qualified name alongside the method, whereas the K2
`matches("Enum%")` form also pulls in unrelated `java.util.EnumSet` members as
noise. We adopt the K1 query in both suites (a pure test-input synchronisation,
no extractor change).
After unification the two suites' output is identical except for a single row:
test-kotlin2 additionally reports `kotlin.Enum | Enum | finalize`. That is a
compiler-builtin difference (the Kotlin `kotlin.Enum` built-in class exposes
`finalize` under the 2.4.0/K2 frontend but not under the 2.3.20/K1 frontend); it
is a compiler-version artifact of the built-in class member set, not something the
extractor synthesises, so it is tracked separately rather than papered over here.
All 3333 tests pass in both suites; test-kotlin1 is unchanged.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The comment extractor has two implementations selected at runtime: the PSI-based `CommentExtractorPSI` (used when the IR is built from PSI, i.e. the K1 frontend) and `CommentExtractorLighterAST` (used under the K2/FIR frontend, where no PSI is available and comments are recovered from the FIR lighter AST). The PSI extractor writes a row per KDoc section (`comment.getAllSections()`: the default section plus `@property`, `@constructor`, ... tag sections), but the lighter-AST extractor did not, because the KDOC node in the FIR lighter AST is a leaf: its section/tag structure is never expanded there. As a result, `ktCommentSections`, `ktCommentSectionNames` and `ktCommentSectionSubjectNames` were entirely absent under K2, producing a large divergence from the K1 output. KDoc section structure can only be recovered by parsing the KDoc text into real PSI, which needs a `Project`. The compiler passes one to the component registrar's `registerProjectComponents`, so we capture it there into `KDocProjectHolder` (a process-global; a weak reference guarded by `isDisposed` avoids keeping a disposed project alive). The lighter-AST extractor then re-parses each KDoc's text with `KtPsiFactory` and reads `getAllSections()`, writing exactly the same rows as the PSI extractor. Because both paths delegate to the same compiler KDoc parser, the section content, names and subject names are reproduced identically. Trade-offs: - A process-global project holder is used rather than threading the project through the extension/extractor constructors, which would touch a lot of unrelated wiring. Each CodeQL extraction is a single compiler invocation, so one project per process is a safe assumption; the weak reference and `isDisposed` check bound the lifetime risk. - The re-parse forces the KDoc text into a doc-comment position via a throwaway trailing declaration. A KDoc is only recognised as a doc comment when it precedes a declaration; the appended declaration is inert and does not affect section parsing. - Section output is byte-identical between K1 (2.3.20) and K2 (2.4.0) today because both use the compiler's own KDoc parser. This is a version-coupled compatibility shim rather than a guaranteed invariant; the shared test suite will catch any future drift. Only the K2 expectations gain rows; the K1 output is unchanged. The `test-kotlin2` comment section relations now match `test-kotlin1` byte-for-byte, except for the line-1 KDoc whose source text still differs between the two suites (addressed separately). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
A KDoc written before a file's `package` directive documents the file itself and has no declaration owner: the PSI extractor's `comment.owner` is null for it. The extractor previously emitted a "Couldn't get owner of KDoc" diagnostic and recorded no owner in that case. The K2/FIR extractor, by contrast, already attributes such a comment to the compilation unit, because the KDOC node is a direct child of the FILE node in the FIR lighter AST and the enclosing `IrFile` is discovered as its owner. The two frontends therefore disagreed on file-level KDoc ownership. Align the PSI extractor with the FIR one: when a KDoc has no declaration owner but is a direct child of the `KtFile`, attribute it to the file (`getLabel(file)` yields the compilation-unit label, the same target the FIR extractor reaches). The now-spurious "Couldn't get owner" diagnostic is suppressed for file-level KDoc (it remains for any genuinely ownerless KDoc elsewhere). This removes the last owner-attribution divergence for file-level KDoc. The `comments` and `dataflow/func` test sources, which previously carried frontend-specific text and a `// Diagnostic Matches` line asserting the old warning, are unified with their K2 counterparts; the K1 expectations lose the diagnostic and gain the compilation-unit owner row, matching K2. Remaining `comments` divergence (owners of enum entries, init blocks and an anonymous function) stems from those IR nodes carrying no FIR source metadata under K2 and is addressed separately. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Under the K2/FIR frontend the lighter-AST comment extractor finds a KDoc's owner by walking the IR and, for each element that carries FIR metadata, checking whether that metadata's source node has a KDOC child. Enum entries (`IrEnumEntry`) and anonymous initializers (`IrAnonymousInitializer`) carry no FIR metadata, so the search could never reach them: their KDoc comments were recorded with no owner, whereas the PSI frontend attributes them (an enum entry to itself, an init block to its enclosing class). These IR declarations do retain valid source offsets, however. This adds a supplementary pass that walks the file's lighter AST, finds the `ENUM_ENTRY` and `CLASS_INITIALIZER` nodes that carry a KDOC child, and matches each to the metadata-less IR declaration whose source offset lies within the node's range. Matching is fail-closed: a node is only attributed when exactly one candidate declaration falls inside it, so ambiguous or unexpected shapes leave the comment ownerless rather than guessing. Offset containment is used rather than per-kind heuristics (matching enum entries by name, initializers by order) because it is uniform across both node kinds and does not depend on names being unique or declaration order being stable. Only the K2 expectations change: the `test-kotlin2` `comments` enum-entry and init-block KDoc owners now match `test-kotlin1` exactly. The one remaining owner difference in that test is a KDoc on an anonymous function passed as a call argument, which the PSI frontend attributes to the enclosing property via its owner walk-up and the FIR frontend does not; that discrepancy is left as-is (see the updated TODO). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ignment For a desugared in-place update such as `v += e` the extractor emits an `AssignAddExpr` (etc.) whose left-hand side is a `VarAccess` of `v`. The location of that `VarAccess` was taken from the underlying `IrSetValue` node (`getLocation(e)`). The two frontends record that node's end offset differently: K1 ends it at the left-hand side (so the access spanned just `v`), whereas K2 ends it past the whole assignment (so the access spanned all of `v += e`, redundantly identical to its parent `AssignAddExpr`). The K1 span is the more intuitive and information-preserving one: a variable access should point at the variable reference, not repeat the enclosing assignment's span. Converge K2 onto it. The desugaring represents `v += e` as `v = get(v).op(e)`, so the update's right-hand call already contains an `IrGetValue` receiver that reads `v`. That receiver's source span is exactly the `v` identifier in both frontends, which makes it a frontend-independent anchor for the LHS location. `getUpdateInPlaceReceiver` recovers it (mirroring the existing `getUpdateInPlaceRHS`), and the LHS `VarAccess` is located there. This is fail-closed: when the node is not such an in-place update, or the receiver lacks a usable (non-synthetic, defined) source span, the raw `getLocation(e)` is kept. Under K1 the recovered span equals the previous one, so K1 output is unchanged. Only the five in-place operators in the `test-kotlin2` `exprs` test change, each narrowing the LHS `updated` `VarAccess` from `270:3:270:14` (the whole `updated += 1`) to `270:3:270:9` (the identifier), matching `test-kotlin1` exactly. Prefix/postfix increment/decrement use a different desugaring (with a temporary) and are not in-place updates in this sense; they are left for a separate change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The KDoc-section parser added for the K2/FIR frontend caught `com.intellij.openapi.progress.ProcessCanceledException` by type in order to rethrow it (a control-flow exception that must never be swallowed) before the general `catch (Exception)` fallback. That compile-time type reference is not resolvable on every supported Kotlin compiler version: the IntelliJ classes bundled in the compiler-embeddable classpath vary across the versions in `versions.bzl`. It compiled under 2.3.20 and 2.4.0 (so the language-test builds were green) but failed the internal extractor build with `unresolved reference 'ProcessCanceledException'`. Match the exception by class name instead of by type. This keeps the exact same behaviour (the cancellation exception is still rethrown, everything else is logged and yields null) while removing the only compile-time reference to a class that is not present on every version's classpath. Verified the standalone extractor now builds for 1.9.20-Beta, 2.0.0-RC1 and 2.4.0. No expected-output changes: the runtime behaviour is unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The KDoc-section parser located the parsed KDoc via `com.intellij.psi.util.PsiTreeUtil.findChildOfType`. Like the `ProcessCanceledException` reference fixed in the previous commit, this `com.intellij` utility class is not present on every build's compiler classpath: the reduced/relocated embeddable classpath used by the internal extractor build strips it, so the build failed with `unresolved reference 'PsiTreeUtil'`. The standalone language-test builds (2.3.20, 2.4.0) bundle the full classpath and so did not catch it. Replace the utility call with a small hand-written depth-first search over the core `com.intellij.psi.PsiElement` navigation API (`firstChild`/`nextSibling`), which is always available (it is already used by the PSI comment extractor). The search has the same semantics as `findChildOfType`: it returns the first KDoc descendant in pre-order, so the parsed sections are unchanged. No expected-output changes: the KDoc lookup mechanism is behaviour-equivalent. Verified the standalone extractor still builds for 1.8.0, 1.9.20-Beta and 2.4.0. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ation The previous two commits removed the `ProcessCanceledException` and `PsiTreeUtil` references from KDoc-section parsing, but the internal extractor build then failed on `unresolved reference 'PsiElement'`: the hand-written tree walk named `com.intellij.psi.PsiElement` (and its `firstChild`/`nextSibling`). The root cause is that the LighterAST/K2 comment extractor is compiled against a curated classpath that deliberately excludes `com.intellij.psi.*`. It exposes only the `com.intellij.lang` LighterAST API (`LighterASTNode`, `FlyweightCapableTreeStructure`) plus the Kotlin PSI classes (`org.jetbrains.kotlin.psi.*`, `...kdoc.psi.*`). That is why `KtPsiFactory`, `KtFile`, `KDoc` and `getAllSections` all resolve while any `com.intellij.psi` reference does not. The full standalone builds bundle the whole classpath, so they never caught this. Locate the parsed KDoc through the Kotlin PSI API instead: the KDoc attaches to the throwaway `val __codeql_kdoc__` as its `docComment`, so `ktFile.declarations.firstOrNull()?.docComment` retrieves it using only `org.jetbrains.kotlin.psi` types (`KtCommonFile.getDeclarations`, `KtDeclaration.getDocComment`). No `com.intellij.psi` symbol remains in the file's code. Behaviour is unchanged: the parsed input is a single KDoc comment followed by one declaration, so its `docComment` is exactly the KDoc the old descendant search returned. No expected-output changes. Verified the standalone extractor builds for 1.9.0-Beta and 2.4.0. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…no cast)
Once the extractor built again, the internal `possiblyThrowingExpressions`
query (a lint over the extractor's own Kotlin source that forbids constructs
which can abort extraction) flagged two spots in the KDoc code added earlier
this session:
* a `throw e` used to re-raise `ProcessCanceledException` from
`parseKDocSections`, and
* an unchecked `element as IrDeclaration` cast in the metadata-less
KDoc-owner pass.
Both were previously masked because the internal build did not compile (the
`com.intellij.psi` references failed first), so the query never ran.
Remove the re-raise and simply log and continue, matching how the rest of the
extractor handles parse failures (it never lets exceptions escape). This also
drops the last reference to `ProcessCanceledException`.
Replace the cast with a `when` over the concrete IR declaration types
(`IrEnumEntry`, `IrAnonymousInitializer`), which smart-casts each branch to the
common `IrDeclaration` supertype. A plain `&& element is IrDeclaration` guard
would instead trip the K2 compiler's "check for instance is always true"
warning, which `-Werror` turns into a build failure.
No expected-output changes: behaviour is unchanged; only the exception handling
and the type narrowing are rewritten. Verified all 12 extractor versions in
`versions.bzl` build warning-free.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
For a call qualified through a not-null assertion (e.g. `s!!.foo()`), `TrapWriter.getStartOffset` derives the call's start from its receiver. Under K1 the receiver is the `CHECK_NOT_NULL`/EXCLEXCL intrinsic IrCall, whose own `startOffset` points at the first `!` character rather than at the operand `s`. The enclosing call therefore inherited the `!!` offset and omitted the receiver operand from its span. The `!!` intrinsic carries its operand as value argument 0 (not as a dispatch/extension receiver), so the existing receiver-based adjustment did not reach it. Extend `getStartOffset` to also consider value argument 0 of an EXCLEXCL call, recursively. This makes a `!!`-qualified call span from the operand, matching K2, which already anchors the receiver at the operand. The change affects source locations only; AST structure, call targets, and dataflow relationships are unchanged. K2 output is unaffected because its offsets already start at the operand. Fully converges dataflow/notnullexpr and exprs/binop between the K1 and K2 suites and removes 6 divergent rows overall. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 86bfc022-3ecc-4746-ba23-3b76c4e4c3e4
A compiler-generated default property accessor (`DEFAULT_PROPERTY_ACCESSOR`)
has no source body, so the two frontends anchor its synthesised body
expressions (`field = value` / `return field`, plus their `<set-?>`, field and
`this` sub-accesses) differently:
- K1 anchors them at the whole `KtProperty`, whose end offset runs through the
initialiser (`var topLevelInt: Int = 0` -> `60:1:60:24`,
`var curValue = 0` -> `26:13:26:24` .. `26:28`).
- K2 has no PSI for these accessors and falls back to the raw signature span,
which stops at the type (`60:1:60:20`) or, when the type is inferred, at the
name (`26:13:26:24`).
The initialiser is not part of the accessor body (it runs in the field
initialiser / `<clinit>`, not in the setter), so K2's narrower signature span is
the more intuitive, information-preserving choice: it keeps the accessor's
synthetic expressions on the property signature and leaves the initialiser to
the genuine `KtInitializerAssignExpr` / `<clinit>` rows.
We converge K1 onto the K2 span with a scoped offset remap set only while
extracting the accessor body (mirroring the existing delegated-property-accessor
remap). The remap is keyed off the accessor's corresponding property and matches
the property's full IR range exactly, so:
- it only rewrites the synthetic body expressions that carry that range;
- the real field-initialiser rows (which share the same source text but are
extracted in the initialiser context) are untouched and keep their
initialiser-inclusive spans; and
- it is a no-op under K2 (no PSI, so `getEnclosingKtProperty` returns null) and
for any property without an initialiser past its signature.
Tradeoff: this converges K1 onto a span that K2 produces natively but that K1
cannot recover without the PSI, so the direction is fixed (K1 -> K2) rather than
chosen freely. That is acceptable here because the K2 span is the more correct
one on the merits (the initialiser does not belong to the accessor body).
Expected updates (K1 only; K2 already emitted these):
- library-tests/exprs/exprs.expected (setCurValue, setTopLevelInt)
- library-tests/methods/exprs.expected (clinit.kt setTopLevelInt)
Both suites relearned; all tests pass. Divergence 820 -> 798 rows.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 86bfc022-3ecc-4746-ba23-3b76c4e4c3e4
…alue
An assignment to a property, delegated property or `@JvmStatic` property
(`lhs = rhs`) desugars to a setter call. The two frontends anchor that
synthesised call differently:
- K1 records the call (and its synthetic implicit-`this` receiver and any
synthetic reflection arguments) at the assignment's left-hand side only
(`varResource0 = 3` -> `37:9:37:20`, `curValue = value` -> `29:17:29:24`).
- K2 spans the whole assignment, through the assigned value
(`37:9:37:24`, `29:17:29:32`).
The setter call *is* the desugaring of the whole assignment, so K2's
whole-assignment span is the more intuitive, information-preserving one: it keeps
the call on the source construct it represents rather than dropping the
right-hand side. This matches the existing array-`set` EQ-origin widening
precedent, which already widens K1 onto the whole-assignment span.
We converge K1 onto the K2 span with a scoped offset remap set only while
extracting the setter call, keyed on the call's exact offset pair. Because the
compiler gives the synthetic implicit-`this` receiver and the synthetic
reflection arguments the same offsets as the call, the single remap widens them
along with the `MethodCall` node, while real source arguments (the assigned
value itself, e.g. the `2` / `3` / `value`) keep their own offsets and are
unaffected. The remap fires only for an `IrStatementOrigin.EQ` call whose last
value argument (the assigned value) ends past the call's own end offset, so it is
a no-op under K2 (where the call already spans the assigned value) and for every
non-assignment call.
Tradeoff: the direction is fixed (K1 -> K2) because K1 cannot, from its raw IR,
recover the whole-assignment span for these synthetic children; K2 produces it
natively. That is acceptable here because the whole-assignment span is the more
correct one on the merits and is already the established convention for array
`set`.
Expected updates (K1 only; K2 already emitted these):
- library-tests/exprs/exprs.expected (delegated + direct property setters)
- library-tests/generic-instance-methods/test.expected (setStored)
- library-tests/jvmstatic-annotation/test.expected (@JvmStatic setters)
Both suites relearned; all tests pass. Divergence 798 -> 758 rows.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 86bfc022-3ecc-4746-ba23-3b76c4e4c3e4
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: