facebook / ktfmt

A program that reformats Kotlin source code to comply with the common community standard for Kotlin code conventions.
https://facebook.github.io/ktfmt/
Apache License 2.0
933 stars 78 forks source link

Missing support for context parameters on properties #518

Open koljakube opened 1 month ago

koljakube commented 1 month ago

Hello!

While introducing context parameters in my code, I noticed that ktfmt is not able to handle them on properties (at least properties in classes/interfaces, global scope seems to work for some reason).

Two possible errors are raised:

0

context(Unit)
val someOuterProp: String get() = ""

This is fine. I use Unit for simplicity's sake, but "real" interfaces exhibit the same behavior.

1

Simple property with context parameter:

interface SomeInterface {
    context(Unit)
    val someInnerProp: String get() = ""
}

error: expected token: 'context'; generated val instead

2

interface BaseInterface {
    context(Unit)
    open val someInnerProp: String get() = ""
}

interface SomeInterface {
    context(Unit)
    override val someInnerProp: String get() = ""
}

Both of them can, in isolation, cause this:

error: did not generate token "context"


A related issue I could find is #471, but that pertains to methods, not properties. If I read the KEEP correctly, these cases should be legal, albeit experimental, Kotlin.

davidtorosyan commented 1 month ago

I'm unclear on how much work we should be putting into supporting experimental features. On the one hand, why not, on the other hand things change around so the value is unclear.

Maybe I'm just questioning it because no matter how many times I read the spec, I can't understand what context receivers / parameters are :P


In this case we should probably put some effort in here, since we previously accepted changes related to this (#400).

In general, as long as we don't support line-by-line formatting, I'd feel bad crashing on stuff like this and preventing usage of ktfmt.


Will take a look, but also tagging @bddckr in case you're interested.

bddckr commented 1 month ago

Let's clarify some confusion here first:

Kotlin first introduced the experimental context receivers feature. By now it's been decided to change the design of those, which leads us to the context parameters feature that was linked above.

Context parameters aren't available yet - they're still in the design phase. So all we have right now is context receivers, which are kinda at an end-of-life stage since context parameters will eventually fully replace them:

[...] We understand that context receivers are already being used by a large number of developers. Therefore, we will begin gradually removing support for context receivers. Our migration plan starts with Kotlin 2.0.20, where warnings are issued in your code when context receivers are used with the -Xcontext-receivers compiler option. [...]

[...] Alternatively, you can wait until the Kotlin release where context parameters are supported in the compiler. Note that context parameters will initially be introduced as an Experimental feature.

https://kotlinlang.org/docs/whatsnew2020.html#phased-replacement-of-context-receivers-with-context-parameters


So my recommendation is to not put any further effort into supporting context receivers. Once context parameters are available, those could be supported - assuming it's checking the boxes for the maintainers to take on that responsibility. Especially considering that it will start as an experimental feature once again...

At a bare minimum, it would be great for ktfmt to not crash and somehow still work when using these features. I personally would be fine with these code parts being left unformatted and kept as-is, as long as the rest is formatted. I understand that's easier said than done, though.

koljakube commented 1 month ago

Thank you for the clarification @bddckr. Yes, it's true that receivers are on their way out. But context parameters are syntactically very similar (they mostly add a parameterName: to the context(SomeType) contents, so I would (naively, not knowing ktfmt's internals) expect you to be able to reuse a good part of the implementation.

I can work around this for now, so no pressure. But according to my research, ktfmt is the only reliable zero-config formatter for Kotlin out there. I think it might make sense to start work on supporting incubating syntax as soon as the stable language features are well supported, to provide formatting to the broadest spectrum of Kotlin code bases.

hick209 commented 1 month ago

@koljakube this might actually be a simple PR, do you want to take a shot at this?

koljakube commented 1 week ago

I would first need to look into the code to get an idea about how much work that would be, I don't have a lot of spare time at the moment.

But I won't sign your CLA, so I guess that disqualifies me.