Always use UTF8 as the encoding for Kotlin code, regardless of the system/default charset

[nreid260]

Closes #373

[nreid260]

@cgrushko I'm not sure I've set up the Maven stuff right. In order to test the default charset is ignored, I want to run the tests using UTF16, which requires passing a JVM startup flag. I know the test is right otherwise because I can get it working in Bazel

[cgrushko]

can we leave this to separate PR? lots of noise in the diff...

[nreid260]

Done

[cgrushko]

I feel this PR is doing multiple things; let's split into multiple PRs.

1. Fix the exact issue at hand - please don't change stderr into stdout or vice verse, some tools rely on text going into one or the other.
2. Add explicit UTF_8 everywhere - my vote is to avoid that. Kotlin uses utf8 by default. A test would be enough to catch problems. (same as we wouldn't explicitly use utf8 in golang).

[nreid260]

1. I've reverted the stdout writes, but it's weird. stdout now emits in two different charsets depending on the type of output.
2. Being explicit is the main point of this PR. The encoding was implicit when the code was originally written, so humans didn't think about what the behaviour should be, which lead to bugs. I want to trigger that consideration next time.

[facebook-github-bot]

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

[nreid260]

Hey @hick209, are you now the best contact person for PRs?

[hick209]

Maybe... 😅

I guess me @cgrushko, @davidtorosyan, @strulovich and @cortinico are the non-official-but-maybe-official-ish maintainers of ktfmt and it depends on who has bandwidth to tackle things.

I was trying to keep track and clean things up here, but then I went on paternity leave in September. Now that I'm back and was able to catch up, I'm trying to put some love into the project, at the very least managing GitHub issues and PRs.

You could also try to use the Slack channel we have tor conversations in case you don't want to do everything through GitHub as well, I guess only @davidtorosyan is not there.

[hick209]

Locally I see a bunch of failing tests from the class com.facebook.ktfmt.cli.MainTest.
@nreid260, would you mind rebasing and taking a look if you repro it as well?

[nreid260]

Locally I see a bunch of failing tests from the class com.facebook.ktfmt.cli.MainTest. @nreid260, would you mind rebasing and taking a look if you repro it as well?

It looks like only the plugin tests are failing, and that's been happening since f64da02

[hick209]

Are you saying that it passes on your local tests?
I think it is related to the fact that we use buck to test it and not Maven, which means we are not using the plugin you set here

ktfmt/core/pom.xml

Lines 140 to 148 in b394fe3

	<plugin>
	<groupId>org.apache.maven.plugins</groupId>
	<artifactId>maven-surefire-plugin</artifactId>
	<version>3.2.5</version>
	<configuration>
	<!-- Tests that everything works when using an unusual default Charset. -->
	<argLine>-Dfile.encoding=UTF-16</argLine>
	</configuration>
	</plugin>

This is what I get from running it locally from this PR and I've just rebased and checked that the base was clear

✗ Fail: fbsource//xplat/ktfmt/core/src/test/java/com/facebook/ktfmt/cli:cli - --dry-run prints nothing when there are no changes needed (file) (com.facebook.ktfmt.cli.MainTest) (0.1s)
expected: UTF-16
but was : UTF-8
at com.facebook.ktfmt.cli.MainTest.setUp(MainTest.kt:51)
✗ Fail: fbsource//xplat/ktfmt/core/src/test/java/com/facebook/ktfmt/cli:cli - all files in args are processed, even if one of them has an error (com.facebook.ktfmt.cli.MainTest) (0.1s)
expected: UTF-16
but was : UTF-8
at com.facebook.ktfmt.cli.MainTest.setUp(MainTest.kt:51)
✗ Fail: fbsource//xplat/ktfmt/core/src/test/java/com/facebook/ktfmt/cli:cli - Using '-' as the filename formats an InputStream (com.facebook.ktfmt.cli.MainTest) (0.1s)
expected: UTF-16
but was : UTF-8
at com.facebook.ktfmt.cli.MainTest.setUp(MainTest.kt:51)
✗ Fail: fbsource//xplat/ktfmt/core/src/test/java/com/facebook/ktfmt/cli:cli - --dry-run prints 'stdin' and does not reformat code from stdin (com.facebook.ktfmt.cli.MainTest) (0.1s)
expected: UTF-16
but was : UTF-8
at com.facebook.ktfmt.cli.MainTest.setUp(MainTest.kt:51)
✗ Fail: fbsource//xplat/ktfmt/core/src/test/java/com/facebook/ktfmt/cli:cli - Exit code is 0 when there are changes (file) (com.facebook.ktfmt.cli.MainTest) (0.1s)
expected: UTF-16
but was : UTF-8
at com.facebook.ktfmt.cli.MainTest.setUp(MainTest.kt:51)
✗ Fail: fbsource//xplat/ktfmt/core/src/test/java/com/facebook/ktfmt/cli:cli - dropbox-style is passed to formatter (file) (com.facebook.ktfmt.cli.MainTest) (0.1s)
expected: UTF-16
but was : UTF-8
at com.facebook.ktfmt.cli.MainTest.setUp(MainTest.kt:51)
✗ Fail: fbsource//xplat/ktfmt/core/src/test/java/com/facebook/ktfmt/cli:cli - formatting from stdin prints formatted code to stdout regardless of whether it was already formatted (com.facebook.ktfmt.cli.MainTest) (0.1s)
expected: UTF-16
but was : UTF-8
at com.facebook.ktfmt.cli.MainTest.setUp(MainTest.kt:51)
✗ Fail: fbsource//xplat/ktfmt/core/src/test/java/com/facebook/ktfmt/cli:cli - --set-exit-if-changed and --dry-run changes nothing, prints filenames, and exits with 1 (file) (com.facebook.ktfmt.cli.MainTest) (0.1s)
expected: UTF-16
but was : UTF-8
at com.facebook.ktfmt.cli.MainTest.setUp(MainTest.kt:51)
✗ Fail: fbsource//xplat/ktfmt/core/src/test/java/com/facebook/ktfmt/cli:cli - Exits with 0 when there are changes (stdin) (com.facebook.ktfmt.cli.MainTest) (0.1s)
expected: UTF-16
but was : UTF-8
at com.facebook.ktfmt.cli.MainTest.setUp(MainTest.kt:51)
✗ Fail: fbsource//xplat/ktfmt/core/src/test/java/com/facebook/ktfmt/cli:cli - Exit code is 1 when there are changes and --set-exit-if-changed is set (file) (com.facebook.ktfmt.cli.MainTest) (0.1s)
expected: UTF-16
but was : UTF-8
at com.facebook.ktfmt.cli.MainTest.setUp(MainTest.kt:51)
✗ Fail: fbsource//xplat/ktfmt/core/src/test/java/com/facebook/ktfmt/cli:cli - --dry-run prints nothing when there are no changes needed (stdin) (com.facebook.ktfmt.cli.MainTest) (0.1s)
expected: UTF-16
but was : UTF-8
at com.facebook.ktfmt.cli.MainTest.setUp(MainTest.kt:51)
✗ Fail: fbsource//xplat/ktfmt/core/src/test/java/com/facebook/ktfmt/cli:cli - Parsing errors are reported (file) (com.facebook.ktfmt.cli.MainTest) (0.1s)
expected: UTF-16
but was : UTF-8
at com.facebook.ktfmt.cli.MainTest.setUp(MainTest.kt:51)
✗ Fail: fbsource//xplat/ktfmt/core/src/test/java/com/facebook/ktfmt/cli:cli - Exit code is 1 when there are changes and --set-exit-if-changed is set (stdin) (com.facebook.ktfmt.cli.MainTest) (0.1s)
expected: UTF-16
but was : UTF-8
at com.facebook.ktfmt.cli.MainTest.setUp(MainTest.kt:51)
✗ Fail: fbsource//xplat/ktfmt/core/src/test/java/com/facebook/ktfmt/cli:cli - expandArgsToFileNames - single arg which is not a file is not returned (com.facebook.ktfmt.cli.MainTest) (0.1s)
expected: UTF-16
but was : UTF-8
at com.facebook.ktfmt.cli.MainTest.setUp(MainTest.kt:51)
✗ Fail: fbsource//xplat/ktfmt/core/src/test/java/com/facebook/ktfmt/cli:cli - dropbox-style is passed to formatter (stdin) (com.facebook.ktfmt.cli.MainTest) (0.1s)
expected: UTF-16
but was : UTF-8
at com.facebook.ktfmt.cli.MainTest.setUp(MainTest.kt:51)
✗ Fail: fbsource//xplat/ktfmt/core/src/test/java/com/facebook/ktfmt/cli:cli - file is modified if it is not formatted (com.facebook.ktfmt.cli.MainTest) (0.1s)
expected: UTF-16
but was : UTF-8
at com.facebook.ktfmt.cli.MainTest.setUp(MainTest.kt:51)
✗ Fail: fbsource//xplat/ktfmt/core/src/test/java/com/facebook/ktfmt/cli:cli - Always use UTF8 encoding (file) (com.facebook.ktfmt.cli.MainTest) (0.1s)
expected: UTF-16
but was : UTF-8
at com.facebook.ktfmt.cli.MainTest.setUp(MainTest.kt:51)
✗ Fail: fbsource//xplat/ktfmt/core/src/test/java/com/facebook/ktfmt/cli:cli - expandArgsToFileNames - multiple directory args are resolved to their recursively contained kt files (com.facebook.ktfmt.cli.MainTest) (0.1s)
expected: UTF-16
but was : UTF-8
at com.facebook.ktfmt.cli.MainTest.setUp(MainTest.kt:51)
✗ Fail: fbsource//xplat/ktfmt/core/src/test/java/com/facebook/ktfmt/cli:cli - expandArgsToFileNames - a dash is an error (com.facebook.ktfmt.cli.MainTest) (0.1s)
expected: UTF-16
but was : UTF-8
at com.facebook.ktfmt.cli.MainTest.setUp(MainTest.kt:51)
✗ Fail: fbsource//xplat/ktfmt/core/src/test/java/com/facebook/ktfmt/cli:cli - file is not modified if it is already formatted (com.facebook.ktfmt.cli.MainTest) (0.1s)
expected: UTF-16
but was : UTF-8
at com.facebook.ktfmt.cli.MainTest.setUp(MainTest.kt:51)
✗ Fail: fbsource//xplat/ktfmt/core/src/test/java/com/facebook/ktfmt/cli:cli - Parsing errors are reported (stdin-name) (com.facebook.ktfmt.cli.MainTest) (0.1s)
expected: UTF-16
but was : UTF-8
at com.facebook.ktfmt.cli.MainTest.setUp(MainTest.kt:51)
✗ Fail: fbsource//xplat/ktfmt/core/src/test/java/com/facebook/ktfmt/cli:cli - --set-exit-if-changed and --dry-run changes nothing, prints filenames, and exits with 1 (stdin) (com.facebook.ktfmt.cli.MainTest) (0.1s)
expected: UTF-16
but was : UTF-8
at com.facebook.ktfmt.cli.MainTest.setUp(MainTest.kt:51)
✗ Fail: fbsource//xplat/ktfmt/core/src/test/java/com/facebook/ktfmt/cli:cli - expandArgsToFileNames - resolves 'kt' and 'kts' filenames only (recursively) (com.facebook.ktfmt.cli.MainTest) (0.1s)
expected: UTF-16
but was : UTF-8
at com.facebook.ktfmt.cli.MainTest.setUp(MainTest.kt:51)
✗ Fail: fbsource//xplat/ktfmt/core/src/test/java/com/facebook/ktfmt/cli:cli - --dry-run prints filename and does not change file (com.facebook.ktfmt.cli.MainTest) (0.1s)
expected: UTF-16
but was : UTF-8
at com.facebook.ktfmt.cli.MainTest.setUp(MainTest.kt:51)
✗ Fail: fbsource//xplat/ktfmt/core/src/test/java/com/facebook/ktfmt/cli:cli - --stdin-name can only be used with stdin (com.facebook.ktfmt.cli.MainTest) (0.1s)
expected: UTF-16
but was : UTF-8
at com.facebook.ktfmt.cli.MainTest.setUp(MainTest.kt:51)
✗ Fail: fbsource//xplat/ktfmt/core/src/test/java/com/facebook/ktfmt/cli:cli - Parsing errors are reported (stdin) (com.facebook.ktfmt.cli.MainTest) (0.1s)
expected: UTF-16
but was : UTF-8
at com.facebook.ktfmt.cli.MainTest.setUp(MainTest.kt:51)
✗ Fail: fbsource//xplat/ktfmt/core/src/test/java/com/facebook/ktfmt/cli:cli - Always use UTF8 encoding (stdin, stdout) (com.facebook.ktfmt.cli.MainTest) (0.1s)
expected: UTF-16
but was : UTF-8
at com.facebook.ktfmt.cli.MainTest.setUp(MainTest.kt:51)
✗ Fail: fbsource//xplat/ktfmt/core/src/test/java/com/facebook/ktfmt/cli:cli - expandArgsToFileNames - single file arg is used as is (com.facebook.ktfmt.cli.MainTest) (0.1s)
expected: UTF-16
but was : UTF-8
at com.facebook.ktfmt.cli.MainTest.setUp(MainTest.kt:51)
✗ Fail: fbsource//xplat/ktfmt/core/src/test/java/com/facebook/ktfmt/cli:cli - expandArgsToFileNames - single arg which is a directory is resolved to its recursively contained kt files (com.facebook.ktfmt.cli.MainTest) (0.1s)
expected: UTF-16
but was : UTF-8
at com.facebook.ktfmt.cli.MainTest.setUp(MainTest.kt:51)

[hick209]

Would it be possible to rewrite the tests in a way that does not rely on that Maven plugin?

[nreid260]

In Bazel I wrote the test with jvm_flags = ["-Dfile.encoding=UTF-16"],. Maven is not my primary build system.

[hick209]

Oh, that's simple enough! Let try that with Buck here, then I'll just make the changes here before shipping it

[hick209]

Yep, that did it! No action item needed from you them, I now just need the sign in from someone from inside to merge this in

[facebook-github-bot]

@hick209 merged this pull request in c0e7db9.