-
Notifications
You must be signed in to change notification settings - Fork 94
Error Prone / NullAway support for JSpecify #196
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.
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
Changes from 1 commit
3f676ac
4fbeccd
4a68a39
7a6749e
afd5dc1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,11 +11,12 @@ plugins { | |
id 'io.github.gradle-nexus.publish-plugin' version '1.0.0' | ||
id 'com.github.ben-manes.versions' version '0.51.0' | ||
id "me.champeau.jmh" version "0.7.3" | ||
id "net.ltgt.errorprone" version '4.2.0' | ||
} | ||
|
||
java { | ||
toolchain { | ||
languageVersion = JavaLanguageVersion.of(11) | ||
languageVersion = JavaLanguageVersion.of(17) | ||
} | ||
} | ||
|
||
|
@@ -75,8 +76,34 @@ dependencies { | |
// this is needed for the idea jmh plugin to work correctly | ||
jmh 'org.openjdk.jmh:jmh-core:1.37' | ||
jmh 'org.openjdk.jmh:jmh-generator-annprocess:1.37' | ||
|
||
errorprone 'com.uber.nullaway:nullaway:0.12.6' | ||
errorprone 'com.google.errorprone:error_prone_core:2.37.0' | ||
} | ||
|
||
import net.ltgt.gradle.errorprone.CheckSeverity | ||
|
||
tasks.withType(JavaCompile) { | ||
options.release = 11 | ||
options.errorprone { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. builds with v17 - targets v11 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmmm .. I don't think we can do that: build with 17 would allow to use APIs which are in 17, but not in 11, which would result in errors when run with 11. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure how to achieve errorprone / nullaway checking without this I took this from @bclozel examples : bclozel@f449972 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It should be safe as the "-release" option is tailored for that:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. https://docs.gradle.org/current/userguide/building_java_projects.html#sec:compiling_with_release
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So we are running the v17 compiler but using the "release" of JDK 11 including libraries |
||
disableAllChecks = true | ||
check("NullAway", CheckSeverity.ERROR) | ||
// | ||
// end state has us with this config turned on - eg all classes | ||
// | ||
//option("NullAway:AnnotatedPackages", "org.dataloader") | ||
option("NullAway:OnlyNullMarked", "true") | ||
option("NullAway:JSpecifyMode", "true") | ||
} | ||
// Include to disable NullAway on test code | ||
if (name.toLowerCase().contains("test")) { | ||
options.errorprone { | ||
disable("NullAway") | ||
} | ||
} | ||
} | ||
|
||
|
||
task sourcesJar(type: Jar) { | ||
dependsOn classes | ||
archiveClassifier.set('sources') | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
package org.dataloader; | ||
|
||
import org.dataloader.annotations.PublicApi; | ||
import org.dataloader.impl.Assertions; | ||
import org.dataloader.instrumentation.ChainedDataLoaderInstrumentation; | ||
import org.dataloader.instrumentation.DataLoaderInstrumentation; | ||
import org.dataloader.instrumentation.DataLoaderInstrumentationHelper; | ||
|
@@ -14,6 +15,7 @@ | |
import java.util.LinkedHashMap; | ||
import java.util.List; | ||
import java.util.Map; | ||
import java.util.Objects; | ||
import java.util.Set; | ||
import java.util.concurrent.ConcurrentHashMap; | ||
import java.util.function.Function; | ||
|
@@ -142,7 +144,7 @@ private static DataLoaderOptions setInInstrumentation(DataLoaderOptions options, | |
*/ | ||
public DataLoaderRegistry register(DataLoader<?, ?> dataLoader) { | ||
String name = dataLoader.getName(); | ||
assertState(name != null, () -> "The DataLoader must have a non null name"); | ||
Objects.requireNonNull(name, "The DataLoader must have a non null name"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The tooling understands Objects.requireNonNull but not out nonNull method There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can teach tooling about this custom assertion method. Spring Framework does that with a custom In a nutshell, two possibilities:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks - for the record the reason we have a custom Assert class is that back in Java 6 days - Objects.requireNonNull() didnt exist and we wanted near zero dependendencies - so we made out own Same in graphql-java btw |
||
dataLoaders.put(name, nameAndInstrumentDL(name, instrumentation, dataLoader)); | ||
return this; | ||
} | ||
|
@@ -176,7 +178,7 @@ public DataLoaderRegistry register(String key, DataLoader<?, ?> dataLoader) { | |
*/ | ||
public <K, V> DataLoader<K, V> registerAndGet(String key, DataLoader<?, ?> dataLoader) { | ||
dataLoaders.put(key, nameAndInstrumentDL(key, instrumentation, dataLoader)); | ||
return getDataLoader(key); | ||
return Objects.requireNonNull(getDataLoader(key)); | ||
} | ||
|
||
/** | ||
|
@@ -251,10 +253,10 @@ public DataLoaderRegistry unregister(String key) { | |
* @param key the key of the data loader | ||
* @param <K> the type of keys | ||
* @param <V> the type of values | ||
* @return a data loader or null if its not present | ||
* @return a data loader or null if it's not present | ||
*/ | ||
@SuppressWarnings("unchecked") | ||
public <K, V> DataLoader<K, V> getDataLoader(String key) { | ||
public <K, V> @Nullable DataLoader<K, V> getDataLoader(String key) { | ||
return (DataLoader<K, V>) dataLoaders.get(key); | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs to run on 17 to get errorprone happy (compiled for 17)
but we target 11 later