The current implementation keeps the old behavior.
But what do you expect here for fq kotlin.test.assertFalse?

in which case it.text could be null? (same for type arguments

I think, for example, in case of some incorrect (can not be compiled) sample snippet fun f(,,,,) but I have not checked this case.

I had a similar question, as I thought it would be weird to see just an empty string in argumentList as it's not mentioned in the KDoc

With the case Vadim provided, not sure what's better though: to pass an empty string or to exclude it at all (via mapNotNull), as both have disadvantages and might lead to invalid/unrannable samples

Is there some place where we could check this or ask someone? May be just providing arguments list as List<String?> (with nullable generic) will be better in this case?

Sorry for the misleading. I have checked :fun f(,,,,) just returns an empty string for the parameter. I will put this case into the KDoc.

An answer is here, it returns getNode().getText() that is marked with NotNull.
So there is no real case for null.

I believe it will be better to provide function instead of map (and use map in implementation if needed), f.e:

public fun functionCallRewriter(name: String): FunctionCallRewritter?

In this case, I think we can also remove ShortFunctionName and just write a kdoc, that name is a short name (with example of what it is)`

What user case does using a function instead of map cover? Or what essential advantages of using it?
As for me, it can be a little confusing to have a function that returns null while FunctionCallRewritter can also return null.

A map can be deprecated easier than a function. For example, let's suppose for merging of multiple rewriters we need to know all rewriters, so a map could be useful.

I agree with Oleg that having a function might be better than a map, and I have a slightly different suggestion to simplify it further: get rid of the FunctionCallRewriter interface at all, and move the rewrite function down into SampleRewriter:

public interface SampleRewriter {
    public fun rewriteFunctionCall(shortName: String, arguments: List<String>, typeArguments: List<String>): String? = null
}

The advantage here is that the user doesn't need to implement the interface for every single function name, and it's generally easier to understand how to use it and what's going on. It should also be easier to implement it (can be a single switch statement)

The disadvantage is that we'll be creating more garbage (creating lists for every function), but it doesn't affect 99.9% of projects that don't have a SampleRewriter. It also shouldn't be noticeable as there are bigger performance problems, and it can be optimized later if needed

The meaning of null changes here. Instead of meaning "remove this call", it'll mean "do not re-write this call", and if you want to remove it - I guess you'd return "", which looks more or less ok

What do you think?

I think a such function is also ok. Additionally I like it is consistent with rewriteImportDirective.
Originally the map was needed because of "do not re-write this call" and an empty string was reserved for keeping formatting.
There is a 'problem' that a such function encourages to rewrite "do not re-write this call" to something like $shortName(${arguments.joinToString()}) that can break formatting or even representation (in case of a lamda).
I have no preference.

While I do like the consistency with rewriteImportDirective, I don't really like, that we introduce such possible performance problem, as the amount of declarations we need to rewrite is small comparing to those we don't need to rewrite.
On current moment if we will go with just rewriteFunctionCall we will:

1. create a visitor for each function call
2. execute it to collect all arguments and type arguments of a function call

• if we need to rewrite the call - we will create some string in rewriter
• if not - we will still need to create a string or manually (via $shortName(${arguments.joinToString()}) somehow handle lambdas) or provide some unobvious stub return value ("") to not rewrite the call and also additionally construct string back.
  So I think In this case we will only complicate things more for this niche use-case.

Regarding Map vs just function which return rewriter: overall the difference is minimal here.
But I think it's better to try to keep API smaller with property and Map migration will be harder. If we will want to change something, as functions could have overloads. Or we will be able to introduce separate function with different name. While with a Map property it's a little harder to do this IMO.

it can be a little confusing to have a function that returns null while FunctionCallRewritter can also return null.

As for me it's look logical, as we have 3 possible cases for a function call:

1. do nothing (functionCallRewriter returns null)
2. remove function (FunctionCallRewritter returns null, same as for rewriteImportDirective)
3. rewrite function (FunctionCallRewritter returns something)

For example, let's suppose for merging of multiple rewriters we need to know all rewriters, so a map could be useful.

I don't see how this will help here, but may be :) Though, we don't have this requirement right now and also don't have an idea on how this should look, how to resolve conflicts, what about ordering, what about rewriting after rewriting and so on. So I would think, that if we will want to support this, there will be more problems and even Map could not be enough.

In the end, I don't have big preference over one or another API, as this API for now should not be used heavily :)

To sum up, nobody does not mind a function instead of a map.
We can choose public fun functionCallRewriter(name: String): FunctionCallRewritter? since it does not have potential problems with performance and an ambiguous default value.
@IgnatBeresnev any objections/preferences?

Sounds good to me

It looks a bit confusing for me that, other extension points of KotlinAnalysisPlugin should be provided by descriptors/symbols, but this one should be provided by user in their plugin.
May be we need to somehow separate those things? (even if for now this is internal use-case)

Hmm, I think I can see your point:

• sampleAnalysisEnvironmentCreator and externalDocumentableProvider should be provided BY an analysis implementation FOR plugin authors to use (thus it cannot be empty/null)
• sampleRewriter should be provided BY plugin authors FOR an analysis implementation to use (thus it can be empty/null)

Can't think of an obviously good way to differentiate between the two now though (the only option KotlinAnalysisExtensionsPlugin is confusing as hell), so it'd be good to discuss this

Yeah, I also thought about providing one more plugin for configuration :)
So may be we can leave with current way for a while

Yeah, I also thought about providing one more plugin for configuration :)

KotlinAnalysisConfigurationPlugin actually sounds like a nice option 👍 With configuration in the name, it's clear to me that it actually configures the analysis in some way as opposed to providing services to use