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
Fix some ErrorProne issues in :model-core #28892
Conversation
83ad671
to
0552d74
Compare
@@ -144,16 +144,16 @@ public int getLinkCount(Predicate<? super MutableModelNode> predicate) { | |||
@Override | |||
public Set<String> getLinkNames(Predicate<? super MutableModelNode> predicate) { | |||
if (links == null) { | |||
return Collections.emptySet(); | |||
return ImmutableSet.of(); |
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.
💭 this has a benefit of avoiding extra copy when the result is used as an argument to ImmutableSet.copyOf
.
@@ -535,17 +536,17 @@ private BindingPredicate mapSubject(ModelReference<?> subjectReference, ModelAct | |||
|
|||
private List<BindingPredicate> mapInputs(List<? extends ModelReference<?>> inputs) { | |||
if (inputs.isEmpty()) { | |||
return Collections.emptyList(); | |||
return ImmutableList.of(); |
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.
💭 this has a benefit of avoiding extra copy when the result is used as an argument to ImmutableList.copyOf
.
@@ -252,7 +252,7 @@ public Object[] toArray() { | |||
} | |||
|
|||
@Override | |||
public <T> T[] toArray(T[] a) { | |||
public <E> E[] toArray(E[] a) { |
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.
💭 to my surprise, you can pass any array to the Collection.toArray
method, even of a completely unrelated type.
@@ -336,10 +339,11 @@ public HasMultipleValues<T> convention(Provider<? extends Iterable<? extends T>> | |||
|
|||
@Override | |||
protected String describeContents() { | |||
return String.format("%s(%s, %s)", collectionType.getSimpleName().toLowerCase(), elementType, describeValue()); | |||
String typeDisplayName = TextUtil.toLowerCaseLocaleSafe(collectionType.getSimpleName()); |
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.
💭 unlike MapPropery, here we can only have List
or Set
, so using the English locale is fine.
@@ -83,7 +83,8 @@ private MappedProperty map(String propertyName, MappedPropertyImpl mapping) { | |||
throw new IllegalStateException(String.format("Could not access property %s.%s", sourceType.getSimpleName(), propertyName), e); | |||
} | |||
if (!mapConventionOn(target, mapping)) { | |||
throw new IllegalStateException(String.format("Unexpected convention-supporting type used in property %s.%s", sourceType.getSimpleName(), propertyName, getter.getReturnType().getName())); | |||
throw new IllegalStateException(String.format( |
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.
💭 A real bug caught!
@@ -469,6 +469,8 @@ protected DynamicInvokeResult setOpaqueProperty(MetaClass metaClass, String name | |||
return DynamicInvokeResult.notFound(); | |||
} | |||
|
|||
@SuppressWarnings("MixedMutabilityReturnType") | |||
// This might be too invasive to fix properly because it is in the dynamic code. |
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.
💭 It looks like making all paths mutable is going to add overhead of allocating empty hashmaps, and making all paths immutable can actually break things.
@@ -54,7 +54,7 @@ public class DefaultConvention implements org.gradle.api.plugins.Convention, Ext | |||
private final InstanceGenerator instanceGenerator; | |||
|
|||
private Map<String, Object> plugins; | |||
private Map<Object, BeanDynamicObject> dynamicObjects; | |||
private IdentityHashMap<Object, BeanDynamicObject> dynamicObjects; |
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.
💭 ErrorProne insists that IndentityHashMap shouldn't be mixed with Map because of a different contract with regard to hashCode/equals.
Fixing it doesn't look too ugly anyway.
0552d74
to
ca0b55c
Compare
The type is user-provided and may contain non-ASCII symbols, or the default locale might have surprising case conversion rules (e.g. `I -> ı` for the Turkish language).
The warning was fixed by some other commit.
The BeanDynamicObject use case doesn't have enough ROI to investigate and fix properly, let's live with what we have. Both always returning mutable and always returning immutable may break stuff.
ca0b55c
to
6533fa9
Compare
@bot-gradle test PT |
I've triggered the following builds for you. Click here to see all build failures. |
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.
Nice!
"[k1: v1] + [k2: v2]" | { property().tap { put("k1", "v1"); put("k2", "v2") } } || "Map(string->String, {k1=v1} + {k2=v2})" | ||
"provider {k: v}" | { property().value(Providers.of([k: "v"])) } || "Map(string->String, fixed(class ${LinkedHashMap.name}, {k=v}))" | ||
"[k: provider {v}]" | { property().tap { put("k", Providers.of("v")) } } || "Map(string->String, entry{k=fixed(class ${String.name}, v)})" | ||
"default" | { property() } || "Map(String->String, {})" |
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.
👍
@bot-gradle merge |
Reviewing cheatsheet
Before merging the PR, comments starting with