Skip to content

Commit

Permalink
Java Style: Ban Record, @autovalue. Discourage #toString()
Browse files Browse the repository at this point in the history 
Also reorganize the sections a bit.

Bug: 1493366
Change-Id: I7cb72ff8fed70cfa60ba8ae9d07cf04f5b9e7f11
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4980185
Commit-Queue: Andrew Grieve <agrieve@chromium.org>
Reviewed-by: Nate Fischer <ntfschr@chromium.org>
Reviewed-by: Peter Conn <peconn@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1219086}
  • Loading branch information
@agrieve
agrieve authored and Chromium LUCI CQ committed Nov 2, 2023
1 parent c54304e commit 99e388f
Show file tree
Hide file tree
Showing 2 changed files with 115 additions and 64 deletions.
10 changes: 9 additions & 1 deletion build/android/gyp/dex.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,15 @@ def _RunD8(dex_cmd, input_paths, output_path, warnings_as_errors,
build_utils.CheckOutput(dex_cmd,
stderr_filter=stderr_filter,
fail_on_output=warnings_as_errors)
except Exception:
except Exception as e:
if isinstance(e, build_utils.CalledProcessError):
output = e.output # pylint: disable=no-member
if "global synthetic for 'Record desugaring'" in output:
sys.stderr.write('Java records are not supported.\n')
sys.stderr.write(
'See https://chromium.googlesource.com/chromium/src/+/' +
'main/styleguide/java/java.md#Records\n')
sys.exit(1)
if orig_dex_cmd is not dex_cmd:
sys.stderr.write('Full command: ' + shlex.join(orig_dex_cmd) + '\n')
raise
Expand Down
169 changes: 106 additions & 63 deletions styleguide/java/java.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,9 @@ get to decide.

[TOC]

## Java 10 Language Features
## Java Language Features

### Type Deduction using `var`
### Type Deduction using "var" {#var}

A variable declaration can use the `var` keyword in place of the type (similar
to the `auto` keyword in C++). In line with the [guidance for
Expand All @@ -37,33 +37,8 @@ try (var ignored = StrictModeContext.allowDiskWrites()) {
}
```

## Java 8 Language Features

[D8] is used to rewrite some Java 7 & 8 language constructs in a way that is
compatible with Java 6 (and thus all Android versions). Use of [these features]
is encouraged.

[D8]: https://developer.android.com/studio/command-line/d8
[these features]: https://developer.android.com/studio/write/java8-support

## Java Library APIs

Android provides the ability to bundle copies of `java.` APIs alongside
application code, known as [Java Library Desugaring]. However, since this
bundling comes with a performance cost, Chrome does not use it. Treat `java.`
APIs the same as you would `android.` ones and guard them with
`Build.VERSION.SDK_INT` checks [when necessary]. The one exception is if the
method is [directly backported by D8] (these are okay to use, since they are
lightweight). Android Lint will fail if you try to use an API without a
corresponding `Build.VERSION.SDK_INT` guard or `@RequiresApi` annotation.

[Java Library Desugaring]: https://developer.android.com/studio/write/java8-support-table
[when necessary]: https://developer.android.com/reference/packages
[directly backported by D8]: https://source.chromium.org/chromium/chromium/src/+/main:third_party/r8/backported_methods.txt

## Other Language Features & APIs

### Exceptions

We discourage overly broad catches via `Throwable`, `Exception`, or
`RuntimeException`, except when dealing with `RemoteException` or similar
system APIs.
Expand All @@ -88,20 +63,6 @@ try {
}
```

### Logging

* Use `org.chromium.base.Log` instead of `android.util.Log`.
* It provides `%s` support, and ensures log stripping works correctly.
* Minimize the use of `Log.w()` and `Log.e()`.
* Debug and Info log levels are stripped by ProGuard in release builds, and
so have no performance impact for shipping builds. However, Warning and
Error log levels are not stripped.
* Function calls in log parameters are *not* stripped by ProGuard.

```java
Log.d(TAG, "There are %d cats", countCats()); // countCats() not stripped.
```

### Asserts

The build system:
Expand Down Expand Up @@ -143,26 +104,69 @@ if (BuildConfig.ENABLE_ASSERTS) {

[`BuildConfig.ENABLE_ASSERTS`]: https://source.chromium.org/search?q=symbol:BuildConfig%5C.ENABLE_ASSERTS

#### DCHECKS vs Java Asserts
#### DCHECKS vs Java Asserts {#asserts}

`DCHECK` and `assert` are similar, but our guidance for them differs:
* CHECKs are preferred in C++, whereas asserts are preferred in Java.

This is because as a memory-safe language, logic bugs in Java are much less
likely to be exploitable.

### Streams
### toString() {#toString}

Most uses of [Java 8 streams] are discouraged. If you can write your code as an
explicit loop, then do so. The primary reason for this guidance is because the
lambdas (and method references) needed for streams almost always result in
larger binary size ([example](https://chromium-review.googlesource.com/c/chromium/src/+/4329952).
Use explicit serialization methods (e.g. `toDebugString()` or `getDescription()`)
instead of `toString()` when dynamic dispatch is not required.

The `parallel()` and `parallelStream()` APIs are simpler than their loop
equivalents, but are are currently banned due to a lack of a compelling use case
in Chrome. If you find one, please discuss on `java@chromium.org`.
1. R8 cannot detect when `toString()` is unused, so overrides will not be stripped
when unused.
2. R8 cannot optimize / inline these calls as well as non-overriding methods.

[Java 8 streams]: https://docs.oracle.com/javase/8/docs/api/java/util/stream/package-summary.html
### Records & AutoValue {#records}

```java
// Banned.
record Rectangle(float length, float width) {}
```

**Rationale:**
* To avoid dead code:
* Records and `@AutoValue` generate `equals()`, `hashCode()`, and `toString()`,
which `R8` is unable to remove when unused.
* When these methods are required, implement them explicitly so that the
intention is clear.
* Also - supporting `record` requires build system work ([crbug/1493366]).

Example with `equals()` and `hashCode()`:

```java
public class ValueClass {
private final SomeClass mObjMember;
private final int mIntMember;

@Override
public boolean equals(Object o) {
return o instanceof ValueClass vc
&& Objects.equals(mObjMember, vc.mObjMember)
&& mIntMember == vc.mIntMember;
}

@Override
public int hashCode() {
return Objects.hash(mObjMember, mIntMember);
}
}
```

[crbug/1493366]: https://crbug.com/1493366

### Enums

Banned. Use [`@IntDef`](#intdefs) instead.

**Rationale:**

Java enums generate a lot of bytecode. Use constants where possible. When a
custom type hierarchy is required, use explicit classes with inheritance.

### Finalizers

Expand All @@ -181,23 +185,61 @@ to ensure in debug builds and tests that `destroy()` is called.
[Google's Java style guide]: https://google.github.io/styleguide/javaguide.html#s6.4-finalizers
[Android's Java style guide]: https://source.android.com/docs/setup/contribute/code-style#dont-use-finalizers

### AndroidX Annotations
## Java Library APIs

* Use them! They are [documented here](https://developer.android.com/studio/write/annotations).
Android provides the ability to bundle copies of `java.*` APIs alongside
application code, known as [Java Library Desugaring]. However, since this
bundling comes with a performance cost, Chrome does not use it. Treat `java.*`
APIs the same as you would `android.*` ones and guard them with
`Build.VERSION.SDK_INT` checks [when necessary]. The one exception is if the
method is [directly backported by D8] (these are okay to use, since they are
lightweight). Android Lint will fail if you try to use an API without a
corresponding `Build.VERSION.SDK_INT` guard or `@RequiresApi` annotation.

[Java Library Desugaring]: https://developer.android.com/studio/write/java8-support-table
[when necessary]: https://developer.android.com/reference/packages
[directly backported by D8]: https://source.chromium.org/chromium/chromium/src/+/main:third_party/r8/backported_methods.txt

### Logging

* Use `org.chromium.base.Log` instead of `android.util.Log`.
* It provides `%s` support, and ensures log stripping works correctly.
* Minimize the use of `Log.w()` and `Log.e()`.
* Debug and Info log levels are stripped by ProGuard in release builds, and
so have no performance impact for shipping builds. However, Warning and
Error log levels are not stripped.
* Function calls in log parameters are *not* stripped by ProGuard.

```java
Log.d(TAG, "There are %d cats", countCats()); // countCats() not stripped.
```

### Streams

Most uses of [Java streams] are discouraged. If you can write your code as an
explicit loop, then do so. The primary reason for this guidance is because the
lambdas (and method references) needed for streams almost always result in
larger binary size ([example](https://chromium-review.googlesource.com/c/chromium/src/+/4329952).

The `parallel()` and `parallelStream()` APIs are simpler than their loop
equivalents, but are are currently banned due to a lack of a compelling use case
in Chrome. If you find one, please discuss on `java@chromium.org`.

[Java streams]: https://docs.oracle.com/javase/8/docs/api/java/util/stream/package-summary.html

### AndroidX Annotations {#annotations}

* Use them liberally. They are [documented here](https://developer.android.com/studio/write/annotations).
* They generally improve readability.
* Some make lint more useful.
* Many make lint more useful.
* `javax.annotation.Nullable` vs `androidx.annotation.Nullable`
* Always prefer `androidx.annotation.Nullable`.
* It uses `@Retention(SOURCE)` rather than `@Retention(RUNTIME)`.

### IntDef Instead of Enum
#### IntDefs {#intdefs}

Java enums generate far more bytecode than integer constants. When integers are
sufficient, prefer using an [@IntDef annotation], which will have usage checked
by [Android lint].

Values can be declared outside or inside the `@interface`. We recommend the
latter, with constants nested within it as follows:
Values can be declared outside or inside the `@interface`. Chromium style is
to declare inside.

```java
@IntDef({ContactsPickerAction.CANCEL, ContactsPickerAction.CONTACTS_SELECTED,
Expand All @@ -220,7 +262,8 @@ Values of `Integer` type are also supported, which allows using a sentinel
[@IntDef annotation]: https://developer.android.com/studio/write/annotations#enum-annotations
[Android lint]: https://chromium.googlesource.com/chromium/src/+/HEAD/build/android/docs/lint.md

## Style / Formatting

## Style / Formatting {#style}

### File Headers
* Use the same format as in the [C++ style guide](https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/c++.md#File-headers).
Expand Down

0 comments on commit 99e388f

Please sign in to comment.