Allow additional types in @KotlinCoreEnvironmentTest annotation

[marschwar]

This change allows #5182 to be tested.

When the test compiler is initialized, only the kotlin standard lib and kotlin coroutines are available to the binding context. This change allows additional jars to be added to the compiler environment via the @KotlinCoreEnvironmentTest annotation. The jars are determined by defining a type from the jar that should be added.

[BraisGabin]

Thanks for this :)

But I'm wondering... Shouldn't we add "by default" all the classes that we have inside the classpath? I mean, in my case my code compiled correctly. I mean, the compiler knew all the types correctly. But the env didn't know abot them. That's realy strange as a test writer and that could create odd issues difficult to spot. We use a lot of safe calls and maybe we are thinking that our tests are working because of X but they are just working because the BindingContext is not the correct one.

[BraisGabin]

Shouldn't these tests be inside detekt-test-utils instead of detekt-test?

[marschwar]

Technically yes. I wanted to test exactly your case from #5182 and detekt-test-utils is completely unaware of detekt rules and I wanted to keep it that way. Should I create an additional test in detekt-test-utils?

[marschwar]

Is there a reason why the KotlinCoreEnvironmentExtensionsare in detekt-test? This makes it really hard to test anything in detekt-test-utils.

[marschwar]

I mean, in my case my code compiled correctly. I mean, the compiler knew all the types correctly.

That is acutally not the case. For regular test execution, the compilation is effectively deactivated (System.getProperty("compile-snippet-tests", "false")). After activation, the test failed with Given Kotlin code is invalid..

Shouldn't we add "by default" all the classes that we have inside the classpath?

I do not think so. The BindingContext should (in general) only need to resolve kotlin language types. For library specific rule sets this is may be different. There it might be reasonable to add spring or compose or ... to the test compiler via this annotation.

[BraisGabin]

I had one of the tests that was invalid but I just fixed it (a private extension function that can't be private because the script restriction). But the other ones compile properly I use ./gradlew :detekt-rules-ruleauthors:test -Pcompile-test-snippets=true and all the snippets compile for that reason I feel this is strange.

[github-actions]

	Warnings
⚠️	This PR is approved with no milestone set. If merged, it won't appear in the Detekt release notes.

Generated by 🚫 dangerJS against 0b07896

[BraisGabin]

(I'm ok merging this but I think that we should take a look on that because it could create some noise for us and/or third party authors)

[marschwar]

You are right. The KotlinScriptEngine uses the current classpath to compile the script, The KtTestCompiler on the other hand does not when creating the KotlinCoreEnvironment.

Maybe @3flex can shed some light on this. I only have a very vague understanding of what is going on here.

[schalkms]

I think I can also shed some light on the KotlinScriptEngine, since I introduced JSR223 to detekt back then. I don’t get the question. Is it about the support of private extension functions? Private extension functions are not supported, as already mentioned.

[BraisGabin]

No, the issue that we have is that in #5182 the test snippets compile perfectly fine BUT the BindingContext that is generated lacks from Rule and BaseRule information so the tests fail. That's the problem, the classpah for the compilation check and the classpath to generate the BindingContext are different.

We can fix it with this PR but the behaviour is counter intuitive and third party rule authors are more likely to find this error because they want to check integration with libraries (that's the case of #5182 where I'm checking against detekt itself).

[cortinico]

Commenting here but sort of valid for the whole PR. I really like this new test feature, but at the same time I'm afraid it might reduce readability.

In this line, it's not immediately clear why you need to rely on CharRange.

Plus this reduces the test isolation as you're effectively exposing those tests to failures if the Rule class change (which might be considered a 'good' end result, but it's not what this test is trying to accomplish). Having a Rule class declared locally in a snippet should serve the same purpose no?

[marschwar]

@BraisGabin and you are right. I will move the test to detekt-test-utils and check against some other class being available

[schalkms]

In this case, I would use just lint() rather than compileAndLint() or merge this PR with the workaround. I don’t it’s so critical.

[marschwar]

Let me try to summarize the discussion here as there are multiple threads.

1. The problem this PR is trying to solve (or put a band-aid on) is that the BindingContext used for type resolution in tests by default can only use types from the stdlib and the coroutines lib. By allowing test authors to add types to the annotation, they become available for type resolution.
2. As @BraisGabin pointed out it is counter intuitive that the script compilation in tests can import all types that are available on the classpath, but only those from stdlib and coroutines are available for type resolution. (I will investigate further and create a separate issue)
3. I moved the tests to the same module as the annotation itself, but since the BindingContext for tests is created in the detekt-test (KotlinCoreEnvironmentExtensions.kt) module instead of the detekt-test-utils module, this function had to be copied as well which is far from ideal. Can we move it?