Skip to content
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

Few comments regarding the FIR and BIR apis #11

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Few comments regarding the FIR and BIR apis #11

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions ...iler-plugin/src/main/kotlin/arrow/inject/compiler/plugin/fir/FirAbstractProofComponent.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,16 @@ internal interface FirAbstractProofComponent {

val session: FirSession

/**
* (FIR API review and comments)
*
* I can see how providing a list of predicates to search up front
* can be efficient to avoid traversing the tree searching for declarations.
* In this case though we are forcing the user to always use or look for annotated declarations.
*
* Ideally predicates are more expressive than just searching for annotations and allow the user to
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's impossible, because it won't work in IDE, because it will require resolution of the whole module to TYPES stage, which will be very slow

* search for any of the parts in the signature declaration.
*/
val contextPredicate: DeclarationPredicate
get() =
has(ProofAnnotationsFqName.ContextAnnotation)
Expand Down
10 changes: 10 additions & 0 deletions ...ugin/src/main/kotlin/arrow/inject/compiler/plugin/fir/FirArrowInjectExtensionRegistrar.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,16 @@ public class FirArrowInjectExtensionRegistrar(
private val proofCache: ProofCache,
) : FirExtensionRegistrar() {

/**
* (FIR API review and comments)
*
* The `+` operator makes for a nice API in simple plugins whose extensions
Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree, it may be not the best syntax. I just thought at some point that it looks pretty nice, but I'm not sure about it right now

* only take a `session` as single argument but becomes more unreadable when like in our
* case we need to pass extra arguments to the constructor.
*
* An alternative could be that our `proofCache` argument becomes a session component.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Indeed, session components are not available in backend. But you can use something like that:

class ProofCache

class ProofCacheStorage(session: FirSession, val proofCache: ProofCache) : FirExtensionSessionComponent(session)

private val FirSession.proofCacheStorage: ProofCacheStorage by FirSession.sessionComponentAccessor()
val FirSession.proofCache: ProofCache
    get() = proofCacheStorage.proofCache

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice!, we will try to refactor to that

* We tried that but then in the IR generation part we don't have access to the session anymore.
*/
override fun ExtensionRegistrarContext.configurePlugin() {
+{ session: FirSession -> ContextProvidersResolutionExtension(proofCache, session) }
+{ session: FirSession -> ResolvedFunctionGenerationExtension(proofCache, session) }
Expand Down
8 changes: 8 additions & 0 deletions ...t-compiler-plugin/src/main/kotlin/arrow/inject/compiler/plugin/fir/FirProofIdSignature.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,14 @@ import org.jetbrains.kotlin.fir.declarations.FirDeclaration
import org.jetbrains.kotlin.fir.signaturer.FirBasedSignatureComposer
import org.jetbrains.kotlin.ir.util.IdSignature

/**
* (FIR API review and comments)
*
* We tried getting signatures or the composer from the FIR session
* but could not find a better way, so we create our own here.
* The generated signatures are used to look up declarations in backend IR related
* to the providers found suitable for resolved calls
*/
interface FirProofIdSignature {

val session: FirSession
Expand Down
9 changes: 9 additions & 0 deletions arrow-inject-compiler-plugin/src/main/kotlin/arrow/inject/compiler/plugin/fir/ProofKey.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,13 @@ package arrow.inject.compiler.plugin.fir

import org.jetbrains.kotlin.fir.declarations.FirPluginKey

/**
* (FIR API review and comments)
*
* # what's the plugin key for?
*
* We have used this one across the frontend to set the `origin`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Keys can be used for passing some information from FIR to IR. We save a plugin key in IrDeclarationOrigin, so you can create it in frontend generator and then read it from the IR for this declaration

We're considering different approaches for such data translation (e.g. by introducing declaration attributes for IR declarations), but for now this is the only way except some external service

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense and this clarifies it.

* property in nodes, but we are not sure what good it does or if
* it's needed at all
*/
internal object ProofKey : FirPluginKey()
9 changes: 8 additions & 1 deletion ...in/kotlin/arrow/inject/compiler/plugin/fir/codegen/ResolvedFunctionGenerationExtension.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,14 @@ internal class ResolvedFunctionGenerationExtension(
valueParameters +=
buildValueParameters(firNamedFunctionSymbol) + unambiguousUnitValueParameter()
}
.apply { this.originalForSubstitutionOverrideAttr = firNamedFunctionSymbol.fir }
.apply {
/**
* (FIR API review and comments)
* We have run into runtime errors a few time when compiling because some of the properties
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a price for builder functions instead of constructor calls. There are two things which we possibly can do with it:

  1. Write proper documentation
  2. Introduce safe builders feature (actually, I want to do it for four years, because it was the topic of my master degree, but the most optimistic estimate I can give is release of FIR, which mostl likely will be 1.10)

Copy link
Member Author

Choose a reason for hiding this comment

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

We are very interested in safe builders as well, if there is anything we can do to help there even if in the future let us know. :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Design for them was done a long time ago. So now we need to actualize it and implement it in the FIR.
The thing is that we don't want to do it in both frontends

* in the FIR and IR builders have `lateinit var` that are not required on construction.
*/
this.originalForSubstitutionOverrideAttr = firNamedFunctionSymbol.fir
}
.symbol
}

Expand Down
13 changes: 13 additions & 0 deletions ...gin/src/main/kotlin/arrow/inject/compiler/plugin/fir/collectors/ExternalProofCollector.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,19 @@ import org.jetbrains.kotlin.name.CallableId
import org.jetbrains.kotlin.name.FqName
import org.jetbrains.kotlin.name.Name

/**
* (FIR API review and comments)
*
* In this service we had to resort to a very costly and slow process which
* involves scanning the classpath. For that we had to use the classgraph library.
* Seems like all FIR apis are geared toward searching and finding things in the same module
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is a dependenciesSymbolProvider which searches for declarations outside the current module (including other source modules in IDE).

But in any case classpath scanning looks for me like a very questionable way to do anything in the compiler plugin

* but there is no way to search for declarations in third party jar packages.
* Since our plugin exports declaration as providers that ultimately get compiled in a jar we are
* in need of scanning the classpath.
*
* A suggested alternative was to codegen a known file on each jar that pointed to the
* declarations but we did not want to add additional resources or binaries to it.
*/
internal class ExternalProofCollector(
override val session: FirSession,
) : FirAbstractProofComponent, FirProofIdSignature {
Expand Down
7 changes: 7 additions & 0 deletions ...n/src/main/kotlin/arrow/inject/compiler/plugin/fir/errors/FirMetaErrorsDefaultMessages.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,13 @@ import org.jetbrains.kotlin.diagnostics.rendering.BaseDiagnosticRendererFactory
import org.jetbrains.kotlin.diagnostics.rendering.Renderer
import org.jetbrains.kotlin.types.model.KotlinTypeMarker

/**
* (FIR API review and comments)
*
* This API regarding how errors are managed and subscribed seems to be
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll improve it as a part of FIR IDE plugins integration

* bringing patterns, name and conventions from the IDEA open API java counterpart.
* Ideally users can just register or report these in the checkers or places where they get used.
*/
internal object FirMetaErrorsDefaultMessages : BaseDiagnosticRendererFactory() {

override val MAP: KtDiagnosticFactoryToRendererMap =
Expand Down
7 changes: 7 additions & 0 deletions ...ain/kotlin/arrow/inject/compiler/plugin/fir/resolution/ProofResolutionCheckerExtension.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,13 @@ import org.jetbrains.kotlin.fir.expressions.FirCall
import org.jetbrains.kotlin.fir.extensions.FirDeclarationPredicateRegistrar
import org.jetbrains.kotlin.fir.types.FirTypeRef

/**
* (FIR API review and comments)
*
* We abstracted away access to this service because
Copy link
Collaborator

Choose a reason for hiding this comment

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

I didn't fully get the problem. There is an example of checkers component for parcelize and I don't think that there is too much boilerplate

class FirParcelizeCheckersExtension(session: FirSession) : FirAdditionalCheckersExtension(session) {
    override val expressionCheckers: ExpressionCheckers = object : ExpressionCheckers() {
        override val annotationCallCheckers: Set<FirAnnotationCallChecker>
            get() = setOf(FirParcelizeAnnotationChecker)
    }

    override val declarationCheckers: DeclarationCheckers = object : DeclarationCheckers() {
        override val classCheckers: Set<FirClassChecker>
            get() = setOf(FirParcelizeClassChecker)

        override val propertyCheckers: Set<FirPropertyChecker>
            get() = setOf(FirParcelizePropertyChecker)

        override val simpleFunctionCheckers: Set<FirSimpleFunctionChecker>
            get() = setOf(FirParcelizeFunctionChecker)

        override val constructorCheckers: Set<FirConstructorChecker>
            get() = setOf(FirParcelizeConstructorChecker)
    }
}

The point of having different sets for different checkers is to ensure that

  1. none checker won't be forgotten during checker stage
  2. we don't run checkers on incompatible declaration types (e.g. class checker won't be run on some simple function)

* the creation of instances and nesting of objects was too much boilerplate
* when registering each service
*/
internal class ProofResolutionCheckerExtension(
session: FirSession,
declarationCheckers: List<FirAbstractDeclarationChecker>,
Expand Down
21 changes: 20 additions & 1 deletion ...row/inject/compiler/plugin/fir/resolution/contexts/ContextProvidersResolutionExtension.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,16 @@ import arrow.inject.compiler.plugin.model.Proof
import org.jetbrains.kotlin.fir.FirSession
import org.jetbrains.kotlin.fir.expressions.FirFunctionCall
import org.jetbrains.kotlin.fir.extensions.FirExpressionResolutionExtension
import org.jetbrains.kotlin.fir.references.FirResolvedNamedReference
import org.jetbrains.kotlin.fir.resolvedSymbol
import org.jetbrains.kotlin.fir.symbols.impl.FirNamedFunctionSymbol
import org.jetbrains.kotlin.fir.types.ConeKotlinType
import org.jetbrains.kotlin.fir.types.FirTypeProjection
import org.jetbrains.kotlin.fir.types.toConeTypeProjection
import org.jetbrains.kotlin.fir.types.type
import org.jetbrains.kotlin.name.CallableId
import org.jetbrains.kotlin.name.FqName
import org.jetbrains.kotlin.name.Name

internal class ContextProvidersResolutionExtension(
override val proofCache: ProofCache,
Expand All @@ -18,6 +24,14 @@ internal class ContextProvidersResolutionExtension(

override val allProofs: List<Proof> by lazy { allCollectedProofs }

/**
* (FIR API review and comments)
*
* This extension is great and takes care of all the scoping resolution issues we have in the frontend.
* Something odd we detected is that for calls that you return additional types then in backend IR the
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please report this as YT issue. It will be great if you attach a very simple reproducer project to it (not the whole arrow-inject)

* expression will show up as an error expression that requires substitution.
* See [arrow.inject.compiler.plugin.ir.ProofsIrContextReceiversRecCodegen.replaceErrorExpressionsWithReceiverValues]
*/
override fun addNewImplicitReceivers(functionCall: FirFunctionCall): List<ConeKotlinType> {
// TODO add resolve proof here instead and iterate over all contexts in the proof
return functionCall.contextSyntheticFunctionTypeArguments.mapNotNull {
Expand All @@ -27,7 +41,12 @@ internal class ContextProvidersResolutionExtension(

// TODO: Improve this equality
private val FirFunctionCall.isCallToContextSyntheticFunction: Boolean
get() = calleeReference.name.asString() == "context"
get() =
(calleeReference as? FirResolvedNamedReference)?.let {
(it.resolvedSymbol as? FirNamedFunctionSymbol)?.let {
it.callableId == CallableId(FqName("arrow.inject.annotations"), Name.identifier("with"))
}
} ?: false

private val FirFunctionCall.contextSyntheticFunctionTypeArguments: List<FirTypeProjection>
get() = if (isCallToContextSyntheticFunction) typeArguments else emptyList()
Expand Down
9 changes: 9 additions & 0 deletions ...kotlin/arrow/inject/compiler/plugin/fir/resolution/resolver/ProofResolutionStageRunner.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,15 @@ import org.jetbrains.kotlin.resolve.calls.tasks.ExplicitReceiverKind
import org.jetbrains.kotlin.resolve.calls.tower.CandidateApplicability
import org.jetbrains.kotlin.types.Variance

/**
* (FIR API review and comments)
*
* This entire service has been ported more or less from the compiler
* FIR call resolver and similar services.
* This was done so that we could resolve a fir call given a list of
* provider candidates. At the time we are writing this FIR does not
* expose any public service or hook for call resolution.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeo, it won't be possible to modify existing resolution. But I think we can simplify API for custom calling of FirCallResolver. Please create a feature request for it in the YT

*/
internal class ProofResolutionStageRunner(
override val session: FirSession,
private val firResolutionProof: FirResolutionProof,
Expand Down
7 changes: 7 additions & 0 deletions ...ompiler-plugin/src/main/kotlin/arrow/inject/compiler/plugin/ir/ProofsIrAbstractCodegen.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,13 @@ interface ProofsIrAbstractCodegen : IrPluginContext, TypeSystemContext {
.filterNotNull()
}

/**
* (FIR API review and comments)
*
* Looked around for an api that would allow me to generate a call given a
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be nice to have some helpers to simplify generating FIR and IR as part of the compiler API. I faced with such boilerplate issues myself and I don't like it.

You can create such helpers by yourself and send them as PR, it would be very helpful

* declaration and fill in all the boilerplate but could not find one.
* Here based on the type of declaration we create an appropriate call for it.
*/
fun IrDeclaration.irCall(): IrExpression =
when (this) {
is IrProperty -> {
Expand Down
Loading