Skip to content

Kotlin: fix cases where type variables were used out of scope #9123

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 14 commits into from
May 19, 2022
Merged

Kotlin: fix cases where type variables were used out of scope #9123

merged 14 commits into from
May 19, 2022

Conversation

smowton
Copy link
Contributor

@smowton smowton commented May 11, 2022 •
edited
Loading

  • Substitute type parameters when creating a SAM class instance that satisfies a generic instance
  • In the process, fix source-locations of SAM class instances to indicate the instantiation site, not the interface being satisfied
  • Fix generic arg extraction when Function23+ get rewritten to FunctionN in the JVM lowering
  • Fix the type-parameter-out-of-scope test to detect these situations in future

This is temporarily based on #9122 -- the real diff is smowton/codeql@smowton/admin/update-kotlin...smowton:smowton/fix/type-variable-in-scope-consistency

@smowton smowton requested review from a team as code owners May 11, 2022 20:13
@github-actions github-actions bot added the Java label May 11, 2022
@smowton smowton added the no-change-note-required This PR does not need a change note label May 11, 2022
github-advanced-security[bot]
github-advanced-security bot found potential problems May 11, 2022
View reviewed changes
tamasvajk
tamasvajk reviewed May 12, 2022
View reviewed changes
java/kotlin-extractor/src/main/kotlin/KotlinUsesExtractor.kt
// References to SomeGeneric<T1, T2, ...> where SomeGeneric is declared SomeGeneric<T1, T2, ...> are extracted
// as if they were references to the unbound type SomeGeneric.
val extractedTypeArgs = when {
c.symbol.isKFunction() && typeArgs != null && typeArgs.isNotEmpty() -> listOf(typeArgs.last())
extractClass.symbol.isKFunction() && typeArgs != null && typeArgs.isNotEmpty() -> listOf(typeArgs.last())
extractClass.fqNameWhenAvailable == FqName("kotlin.jvm.functions.FunctionN") && typeArgs != null && typeArgs.isNotEmpty() -> listOf(typeArgs.last())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a test case for this? How come we didn't see this issue before? We have tests that cover big-arity functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can add one -- I think we simply didn't notice or care that the type extracted was FunctionN<FirstArgumentType> not FunctionN<ReturnType>. It's probably not a critical bug but rather a weird surprise waiting for someone down the line.

@tamasvajk
Copy link
Contributor

tamasvajk commented May 12, 2022 •
edited
Loading

Are the kotlin CI tests running on this PR? If not, I think it would make sense to open the PR still in the private repo.

@smowton
Copy link
Contributor Author

smowton commented May 13, 2022

@tamasvajk it appears yes they are-- in recent runs of the semmle-code job Java Language Tests Linux we see lines like

[2/8] [991/1179 comp 4.3s eval 206ms] PASSED /home/runner/work/semmle-code/semmle-code/ql/java/ql/test/kotlin/library-tests/type_equivalences/type_equivalences.ql

I believe our integration tests are not running however: I'll make a PR to fix this later today.

@smowton
Copy link
Contributor Author

smowton commented May 13, 2022 •
edited
Loading

Ah no I take it back, we do run the Kotlin integration tests. To check: do we have the right file filters selecting when the integration tests run, and do we need a single-language tests job for Kotlin?

@smowton smowton force-pushed the smowton/fix/type-variable-in-scope-consistency branch from 6024ec6 to 2f55999 Compare May 13, 2022 15:10
@smowton
Copy link
Contributor Author

smowton commented May 13, 2022 •
edited
Loading

@tamasvajk added a test -- if you revert the commit "Fix extracted type arguments of kotlin.jvm.functions.FunctionN" then you'll see the FunctionN type arguments change improperly (note their pretty-printed names work but the actual argument disagrees):

-| test.kt:4:5:4:138 | f1 | FunctionN<String> | String |
-| test.kt:5:5:5:134 | f2 | FunctionN<T1> | T1 |
-| test.kt:6:5:6:110 | f3 | FunctionN<T3> | T3 |
+| test.kt:4:5:4:138 | f1 | FunctionN<String> | Integer |
+| test.kt:5:5:5:134 | f2 | FunctionN<T1> | Integer |
+| test.kt:6:5:6:110 | f3 | FunctionN<T3> | T2 |

@smowton smowton force-pushed the smowton/fix/type-variable-in-scope-consistency branch from 2f55999 to 189b555 Compare May 13, 2022 17:23
@smowton
Copy link
Contributor Author

smowton commented May 13, 2022

Requires semmle-code PR https://github.com/github/semmle-code/pull/42569

smowton added 11 commits May 19, 2022 11:55
@smowton
Fix typeParametersInScope consistency query
d291e0c 
The selection of type variables mentioned in a particular class previously didn't work as intended, so the consistency query would always pass.
@smowton
Extract type parameters without substituting their parent functions
b09b769 
Otherwise references to type variables declared on kotlin.Xyz.someFunction can refer to its Java equivalent java.Xyz.someFunction if it has one.
@smowton
Type parameter scoping check: distinguish type arguments from type pa…
048a530 
…rameters

I had forgotten that the Java QL lib regards a ParameterizedType as either an instantiation Generic<String>, or the unbound declaration Generic<T>.
@smowton
Fix type substitution and source locations in SAM-converted generic i…
102cdcd 
…nterface implementations

For example, in implementing Producer<T> by an actual lambda of type () -> Int, the return type should be Int, not T. This produced type-variable-out-of-scope consistency check failures.
@smowton
Fix extracted type arguments of kotlin.jvm.functions.FunctionN
4e15f5f 
Previously we accidentally extracted an argument type instead of the result type.
@smowton
Type-variable-in-scope consistency query: account for all enclosing e…
49c9c36 
…lements that declare type parameters.
@smowton
Distinguish result type parameter names
ada31f3 
This makes debugging a little easier.
@smowton
Accept test changes
8a90dde 
These are mainly moving the source locations and type specialisations in SAM-converted methods.
@smowton
Add test
ea9aa59
@smowton
Accept consistency check failure
4f54bb6 
The Java extractor assigns a type with unbound type variables to the result of ImmutableSortedMap.of calls.
@smowton
Autoformat
e722c99
@smowton smowton force-pushed the smowton/fix/type-variable-in-scope-consistency branch from 6180996 to e722c99 Compare May 19, 2022 10:55
@smowton smowton mentioned this pull request May 19, 2022
Merged
@smowton
Copy link
Contributor Author

smowton commented May 19, 2022

One test failure is expected (taken care of by the corresponding internal PR); one is an unrelated failure also affecting main.

@smowton smowton merged commit 01aaa6c into github:main May 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@igfoo igfoo igfoo approved these changes

+1 more reviewer

@tamasvajk tamasvajk tamasvajk left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Java no-change-note-required This PR does not need a change note
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@smowton @tamasvajk @igfoo