-
Notifications
You must be signed in to change notification settings - Fork 737
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
Allow lambdas and method references referencing subclasses #4438
base: master
Are you sure you want to change the base?
Conversation
@Test | ||
public void subclassStaticMethod() { | ||
testHelper | ||
.addSourceLines( | ||
"Foo.java", | ||
"class A {", | ||
" // BUG: Diagnostic contains:", | ||
" static int value = B.value(); ", | ||
"}", | ||
"class B extends A {", | ||
" static int value() { return 0; }", | ||
"}") | ||
.doTest(); | ||
} |
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.
The check's treatment of this situation is unchanged with this PR. I just noticed it wasn't tested and wanted to make sure it was still catching this usage while working on the check.
Hi, thanks for raising this and for the suggested fix. I think the code generated by here by immutables sounds similar to AutoValue. The check has a heuristic for AutoValue:
And there's a separate check to discourage usages of AutoValue that could lead to deadlocks: https://errorprone.info/bugpattern/AutoValueSubclassLeaked Does any of that approach seem like it could be applicable to immutables? All else being equal I think it'd be nice to avoid a blanket exemption for lambdas and method references. If there aren't other good options for immutables and we decided not to add that exemption by default, I suppose we could always add a flag to make the handling of lambdas and method references configurable. What do you think? |
Background
In our company's codebase, we make heavy use of the popular Immutables library to generate value objects. Our typical style looks like this:
From this definition, Immutables would generate a
MyImmutable
value class extendingMyImmutableIF
, and an associated builder. We tend to reference the generated type (MyImmutable
in this case) in consuming code rather than the abstract definition type (MyImmutableIF
). In hindsight, that usage pattern does complicate some things, but we now have thousands of these value classes so we'll likely need to support this pattern for the foreseeable future.One downside of this usage pattern is it tends to lead to the chance of a class initialization deadlock. For example, often immutable creators want to have some default instance, and they'll define that on their abstract type:
This is obviously prone to a class initialization deadlock (since
MyImmutable
extendsMyImmutableIF
, andMyImmutable FORTY_TWO
is a static field ofMyImmutableIF
assigned to during the class's initialization), and we've been bitten by them many times in the past using this pattern. This was the inspiration for my fix in #4429, and with that in placeClassInitializationDeadlock
correctly detects these situations.One way we've worked around the possibility of deadlocks in these situations is to defer the construction of the default value past class initialization time:
(Using Guava's
Suppliers.memoize
)This is not prone to class initialization deadlock since, according to the JVM spec and real-world experimentation,
MyImmutable
won't be initialized whenMyImmutableIF
is initialized.MyImmutable
will only be initialized onceMyImmutableIF.fortyTwo()
is called since that's the first time something onMyImmutable
is accessed (assuming it hasn't otherwise already been initialized before that, of course).Another common example in our codebase is holding some common function object that operates on instances of the value class:
This is also not prone to class initialization deadlock; referencing methods does not initialize the class containing the referenced method (at least not with
javac
; I think it's possible for that to differ depending on how method references are compiled).However,
ClassInitializationDeadlock
(with my previous fix) flags both of these situations, the first at the usage ofMyImmutable.builder()
inside the lambda passed toSuppliers.memoize
, and the second at the reference toMyImmutable::property
.The fix
This PR updates
ClassInitializationDeadlock
to ignore lambda expressions and member references when looking for references that could enable a class initialization deadlock.I recognize this isn't a perfect fix. Some lambdas or member references can actually be problematic. Here's a contrived example:
With this PR, Error Prone wouldn't flag that usage despite it being prone to a class initialization deadlock.
However, I can't think of any useful usages of lambdas or member references that would both enable a class initialization deadlock and wouldn't otherwise reference the other class in a way that would initialize it anyway. I'm sure we can come up with some examples, but at least in our codebase, most of the method references and lambdas the check is flagging are false positives.
Thanks for your consideration.