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

Jump to bottom

Fix a bunch of build warnings #283

Closed
wants to merge 2 commits into from
Closed

Fix a bunch of build warnings #283

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8af27d2
Fix a bunch of build warnings
nreid260 Feb 13, 2022
a31864f
Merge branch 'facebookincubator:main' into fix_warnings
nreid260 Apr 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
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 @@ -148,7 +148,7 @@
</dependency>
<dependency>
<groupId>org.jetbrains.kotlin</groupId>
<artifactId>kotlin-stdlib</artifactId>
<artifactId>kotlin-stdlib-jdk7</artifactId>
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the build warning here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this is needed for the changes to core/src/test/java/com/facebook/ktfmt/cli/MainTest.kt

<version>${kotlin.version}</version>
</dependency>
<dependency>
Expand Down
19 changes: 10 additions & 9 deletions core/src/main/java/com/facebook/ktfmt/format/KotlinInput.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import com.google.googlejavaformat.Input
import com.google.googlejavaformat.Newlines
import com.google.googlejavaformat.java.FormatterException
import com.google.googlejavaformat.java.JavaOutput
import java.util.LinkedHashMap
import org.jetbrains.kotlin.com.intellij.openapi.util.text.StringUtil
import org.jetbrains.kotlin.lexer.KtTokens
import org.jetbrains.kotlin.psi.KtFile
Expand Down Expand Up @@ -107,7 +108,6 @@ class KotlinInput(private val text: String, file: KtFile) : Input() {
*/
@Throws(FormatterException::class)
internal fun characterRangeToTokenRange(offset: Int, length: Int): Range<Int> {
var length = length
val requiredLength = offset + length
if (requiredLength > text.length) {
throw FormatterException(
Expand All @@ -116,14 +116,15 @@ class KotlinInput(private val text: String, file: KtFile) : Input() {
length,
requiredLength))
}
when {
length < 0 -> return EMPTY_RANGE
length == 0 -> // 0 stands for "format the line under the cursor"
length = 1
}
val expandedLength =
when {
length < 0 -> return EMPTY_RANGE
length == 0 -> 1 // 0 stands for "format the line under the cursor"
else -> length
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be else -> 1, at least to preserve the current behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should preserve the current behaviour. length = 1 isn't a case, it's the behaviour when length == 0.

Copy link
Contributor

Choose a reason for hiding this comment

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

/facepalm

}
val enclosed =
getPositionTokenMap()
.subRangeMap(Range.closedOpen(offset, offset + length))
.subRangeMap(Range.closedOpen(offset, offset + expandedLength))
.asMapOfRanges()
.values
return if (enclosed.isEmpty()) {
Expand All @@ -134,11 +135,11 @@ class KotlinInput(private val text: String, file: KtFile) : Input() {
}

private fun makePositionToColumnMap(toks: List<KotlinTok>): ImmutableMap<Int, Int> {
val builder = ImmutableMap.builder<Int, Int>()
val builder = LinkedHashMap<Int, Int>()
for (tok in toks) {
builder.put(tok.position, tok.column)
}
return builder.build()
return ImmutableMap.copyOf(builder)
Comment on lines -137 to +142
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the build warning here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ImmutableMap.Builder.build() throws if there are are duplicate keys. This caused enough confusion that a preferred, explicit method buildOrThrow was added in Guava 31.0. Google builds from the newest version of Guava. https://guava.dev/releases/31.0.1-jre/api/docs/com/google/common/collect/ImmutableMap.Builder.html#build()

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it. I think it's fine this time, but we'll keep running into these problems, since Meta / OSS isn't aware of Guava 31.
Don't know if policy allows that, but you may want to consider adding Guava 29 to third-party (and limiting its visibility to ktfmt).

}

private fun buildToks(file: KtFile, fileText: String): ImmutableList<KotlinTok> {
Expand Down
3 changes: 1 addition & 2 deletions core/src/main/java/com/facebook/ktfmt/format/KotlinInputAstVisitor.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1341,8 +1341,7 @@ class KotlinInputAstVisitor(
builder.open(ZERO)
builder.block(blockIndent) {
builder.breakOp(Doc.FillMode.UNIFIED, "", ZERO)
val (enumEntries, nonEnumEntryStatements) =
body?.children?.partition { it is KtEnumEntry } ?: fail()
val (enumEntries, nonEnumEntryStatements) = body.children.partition { it is KtEnumEntry }
builder.forcedBreak()
visitEnumEntries(enumEntries)

Expand Down
1 change: 0 additions & 1 deletion core/src/main/java/com/facebook/ktfmt/format/Tokenizer.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,6 @@ class Tokenizer(private val fileText: String, val file: KtFile) : KtTreeVisitorV
}
is LeafPsiElement -> {
val elementText = element.text
val startIndex = element.startOffset
val endIndex = element.endOffset
if (element is PsiWhiteSpace) {
val matcher = WHITESPACE_NEWLINE_REGEX.matcher(elementText)
Expand Down
10 changes: 5 additions & 5 deletions core/src/main/java/com/facebook/ktfmt/format/TypeNameClassifier.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,16 +124,16 @@ object TypeNameClassifier {
var hasUppercase = false
var hasLowercase = false
var first = true
for (element in name) {
if (!Character.isAlphabetic(element.toInt())) {
for (char in name) {
if (!Character.isAlphabetic(char.code)) {
continue
}
if (first) {
firstUppercase = Character.isUpperCase(element)
firstUppercase = Character.isUpperCase(char)
first = false
}
hasUppercase = hasUppercase or Character.isUpperCase(element)
hasLowercase = hasLowercase or Character.isLowerCase(element)
hasUppercase = hasUppercase or Character.isUpperCase(char)
hasLowercase = hasLowercase or Character.isLowerCase(char)
Comment on lines -127 to +136
Copy link
Contributor

Choose a reason for hiding this comment

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

If you're changing this how about using the Kotlin extension methods?

}
return if (firstUppercase) {
if (hasLowercase) UPPER_CAMEL else UPPERCASE
Expand Down
9 changes: 4 additions & 5 deletions core/src/main/java/com/facebook/ktfmt/kdoc/KDocCommentsHelper.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,13 +86,12 @@ class KDocCommentsHelper(private val lineSeparator: String, private val maxLineL

// Wraps and re-indents line comments.
private fun indentLineComments(lines: List<String>, column0: Int): String {
var lines = lines
lines = wrapLineComments(lines, column0)
val wrappedLines = wrapLineComments(lines, column0)
val builder = StringBuilder()
builder.append(lines[0].trim())
builder.append(wrappedLines[0].trim())
val indentString = Strings.repeat(" ", column0)
for (i in 1 until lines.size) {
builder.append(lineSeparator).append(indentString).append(lines[i].trim())
for (i in 1 until wrappedLines.size) {
builder.append(lineSeparator).append(indentString).append(wrappedLines[i].trim())
}
return builder.toString()
}
Expand Down
3 changes: 2 additions & 1 deletion core/src/test/java/com/facebook/ktfmt/cli/MainTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import java.io.PrintStream
import java.lang.IllegalStateException
import java.nio.file.Files
import java.util.concurrent.ForkJoinPool
import kotlin.io.path.createTempDirectory
import org.junit.After
import org.junit.Assert.fail
import org.junit.Test
Expand All @@ -33,7 +34,7 @@ import org.junit.runners.JUnit4
@RunWith(JUnit4::class)
class MainTest {

private val root = createTempDir()
private val root = createTempDirectory().toFile()
Comment on lines -36 to +37
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the issue here?

Copy link
Contributor

Choose a reason for hiding this comment

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

createTempDir is marked deprecated


private val emptyInput = "".byteInputStream()
private val out = ByteArrayOutputStream()
Expand Down
2 changes: 1 addition & 1 deletion 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 @@ -2229,7 +2229,7 @@ class FormatterTest {
deduceMaxWidth = true)

@Test
fun `handle ? for nullalble types`() =
fun `handle qmark for nullalble types`() =
Copy link
Contributor

Choose a reason for hiding this comment

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

"question mark" please

Let's avoid abbreviations for easier readability

assertFormatted(
"""
|fun doItWithNullReturns(a: String, b: String): Int? {
Expand Down