Support context receivers #400

bddckr · 2023-04-29T16:32:07Z

bddckr · 2023-04-29T16:33:46Z

I mostly have no idea what I'm doing 😅, but the added tests pass with these changes.

Please let me know if I'm missing anything. In particular, I've not been able to find how to lint my changes, as requested by the contributing docs.

nreid260 · 2023-05-12T05:45:38Z

core/src/main/java/com/facebook/ktfmt/format/KotlinInputAstVisitor.kt

+    builder.sync(contextReceiverList)
+    builder.token("context")
+    builder.token("(")
+    visitEachCommaSeparated(contextReceiverList.contextReceivers())


visitEachCommaSeparated has parameters for enclosing parens, to handle some weird indentation cases.

Pushed a commit to use that, thank you for pointing that out!

nreid260 · 2023-05-12T05:47:02Z

core/src/test/java/com/facebook/ktfmt/format/TokenizerTest.kt

+  fun `Context receivers are parsed correctly`() {
+    val code = """
+      |class A {
+      |  context(Logger, Raise<Error>)


What happens if the list of receivers is longer than the max line length?

What happens if there's a trailing comma?

What happens to comments inside the parens?

What happens if there's a trailing comma?

That's a compile error giving error: Type expected. Correct me if I'm wrong, but ktfmt should fail in that case and not try to parse/format when a compile error is encountered.

What happens if the list of receivers is longer than the max line length?
What happens to comments inside the parens?

I've changed the test to include a comment, which also shows that the regular wrapping logic works as it goes over the test's max line length.

Thanks. It's very weird to me that trailing commas aren't accepted in the context receiver list. I suspect that's a bug in kotlinc, but nothing we can do right now.

https://youtrack.jetbrains.com/issue/KT-59506/Trailing-commas-are-illegal-in-context-receiver-lists

nreid260 · 2023-06-19T16:27:39Z

core/src/main/java/com/facebook/ktfmt/format/KotlinInputAstVisitor.kt

@@ -1461,8 +1470,12 @@ class KotlinInputAstVisitor(

  override fun visitClassOrObject(classOrObject: KtClassOrObject) {
    builder.sync(classOrObject)
+    val contextReceiverList = classOrObject.getStubOrPsiChild(KtStubElementTypes.CONTEXT_RECEIVER_LIST)


There's a method for this: getContextReceiverList()

nreid260 · 2023-06-19T16:35:11Z

core/src/test/java/com/facebook/ktfmt/format/FormatterTest.kt

+      |context(Something)
+      |class A {
+      |  context(
+      |  // Test comment.


There should be some indentation here.

+1 to this and your other comments in this test. Don't think any of it is blocking though.

nreid260 · 2023-06-19T16:37:12Z

core/src/main/java/com/facebook/ktfmt/format/KotlinInputAstVisitor.kt

+            contextReceiverList.contextReceivers(),
+            prefix = "(",
+            postfix = ")",
+            breakAfterPrefix = false,


You probably don't want to specify these. They override the defaults to say "never break before or after the prefix, regardless of line length".

nreid260 · 2023-06-19T16:41:19Z

core/src/test/java/com/facebook/ktfmt/format/FormatterTest.kt

+      |  // Test comment.
+      |  Logger,
+      |  Raise<Error>)
+      |  @SomeAnnotation


If the context receivers break to multiple lines, I'd expect the list of annotations to also break. The repo owners may disagree though.

Either way, there should be a test for the case of multiple annotations.

nreid260 · 2023-06-19T16:43:11Z

core/src/main/java/com/facebook/ktfmt/format/KotlinInputAstVisitor.kt

I may have missed it, but there don't seem to be changes for context receivers on lambda parameters:

fun foo(cb: context(String) (Int) -> Unit) { cb("", 0) }

nreid260 · 2023-06-19T16:44:31Z

core/src/test/java/com/facebook/ktfmt/format/FormatterTest.kt

+      |  context(
+      |  // Test comment.
+      |  Logger,
+      |  Raise<Error>)


This is very subtle, but I'm specifically curious what happens when there's a comment between the last receiver type and the paren. That positioning is often buggy.

JavierSegoviaCordoba · 2023-07-06T13:17:48Z

Even the context syntax will have more problems, currently, the main issue is the space generated between context and the function itself. I have not ever seen issues when it is combined with annotations, or when it is used on lambdas (fun foo(block: context(Foo, Bar) () -> Unit) or adding basic comments to them.

I haven't seen any crashes too which could be a real problem.

Can we get a quick fix for the main issue and improve it later by merging this PR soon?

Even Kotlin itself has issues that need to be fixed, for example, trailing commas are not working, annotation position is affecting and it will not affect in the future (I think), and so on. So spending a lot of time when maybe in the future the feature is changed a bit cannot be productive too.

I use ktfmt even in playground projects without any other plugin than the IDE one, and it is really annoying to fix those issues manually as pressing cmd + alt + L is automatic.

facebook-github-bot · 2023-08-08T21:22:00Z

@hick209 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

davidtorosyan · 2023-09-13T20:35:16Z

core/src/test/java/com/facebook/ktfmt/format/TokenizerTest.kt

+            .containsExactly("context", "(", "Something", ")", "\n", "class", " ", "A", " ", "{", "\n", "  ", "context", "(", "\n", "  ", "// Test comment.", "\n", "  ", "Logger", ",", " ", "Raise", "<", "Error", ">", ")", "\n", "  ", "fun", " ", "test", "(", ")", " ", "{", "}", "\n", "}")
+            .inOrder()
+    assertThat(tokenizer.toks.map { it.index })
+            .containsExactly(0, 1, 2, 3, -1, 4, -1, 5, -1, 6, -1, -1, 7, 8, -1, -1, 9, -1, -1, 10, 11, -1, 12, 13, 14, 15, 16, -1, -1, 17, -1, 18, 19, 20, -1, 21, 22, -1, 23)


nit: not a problem for this PR, but this unit test file isn't very maintainable. We should add some helpers to make it so that it's clear what's actually being tested, and how the numbers relate to the strings.

facebook-github-bot · 2023-09-13T21:37:07Z

@hick209 merged this pull request in 17d80bf.

bddckr · 2023-09-14T05:04:37Z

Thanks for merging this as-is for now!

Sadly, I've not had time yet to come back to this to clean up and split the tests, add more test cases based on the raised PR review comments, and handle more scenarios. Depending on how soon someone else might get to that, I'm still interested in contributing further improvements. However, I may not get to that before the end of this year.

* upstream/main: Add unit tests to capture line break behavior on type specifiers Plugin doesn't work with if "Only VCS changed text" is selected from code-reformat settings (facebook#386) Bump version to 0.47-SNAPSHOT Bump version to 0.46 Fix indentation of trailing comments in a bunch of cases (facebook#418) Adjust .editorconfig for kotlinlang style for IntelliJ to better align with ktfmt (facebook#412) Bump Kotlin version to 1.8.22 Bump version to 0.46-SNAPSHOT Bump version to 0.45 Bump word-wrap from 1.2.3 to 1.2.4 in /website (facebook#410) Use inExpression in a nullsafe way (facebook#417) Update ktfmt component on FBS:master Back out "Improve argsfile support" Improve argsfile support Fix double indentation in Elvis chains (facebook#416) Daily `arc lint --take KTFMT` Remove TypeNameClassifier Support context receivers (facebook#400) Added link to live playground directly into README file Keep imports from the same package if the name is overloaded (facebook#414)

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 29, 2023

nreid260 reviewed May 12, 2023

View reviewed changes

bddckr added 4 commits June 19, 2023 13:43

Support context receivers

8993576

Test with annotation and superfluous newlines instead

14e5f1d

Fix context receiver on class not parsing

9c3d852

Remove blank lines after context receiver

0694e1e

bddckr force-pushed the context-receivers branch from 1675a29 to 0694e1e Compare June 19, 2023 11:43

bddckr added 2 commits June 19, 2023 13:55

Use visitEachCommaSeparated's prefix & postfix for enclosing params

d9aeecd

Ensure comments can be inside context's parentheses

8502fd5

bddckr requested a review from nreid260 June 19, 2023 12:12

Don't break before closing parenthesis in non-Google styles

e2fe9bd

nreid260 reviewed Jun 19, 2023

View reviewed changes

davidtorosyan reviewed Sep 13, 2023

View reviewed changes

facebook-github-bot closed this in 17d80bf Sep 13, 2023

facebook-github-bot added the Merged label Sep 13, 2023

This was referenced Sep 18, 2023

context receivers support #314

Closed

Format about context receiver #374

Closed

liutikas mentioned this pull request Sep 18, 2023

ktfmt 0.45 is shipping against ancient milestone version of Kotlin #421

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support context receivers #400

Support context receivers #400

bddckr commented Apr 29, 2023 •

edited

Loading

bddckr commented Apr 29, 2023

nreid260 May 12, 2023

bddckr Jun 19, 2023

nreid260 May 12, 2023 •

edited

Loading

bddckr Jun 19, 2023

nreid260 Jun 19, 2023 •

edited

Loading

nreid260 Jun 19, 2023

nreid260 Jun 19, 2023

davidtorosyan Sep 13, 2023

nreid260 Jun 19, 2023

nreid260 Jun 19, 2023

nreid260 Jun 19, 2023

nreid260 Jun 19, 2023

JavierSegoviaCordoba commented Jul 6, 2023

facebook-github-bot commented Aug 8, 2023

davidtorosyan Sep 13, 2023 •

edited

Loading

facebook-github-bot commented Sep 13, 2023

bddckr commented Sep 14, 2023

Support context receivers #400

Support context receivers #400

Conversation

bddckr commented Apr 29, 2023 • edited Loading

bddckr commented Apr 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nreid260 May 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nreid260 Jun 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JavierSegoviaCordoba commented Jul 6, 2023

facebook-github-bot commented Aug 8, 2023

davidtorosyan Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

facebook-github-bot commented Sep 13, 2023

bddckr commented Sep 14, 2023

bddckr commented Apr 29, 2023 •

edited

Loading

nreid260 May 12, 2023 •

edited

Loading

nreid260 Jun 19, 2023 •

edited

Loading

davidtorosyan Sep 13, 2023 •

edited

Loading