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

Support context receivers #400

Closed
wants to merge 7 commits into from
Closed

Support context receivers #400

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8993576
Support context receivers
bddckr Apr 29, 2023
14e5f1d
Test with annotation and superfluous newlines instead
bddckr Apr 29, 2023
9c3d852
Fix context receiver on class not parsing
bddckr Apr 29, 2023
0694e1e
Remove blank lines after context receiver
bddckr Apr 30, 2023
d9aeecd
Use `visitEachCommaSeparated`'s prefix & postfix for enclosing params
bddckr Jun 19, 2023
8502fd5
Ensure comments can be inside `context`'s parentheses
bddckr Jun 19, 2023
e2fe9bd
Don't break before closing parenthesis in non-Google styles
bddckr Jun 19, 2023
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
2 changes: 1 addition & 1 deletion core/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

<properties>
<dokka.version>0.10.1</dokka.version>
<kotlin.version>1.6.10</kotlin.version>
<kotlin.version>1.6.20-M1</kotlin.version>
<kotlin.compiler.incremental>true</kotlin.compiler.incremental>
<kotlin.compiler.jvmTarget>1.8</kotlin.compiler.jvmTarget>
<main.class>com.facebook.ktfmt.cli.Main</main.class>
Expand Down
32 changes: 32 additions & 0 deletions core/src/main/java/com/facebook/ktfmt/format/KotlinInputAstVisitor.kt
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

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)
}

Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ import org.jetbrains.kotlin.psi.KtCollectionLiteralExpression
import org.jetbrains.kotlin.psi.KtConstantExpression
import org.jetbrains.kotlin.psi.KtConstructorDelegationCall
import org.jetbrains.kotlin.psi.KtContainerNode
import org.jetbrains.kotlin.psi.KtContextReceiverList
import org.jetbrains.kotlin.psi.KtContinueExpression
import org.jetbrains.kotlin.psi.KtDelegatedSuperTypeEntry
import org.jetbrains.kotlin.psi.KtDestructuringDeclaration
Expand Down Expand Up @@ -94,6 +95,7 @@ import org.jetbrains.kotlin.psi.KtQualifiedExpression
import org.jetbrains.kotlin.psi.KtReferenceExpression
import org.jetbrains.kotlin.psi.KtReturnExpression
import org.jetbrains.kotlin.psi.KtScript
import org.jetbrains.kotlin.psi.KtScriptInitializer
import org.jetbrains.kotlin.psi.KtSecondaryConstructor
import org.jetbrains.kotlin.psi.KtSimpleNameExpression
import org.jetbrains.kotlin.psi.KtStringTemplateExpression
Expand Down Expand Up @@ -124,6 +126,7 @@ import org.jetbrains.kotlin.psi.psiUtil.children
import org.jetbrains.kotlin.psi.psiUtil.getPrevSiblingIgnoringWhitespace
import org.jetbrains.kotlin.psi.psiUtil.startOffset
import org.jetbrains.kotlin.psi.psiUtil.startsWithComment
import org.jetbrains.kotlin.psi.stubs.elements.KtStubElementTypes

/** An AST visitor that builds a stream of {@link Op}s to format. */
class KotlinInputAstVisitor(
Expand Down Expand Up @@ -162,6 +165,7 @@ class KotlinInputAstVisitor(
builder.sync(function)
builder.block(ZERO) {
visitFunctionLikeExpression(
function.getStubOrPsiChild(KtStubElementTypes.CONTEXT_RECEIVER_LIST),
function.modifierList,
"fun",
function.typeParameterList,
Expand Down Expand Up @@ -282,6 +286,7 @@ class KotlinInputAstVisitor(
* list of supertypes.
*/
private fun visitFunctionLikeExpression(
contextReceiverList: KtContextReceiverList?,
modifierList: KtModifierList?,
keyword: String,
typeParameters: KtTypeParameterList?,
Expand All @@ -294,6 +299,9 @@ class KotlinInputAstVisitor(
typeOrDelegationCall: KtElement?,
) {
builder.block(ZERO) {
if (contextReceiverList != null) {
visitContextReceiverList(contextReceiverList)
}
if (modifierList != null) {
visitModifierList(modifierList)
}
Expand Down Expand Up @@ -1372,6 +1380,7 @@ class KotlinInputAstVisitor(

builder.block(ZERO) {
visitFunctionLikeExpression(
null,
accessor.modifierList,
accessor.namePlaceholder.text,
null,
Expand Down Expand Up @@ -1461,8 +1470,12 @@ class KotlinInputAstVisitor(

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

Choose a reason for hiding this comment

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

There's a method for this: getContextReceiverList()

val modifierList = classOrObject.modifierList
builder.block(ZERO) {
if (contextReceiverList != null) {
visitContextReceiverList(contextReceiverList)
}
if (modifierList != null) {
visitModifierList(modifierList)
}
Expand Down Expand Up @@ -1533,6 +1546,7 @@ class KotlinInputAstVisitor(

val delegationCall = constructor.getDelegationCall()
visitFunctionLikeExpression(
constructor.getStubOrPsiChild(KtStubElementTypes.CONTEXT_RECEIVER_LIST),
constructor.modifierList,
"constructor",
null,
Expand Down Expand Up @@ -1638,6 +1652,20 @@ class KotlinInputAstVisitor(
builder.forcedBreak()
}

/** Example `context(Logger, Raise<Error>)` */
override fun visitContextReceiverList(contextReceiverList: KtContextReceiverList) {
builder.sync(contextReceiverList)
builder.token("context")
visitEachCommaSeparated(
contextReceiverList.contextReceivers(),
prefix = "(",
postfix = ")",
breakAfterPrefix = false,
Copy link
Contributor

Choose a reason for hiding this comment

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

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".

breakBeforePostfix = false
)
builder.forcedBreak()
}

/** For example `@Magic private final` */
override fun visitModifierList(list: KtModifierList) {
builder.sync(list)
Expand Down Expand Up @@ -2427,6 +2455,7 @@ class KotlinInputAstVisitor(
override fun visitScript(script: KtScript) {
markForPartialFormat()
var lastChildHadBlankLineBefore = false
var lastChildIsContextReceiver = false
var first = true
for (child in script.blockExpression.children) {
if (child.text.isBlank()) {
Expand All @@ -2436,13 +2465,16 @@ class KotlinInputAstVisitor(
val childGetsBlankLineBefore = child !is KtProperty
if (first) {
builder.blankLineWanted(OpsBuilder.BlankLineWanted.PRESERVE)
} else if (lastChildIsContextReceiver) {
builder.blankLineWanted(OpsBuilder.BlankLineWanted.NO)
} else if (child !is PsiComment &&
(childGetsBlankLineBefore || lastChildHadBlankLineBefore)) {
builder.blankLineWanted(OpsBuilder.BlankLineWanted.YES)
}
visit(child)
builder.guessToken(";")
lastChildHadBlankLineBefore = childGetsBlankLineBefore
lastChildIsContextReceiver = child is KtScriptInitializer && child.firstChild?.firstChild?.firstChild?.text == "context"
first = false
}
markForPartialFormat()
Expand Down
42 changes: 42 additions & 0 deletions core/src/test/java/com/facebook/ktfmt/format/FormatterTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6674,6 +6674,48 @@ class FormatterTest {
assertThatFormatting(code).isEqualTo(expected)
}

@Test
fun `context receivers`() {
val code =
"""
|context(Something)
|
|class A {
| context(
| // Test comment.
| Logger, Raise<Error>)
|
| @SomeAnnotation
|
| fun doNothing() {}
|
| context(SomethingElse)
|
| private class NestedClass {}
|}
|"""
.trimMargin()

val expected =
"""
|context(Something)
|class A {
| context(
| // Test comment.
Copy link
Contributor

Choose a reason for hiding this comment

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

There should be some indentation here.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

| Logger,
| Raise<Error>)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

| @SomeAnnotation
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

| fun doNothing() {}
|
| context(SomethingElse)
| private class NestedClass {}
|}
|"""
.trimMargin()

assertThatFormatting(code).isEqualTo(expected)
}

companion object {
/** Triple quotes, useful to use within triple-quoted strings. */
private const val TQ = "\"\"\""
Expand Down
24 changes: 24 additions & 0 deletions core/src/test/java/com/facebook/ktfmt/format/TokenizerTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,4 +110,28 @@ class TokenizerTest {
.containsExactly(0, -1, 1, 2, 3, -1, 4, -1, 5, 6, 7)
.inOrder()
}

@Test
fun `Context receivers are parsed correctly`() {
val code = """
|context(Something)
|class A {
| context(
| // Test comment.
| Logger, Raise<Error>)
| fun test() {}
|}
|""".trimMargin().trimMargin()

val file = Parser.parse(code)
val tokenizer = Tokenizer(code, file)
file.accept(tokenizer)

assertThat(tokenizer.toks.map { it.originalText })
.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)
Copy link
Contributor

@davidtorosyan davidtorosyan Sep 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

.inOrder()
}
}