From e1be664048029d9c5bc7ed2460aa9a6c18c51b2f Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 19 Jul 2025 17:08:30 +0000 Subject: [PATCH 1/4] Initial plan From 052da6e2be751795b1049adddb2c8e002f0e785c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 19 Jul 2025 17:22:39 +0000 Subject: [PATCH 2/4] Sync errorprone configuration with OpenTelemetry instrumentation Co-authored-by: trask <218610+trask@users.noreply.github.com> --- .../ai.errorprone-conventions.gradle.kts | 117 ++++++++++++++---- settings.gradle.kts | 1 + 2 files changed, 94 insertions(+), 24 deletions(-) diff --git a/buildSrc/src/main/kotlin/ai.errorprone-conventions.gradle.kts b/buildSrc/src/main/kotlin/ai.errorprone-conventions.gradle.kts index b07b25d9e00..429558edad1 100644 --- a/buildSrc/src/main/kotlin/ai.errorprone-conventions.gradle.kts +++ b/buildSrc/src/main/kotlin/ai.errorprone-conventions.gradle.kts @@ -9,6 +9,7 @@ dependencies { } val disableErrorProne = properties["disableErrorProne"]?.toString()?.toBoolean() ?: false +val testLatestDeps = gradle.startParameter.projectProperties["testLatestDeps"] == "true" tasks { withType().configureEach { @@ -22,57 +23,125 @@ tasks { disableWarningsInGeneratedCode.set(true) allDisabledChecksAsWarnings.set(true) - excludedPaths.set(".*/build/generated/.*") + // Ignore warnings for generated and vendored classes + excludedPaths.set(".*/build/generated/.*|.*/concurrentlinkedhashmap/.*") - if (System.getenv("CI") == null) { - disable("SystemOut") - } - - // Still Java 8 - disable("Varifier") + // it's very convenient to debug stuff in the javaagent using System.out.println + // and we don't want to conditionally only check this in CI + // because then the remote gradle cache won't work for local builds + // so we check this via checkstyle instead + disable("SystemOut") - // Intellij does a nice job of displaying parameter names disable("BooleanParameter") - // Needed for legacy 2.x bridge - disable("JavaUtilDate") + // We often override a method returning Iterable which this makes tedious + // for questionable value. + disable("PreferredInterfaceType") // Doesn't work well with Java 8 disable("FutureReturnValueIgnored") - // Needs Java 9+ - disable("JavaDurationGetSecondsToToSeconds") + // Still Java 8 + disable("Varifier") + + // Doesn't currently use Var annotations. + disable("Var") // "-Xep:Var:OFF" + + // ImmutableRefactoring suggests using com.google.errorprone.annotations.Immutable, + // but currently uses javax.annotation.concurrent.Immutable + disable("ImmutableRefactoring") - // Require Guava + // AutoValueImmutableFields suggests returning Guava types from API methods disable("AutoValueImmutableFields") - disable("StringSplitter") + // Suggests using Guava types for fields but we don't use Guava disable("ImmutableMemberCollection") - // Don't currently use this (to indicate a local variable that's mutated) but could - // consider for future. - disable("Var") + // Fully qualified names may be necessary when deprecating a class to avoid + // deprecation warning. + disable("UnnecessarilyFullyQualified") - // Don't support Android without desugar + // TODO (trask) use animal sniffer + disable("Java8ApiChecker") disable("AndroidJdkLibsChecker") + + // apparently disabling android doesn't disable this disable("StaticOrDefaultInterfaceMethod") - // needed temporarily while hosting azure-monitor-opentelemetry-exporter in this repo - disable("MissingSummary") - disable("UnnecessaryDefaultInEnumSwitch") - disable("InconsistentOverloads") + // We don't depend on Guava so use normal splitting + disable("StringSplitter") + + // Prevents lazy initialization + disable("InitializeInline") + + // Seems to trigger even when a deprecated method isn't called anywhere. + // We don't get much benefit from it anyways. + disable("InlineMeSuggester") + + disable("DoNotCallSuggester") + + // We have nullaway so don't need errorprone nullable checks which have more false positives. + disable("FieldMissingNullable") + disable("ParameterMissingNullable") + disable("ReturnMissingNullable") + disable("VoidMissingNullable") + + // allow UPPERCASE type parameter names + disable("TypeParameterNaming") + + // In bytecode instrumentation it's very common to separate across onEnter / onExit + // TODO: Only disable for javaagent instrumentation modules. + disable("MustBeClosedChecker") + + // Common to avoid an allocation. Revisit if it's worth opt-in suppressing instead of + // disabling entirely. + disable("MixedMutabilityReturnType") + + // We end up using obsolete types if a library we're instrumenting uses them. + disable("JdkObsolete") + disable("JavaUtilDate") + + // TODO: Remove this, we use this pattern in several tests and it will mean some moving. + disable("DefaultPackage") + + // We don't have custom checks like OpenTelemetry, so disable the standard utility class check + disable("PrivateConstructorForUtilityClass") disable("CanIgnoreReturnValueSuggester") - disable("NonFinalStaticField") + // TODO: Remove this, probably after instrumenter API migration instead of dealing with + // older APIs. + disable("InconsistentOverloads") + + // lots of low level APIs use arrays + disable("AvoidObjectArrays") + + disable("BanClassLoader") // YodaConditions may improve safety in some cases. The argument of increased // cognitive load is dubious. disable("YodaCondition") + disable("NonFinalStaticField") + // Requires adding compile dependency to JSpecify disable("AddNullMarkedToPackageInfo") - if (name.contains("Jmh")) { + // needed temporarily while hosting azure-monitor-opentelemetry-exporter in this repo + disable("MissingSummary") + disable("UnnecessaryDefaultInEnumSwitch") + + // Needs Java 9+ + disable("JavaDurationGetSecondsToToSeconds") + + if (testLatestDeps) { + // Some latest dep tests are compiled for java 17 although the base version uses an older + // version. Disable rules that suggest using new language features. + disable("StatementSwitchToExpressionSwitch") + disable("PatternMatchingInstanceof") + } + + if (name.contains("Jmh") || name.contains("Test")) { + // Allow underscore in test-type method names disable("MemberName") } } diff --git a/settings.gradle.kts b/settings.gradle.kts index b9f5f411631..6f8af6bb4ad 100644 --- a/settings.gradle.kts +++ b/settings.gradle.kts @@ -2,6 +2,7 @@ import org.apache.tools.ant.taskdefs.condition.Os pluginManagement { plugins { + id("com.github.ben-manes.versions") version "0.52.0" id("com.github.jk1.dependency-license-report") version "2.9" id("me.champeau.jmh") version "0.7.3" id("com.gradle.plugin-publish") version "1.3.1" From 44ea517ef66e44326d8f19c8709a4fa693a49365 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 19 Jul 2025 17:43:00 +0000 Subject: [PATCH 3/4] Address review feedback for ErrorProne configuration Co-authored-by: trask <218610+trask@users.noreply.github.com> --- .../main/kotlin/ai.errorprone-conventions.gradle.kts | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/buildSrc/src/main/kotlin/ai.errorprone-conventions.gradle.kts b/buildSrc/src/main/kotlin/ai.errorprone-conventions.gradle.kts index 429558edad1..57328bb11c6 100644 --- a/buildSrc/src/main/kotlin/ai.errorprone-conventions.gradle.kts +++ b/buildSrc/src/main/kotlin/ai.errorprone-conventions.gradle.kts @@ -9,7 +9,6 @@ dependencies { } val disableErrorProne = properties["disableErrorProne"]?.toString()?.toBoolean() ?: false -val testLatestDeps = gradle.startParameter.projectProperties["testLatestDeps"] == "true" tasks { withType().configureEach { @@ -23,8 +22,8 @@ tasks { disableWarningsInGeneratedCode.set(true) allDisabledChecksAsWarnings.set(true) - // Ignore warnings for generated and vendored classes - excludedPaths.set(".*/build/generated/.*|.*/concurrentlinkedhashmap/.*") + // Ignore warnings for generated classes + excludedPaths.set(".*/build/generated/.*") // it's very convenient to debug stuff in the javaagent using System.out.println // and we don't want to conditionally only check this in CI @@ -133,13 +132,6 @@ tasks { // Needs Java 9+ disable("JavaDurationGetSecondsToToSeconds") - if (testLatestDeps) { - // Some latest dep tests are compiled for java 17 although the base version uses an older - // version. Disable rules that suggest using new language features. - disable("StatementSwitchToExpressionSwitch") - disable("PatternMatchingInstanceof") - } - if (name.contains("Jmh") || name.contains("Test")) { // Allow underscore in test-type method names disable("MemberName") From 0301a2267fbab13fed2108b57150939b5bb6ad3d Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Sat, 19 Jul 2025 10:47:07 -0700 Subject: [PATCH 4/4] Update settings.gradle.kts --- settings.gradle.kts | 1 - 1 file changed, 1 deletion(-) diff --git a/settings.gradle.kts b/settings.gradle.kts index 6f8af6bb4ad..b9f5f411631 100644 --- a/settings.gradle.kts +++ b/settings.gradle.kts @@ -2,7 +2,6 @@ import org.apache.tools.ant.taskdefs.condition.Os pluginManagement { plugins { - id("com.github.ben-manes.versions") version "0.52.0" id("com.github.jk1.dependency-license-report") version "2.9" id("me.champeau.jmh") version "0.7.3" id("com.gradle.plugin-publish") version "1.3.1"