Skip to content

Support for BindingContext in FileProcessListener #2872

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

Closed
damianw opened this issue Jul 15, 2020 · 4 comments · Fixed by #2900
Closed

Support for BindingContext in FileProcessListener #2872

damianw opened this issue Jul 15, 2020 · 4 comments · Fixed by #2900
Labels
api feature migration Marker to add a migration step in the changelog
Milestone

Comments

@damianw
Copy link
Contributor

damianw commented Jul 15, 2020

Current Behavior

Implementations of Rule have access to the BindingContext, granting them the ability to perform complex analysis within the scope of the entire compilation unit. On the other hand, the FileProcessListener (and AbstractProcessor, etc) APIs do not provide any way to access the BindingContext, limiting their ability to resolve types, functions, and so forth.

Expected Behavior

In order to enable the same level of analysis for implementations of FileProcessListener, the API should provide them a BindingContext against which references can be resolved.

Context

I am in the process of implementing "occurrences counting" metrics for our codebase such as "number of invocations of function A" and "number of constructor calls to classes which implement interface X". Complex queries such as this require a BindingContext, but the provided mechanism for making such queries, AbstractProjectMetricProcessor, does not provide it.

@arturbosch arturbosch added api feature migration Marker to add a migration step in the changelog labels Jul 15, 2020
@arturbosch
Copy link
Member

Yep, noticed the same.
We could either introduce new functions with KtFile and BindingContext or use @JvmOverloads for not introduce a breaking change.

@cortinico
Copy link
Member

We could either introduce new functions with KtFile and BindingContext or use @JvmOverloads for not introduce a breaking change.

I took a quick look at this.

I think that the breaking change here is unavoidable with @JvmOverloads as its not allowed on interface methods:

/**
* Called when processing of a file begins.
* This method is called from a thread pool thread. Heavy computations allowed.
*/
fun onProcess(file: KtFile) {}

What we could do, is use @JvmDefault and provide something like:

@JvmDefault
fun onProcess(file: KtFile, bindingContext: BindingContext) {
    onProcess(file)
}

But we would need to add a compiler flag. Is this something we want to do?

@arturbosch
Copy link
Member

arturbosch commented Jul 20, 2020

What we could do, is use @JvmDefault and provide something like:

@JvmDefault
fun onProcess(file: KtFile, bindingContext: BindingContext) {
    onProcess(file)
}

But we would need to add a compiler flag. Is this something we want to do?

Is there a benefit in using @JvmDefault instead of a plain interface function calling onProcess(file)? I guess less bytecode because it is the 'official' java way on the JVM.

@cortinico
Copy link
Member

Is there a benefit in using @JvmDefault instead of a plain interface function calling onProcess(file)? I guess less bytecode because it is the 'official' java way on the JVM.

Yes but I think I overlooked the API change here. According to eclipse.org adding a API method should always be considered a breaking change (as our clients might already have a method with the same signature).

In this specific case, If we don't use @JvmDefault this is going to break compatibility towards our Java users as they will need to implement the onProcess(KtFile, BindingContext) method. Adding the @JvmDefault will make sure that a default method is generated, hence users of this interface won't be forced to implement the onProcess(KtFile, BindingContext) method.

However, I expect the majority of our users to use Kotlin to code their custom rules/processors. Therefore, we should be fine with just:

fun onProcess(file: KtFile, bindingContext: BindingContext) {
    onProcess(file)
}

and we should probably not be worried about our Java users (as we probably don't have any).

@arturbosch arturbosch added this to the 1.11.0 milestone Jul 27, 2020
arturbosch added a commit that referenced this issue Jul 28, 2020
* Introduce binding context aware members for FileProcessListener

* Use FileProcessListener with binding context everywhere

* Move deprecation on function level
@arturbosch arturbosch modified the milestones: 1.11.0, 1.10.1 Aug 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api feature migration Marker to add a migration step in the changelog
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants