Use consistent nullability for code arguments

[GlassBricks]

Many functions in FunSpecBuilder and other places that take code arguments are non-nullable, whereas the corresponding functions in CodeBlock Builder have code arguments that are non-nullable.

Code arguments should either be consistently nullable or non-nullable.

Changing all code arguments to be nullable would be source and binary compatible (kotlin treats Any? as a supertype of Any, but both erase to Object on jvm).

Relates to (#789)

[Egorand]

I guess you'd usually want to either omit passing any args or pass non-null args, with CodeBlock("whatever", null) being an edge case. Are there any use cases that would benefit from Builder methods accepting nullable code arguments, or is it just for the sake of consistency?

[GlassBricks]

In some cases, this is definitely for the sake of consistency (in FunSpec Builder, addCode takes nullable args,addStatement does not).

There are other cases when nullable args may be desirable (e.g. #789).

However, as in many cases nullability seems to be an edge case, the current CodeBlock.of() workaround could be used, but this should be documented.

[Egorand]

I'm leaning towards reducing the number of APIs that accept nullable arguments, I believe that would lead to less ambiguity in the API. In the example from #789, should the library consumer expect that CodeBlock.of("%S", null) produces null or "null"? Making args non-nullable everywhere should help solve this, however this is a source-incompatible change.

[GlassBricks]

On another note, some functions in FunSpec.Builder that directly correspond to functions in CodeBlock.Builder, take non-nullable arguments, whereas the CodeBlock.Builder counterparts take nullable arguments: addStatement, begin/endControlFlow. I think at least these functions should have nullable arguments for consistency. Reducing nullability in other contexts sound reasonable.

Whatever later changes, there should be some way of handling the corner case where nullable args is desired.

[Egorand]

there should be some way of handling the corner case where nullable args is desired

I consider CodeBlock to be a more advanced concept in the API hence it provides more flexibility at the cost of null safety and predictability. In many cases it should be possible to avoid using CodeBlock and stick to the methods on the spec builders which are safer but less flexible.

[GlassBricks]

FunSpec.Builder.addStatement does not take nullable args; CodeBlock.Builder.addStatement does; similarly for beginControlFlow and nextControlFlow. FunSpec.Buidler.addCode, however, anomalously takes nullable args.

I think at least in FunSpec.Builder addStatement should take nullable arguments for consistency, and maybe begin/nextControlFlow. (This was the original intended subject of this issue)

It would make sense to keep args non-nullable in other cases.

[Egorand]

FunSpec.Builder.addStatement does not take nullable args; CodeBlock.Builder.addStatement does; similarly for beginControlFlow and nextControlFlow.

That makes sense to me from the perspective of CodeBlock API being more allowing/unsafe. Use FunSpec.Builder.addStatement(String) fore simple scenarios, and FunSpec.Builder.addStatement(CodeBlock) for more complex ones.

I think at least in FunSpec.Builder addStatement should take nullable arguments for consistency, and maybe begin/nextControlFlow.

My stance on this is that we shouldn't make the API less safe and less predictable by switching from non-nullable parameters to nullable for the sake of consistency only - if there are specific use-cases where the API feels unreasonable let's evaluate them one by one and decide on the action to take. The other way around (nullable -> non-nullable) would be fine by me, but that requires KotlinPoet 2.0.

[micHar]

https://github.com/FutureMind/koru/pull/14/commits/56b85b1fd8ddd0d15f9ec943ee5cde8cc05ba2a0#diff-7497f41874fbb53e06fe3194cdebddb7dd5d1bbc2d6b35e6f8c656f43f679b01L78

I'm not sure if you can count it as a reasonable use case, but I think that having nullable params is legitimate. Unless I'm not missing something, it would nicely simplify the code - now depending on wether my member is null or not, I need to use a completely different statement.

[Egorand]

@micHar does something like this work in your case?

val result = if (scopeProviderMemberName != null) {
  CodeBlock.of("%M", scopeProviderMemberName)
} else {
  CodeBlock.of("null")
}
this.addStatement(
  "return %T(%L) { ${originalFunSpec.asInvocation()} }",
  SuspendWrapper::class,
  result,
)

[micHar]

Oh, looks better than what we finally merged, thank you!

Still, could be even simpler if you allowed for nullable type inside MemberName.

Although I must admit, that I feel there is some difference between an argument with a nullable type and an optional argument and that what I wanted to do originally might be semantically debatable... Anyway that's my two cents.

[Egorand]

Yeah, in your specific case I don't think it makes sense to expect CodeBlock.of("%M", null) to evaluate to null. Semantically, null can never be a member (same as null can't be a type). Our current behaviour of throwing an NPE seems correct to me:

java.lang.NullPointerException: null cannot be cast to non-null type com.squareup.kotlinpoet.MemberName