Skip to content

fix: detect usage of more kinds of import, e.g. of inner classes. #1505

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.

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

Merged
merged 1 commit into from
Aug 7, 2025

Conversation

autonomousapps
Copy link
Owner

Resolves #1504

Comment on lines 61 to 60
// normalize class names to only contain '.' characters (in case of inner classes)
.associate { it.className.replace('$', '.') to it.constantFields.toSortedSet() }
Copy link
Owner Author

Choose a reason for hiding this comment

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

the fix part 1

Copy link

Job Summary for Gradle

Test :: gradle
Gradle Root Project Requested Tasks Gradle Version Build Outcome Build Scan®
build-logic buildHealth 8.14.3 Build Scan not published
build-logic check 8.14.3 Build Scan not published
dependency-analysis-gradle-pl…
buildHealth 8.14.3 Build Scan published
testkit buildHealth 8.14.3 Build Scan published
dependency-analysis-gradle-pl…
check 8.14.3 Build Scan published
testkit check 8.14.3 Build Scan published
dependency-analysis-gradle-pl…
:functionalTest 8.14.3 Build Scan published

@autonomousapps
Copy link
Owner Author

Ah darn, this is too aggressive a change and will result in false positives.

@autonomousapps
Copy link
Owner Author

autonomousapps commented Aug 2, 2025

Notes to self:

First, when enabling asm debug logging, I see this:

ClassNameAndAnnotationsVisitor#visit: PUBLIC com/example/library/Library$Inner extends java/lang/Object
...
- visitField: PUBLIC descriptor=Ljava/lang/String; name=CONSTANT signature=null value=magic

which clearly shows a field that would look roughly like this in source: public String CONSTANT = "magic". The logging may be deficient and probably it should state directly that it's public static... (i.e., a compile-time constant).

This analysis is on the producer side; specifically it's looking at the compiled class files coming from a project dependency. I don't know if it would be any different if analyzing a jar from an external dependency, but presumably not (or something is very wrong). This code is compiled with javac from the core Gradle plugins.

As an aside, I'm seeing that output twice, for both Task :consumer:explodeJarTest and > Task :consumer:explodeJarMain. That seems like a dumb waste of time.

Second, I see this on the consumer side:

ClassAnalyzer#visitMethod: PUBLIC useConstant ()V
- MethodAnalyzer#visitLdcInsn: magic
- MethodAnalyzer#visitFieldInsn: java/lang/System.out Ljava/io/PrintStream;
- MethodAnalyzer#visitMethodInsn: java/io/PrintStream.println (Ljava/lang/Object;)V

for the Kotlin source:

fun useConstant() {
  println(Inner.CONSTANT)
}

Due to constant inlining, I don't think I'll be able to see any reference to CONSTANT (the name of the constant field) in the consumer bytecode. But since I see this LDC bytecode instruction clearly referencing the value of that constant ("magic"), that might be enough to correlate things. I suppose multiple producers might have constants with the same value, so there's a 1-to-many relationship between these LDC values (on the consumer side) and the constants that have those values on the producer side. So there might be "this dependency is used" false positives.

New observation. I added int and float constants to the producer and usages of those on the consumer side. Here's the Java source:

public static final String CONSTANT = "magic";
public static final int INT_CONST = 9;
public static final float FLOAT_CONST = 4.2f;

and the Kotlin source:

fun useConstant() {
  println(Inner.CONSTANT)
  println(Inner.INT_CONST)
  println(Inner.FLOAT_CONST)
}

Here's the consumer bytecode:

ClassAnalyzer#visitMethod: PUBLIC useConstant ()V
- MethodAnalyzer#visitLdcInsn: magic
- MethodAnalyzer#visitFieldInsn: java/lang/System.out Ljava/io/PrintStream;
- MethodAnalyzer#visitMethodInsn: java/io/PrintStream.println (Ljava/lang/Object;)V
- MethodAnalyzer#visitFieldInsn: java/lang/System.out Ljava/io/PrintStream;
- MethodAnalyzer#visitMethodInsn: java/io/PrintStream.println (I)V
- MethodAnalyzer#visitLdcInsn: 4.2
- MethodAnalyzer#visitFieldInsn: java/lang/System.out Ljava/io/PrintStream;
- MethodAnalyzer#visitMethodInsn: java/io/PrintStream.println (F)V
- MethodAnalyzer#visitLocalVariable: this Lcom/example/Main;

there's a missing

- MethodAnalyzer#visitLdcInsn: 9

What is Kotlin doing when loading an int? Why is that behavior different from loading a String or float?

Ok, more info. If I add logging for the visitLineNumber() function, I see this:

ClassAnalyzer#visitMethod: PUBLIC useConstant ()V
- MethodAnalyzer#visitLineNumber: line=7
- MethodAnalyzer#visitLdcInsn: magic
- MethodAnalyzer#visitFieldInsn: java/lang/System.out Ljava/io/PrintStream;
- MethodAnalyzer#visitMethodInsn: java/io/PrintStream.println (Ljava/lang/Object;)V
- MethodAnalyzer#visitLineNumber: line=8
- MethodAnalyzer#visitFieldInsn: java/lang/System.out Ljava/io/PrintStream;
- MethodAnalyzer#visitMethodInsn: java/io/PrintStream.println (I)V
- MethodAnalyzer#visitLineNumber: line=9
- MethodAnalyzer#visitLdcInsn: 4.2
- MethodAnalyzer#visitFieldInsn: java/lang/System.out Ljava/io/PrintStream;
- MethodAnalyzer#visitMethodInsn: java/io/PrintStream.println (F)V
- MethodAnalyzer#visitLineNumber: line=10
- MethodAnalyzer#visitLocalVariable: this Lcom/example/Main;

So I suppose kotlinc is just doing some performance-related things for ints that it isn't doing for Strings or floats. The line numbers correspond 1:1 with the line numbers in the source code. It's possible I could use that.

Why can't kotlinc just produce a class file with a constant pool like a normal compiler?

Copy link

github-actions bot commented Aug 6, 2025

Job Summary for Gradle

Test :: gradle
Gradle Root Project Requested Tasks Gradle Version Build Outcome Build Scan®
build-logic buildHealth 8.14.3 Build Scan not published
build-logic check 8.14.3 Build Scan not published
dependency-analysis-gradle-pl…
buildHealth 8.14.3 Build Scan published
testkit buildHealth 8.14.3 Build Scan published
dependency-analysis-gradle-pl…
check 8.14.3 Build Scan published
testkit check 8.14.3 Build Scan published
dependency-analysis-gradle-pl…
:functionalTest 8.14.3 Build Scan published

Copy link

github-actions bot commented Aug 6, 2025

Job Summary for Gradle

Test :: gradle
Gradle Root Project Requested Tasks Gradle Version Build Outcome Build Scan®
build-logic buildHealth 8.14.3 Build Scan not published
build-logic check 8.14.3 Build Scan not published
dependency-analysis-gradle-pl…
buildHealth 8.14.3 Build Scan published
testkit buildHealth 8.14.3 Build Scan published
dependency-analysis-gradle-pl…
check 8.14.3 Build Scan published
testkit check 8.14.3 Build Scan published
dependency-analysis-gradle-pl…
:functionalTest 8.14.3 Build Scan published

Copy link

github-actions bot commented Aug 6, 2025

Job Summary for Gradle

Test :: gradle
Gradle Root Project Requested Tasks Gradle Version Build Outcome Build Scan®
build-logic buildHealth 8.14.3 Build Scan not published
build-logic check 8.14.3 Build Scan not published
dependency-analysis-gradle-pl…
buildHealth 8.14.3 Build Scan published
testkit buildHealth 8.14.3 Build Scan published
dependency-analysis-gradle-pl…
check 8.14.3 Build Scan published
testkit check 8.14.3 Build Scan published
dependency-analysis-gradle-pl…
:functionalTest 8.14.3 Build Scan published
dependency-analysis-gradle-pl…
:functionalTest 8.14.3 Build Scan published

@autonomousapps
Copy link
Owner Author

Sigh there are definitely false positives with this approach. Why do so many libraries define empty string constants, some even with the same name? Fucking Kotlin.

Comment on lines 218 to 224
is ConstantCapability -> usesConstant = usesConstant(dependencyCoordinates, capability, context)
is ConstantCapability -> {
// TODO
usesConstant = usesConstantByBytecode(dependencyCoordinates, capability, context)
usesConstant = usesConstant || usesConstantByImport(dependencyCoordinates, capability, context)
}

Copy link
Owner Author

Choose a reason for hiding this comment

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

TODO

@autonomousapps
Copy link
Owner Author

The problem is... solved... thanks to a pair of aggressive heuristics that partially cancel each other out, resulting in a successful test suite. Woohoo.

…asses.

This involves a pair of heuristics, one for source parsing (looking at import statements), and another from bytecode analysis (looking at the ldc instruction) and from that
inferring usage of inlined constants.

Resolves #1504"
@autonomousapps autonomousapps merged commit b447ea6 into main Aug 7, 2025
1 check passed
@autonomousapps autonomousapps deleted the trobalik.issue-1504 branch August 7, 2025 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use of constant from inner static class not detected
1 participant