-
Notifications
You must be signed in to change notification settings - Fork 125
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
Resorce Leak compiler error/warning (detection) has some problem #2207
Comments
Testing on JDK-17 the compiler informs me
Testing on JDK-21 plenty of warnings reported as missing are actually shown. @mmariotti please let me know which JDK version you build against and your compliance settings. |
All test cases with a lambda like Currently, the analysis fails to see that the expression For any added precision we probably need an I doubt, though, that we have a realistic chance to trace the data flow through any methods of |
@stephan-herrmann I changed the test class to be compatible with java 11 on, the result is the same using:
I uploaded the sample PoC here: https://github.com/mmariotti/eclipse-jdt-2207 The project is complete with Eclipse Project specific settings files, if you import it you should see exactly what I see. I enabled the "potential-resource-leak" detection too (in project => properties => java compiler => error/warnings => potential resource leak) It seems to me that it potential-detector is reporting 100% correct effective-leaks, not potential-leaks. Here you are a screenshot for brevity (resource leaks: WARN - yellow | potential: INFO - teal): Here is the summary table:
Indeed. Please note the singular behavior of the effective-detector in method It should be sufficient to check if the lambda is returning the
I agree, nevertheless I think the main requisite is to avoid false positives, at least to some extent. And obviously you can't follow the data flow, but it can be fully ignored: Take this method: public void t_supplier_lambdaExpr_localVariable(TestAutoCloseable t)
{
/* CORRECT (but misplaced)
* expected: WARNING
* actual: WARNING
*/
TestAutoCloseable t2 = Optional.ofNullable(t).orElseGet(() -> TestAutoCloseable.newInstance());
t2.toString();
} Leave out |
OK, as now you avoid the JDK classes that have changed between 17 and 21 the JDK version is no longer relevant. This should ensure we are seeing the same things :) First observation: whenever a test case acquires the resource using
That's why none of the problems reported are as definite resource leaks. This is by design.
When the compiler falsely reports a definite resource leak, that's a bug (and in the lambda case I have a fix already). When the analysis doesn't come to a definite conclusion (neither definite leak, nor definitely clean) then reporting a potential problem is the best we can do and it will be difficult to argue that the report is a false positive. There is no such bias as "false positives are worse than false negatives". That said, I don't see much leeway in improving the "naked" resource analysis, but the way forward is to use Then we can use your test cases to discuss if they can be improved such that most of the potential issues can be analyzed with certainty. Will you join me in that task? |
fixes eclipse-jdt#2207 + treat body of an expression lambda like a return statement + fetch owning bits from the lambda descriptor
fixes #2207 + treat body of an expression lambda like a return statement + fetch owning bits from the lambda descriptor
#2257 fixed the lambda case. You may want to pickup the next I-build when it appears at https://download.eclipse.org/eclipse/downloads/index.html and re-test the issue. If you find any use case that looks wrong despite the explanations I gave above, feel free to re-open and argue from which information the compiler should conclude differently than it does. |
Patch for this one seems to have caused some failures in I-build https://download.eclipse.org/eclipse/downloads/drops4/I20240331-1800/testresults/html/org.eclipse.jdt.core.tests.compiler_ep432I-unit-cen64-gtk3-java17_linux.gtk.x86_64_17.html |
fixes eclipse-jdt#2207 Don't run tests using lambda below compliance 1.8
Thanks. @mmariotti As this is only a problem of the test, not of the implemention, this build should be good for verifying the fix: https://download.eclipse.org/eclipse/downloads/drops4/I20240331-1800/ |
fixes #2207 Don't run tests using lambda below compliance 1.8
@stephan-herrmann I just downloaded I20240331-1800, but nothing has changed: false positives and negatives are exactly the same as of 2024-03. Look at Here, too: how can this be a Without going deeper with lambdas & co. detectors are a mess when just methods are involved: I have limited time available, but I'll try to learn how detection works and see if I can propose some fix. However, before focusing on lambdas and "owning" annots I think it must work, at least decently, with legacy code. |
I still don't see a bug in the examples you show :) so I'll try to explain one by one: In the picture I see "Potential resource leak: "t2" may not be closed". Isn't that exactly what you are expecting? The message says potential. Or did you mean to say "the leak is effective"? In that case, here's the explanation:
As you say: same issue, except that here the message receiver is not an instance but a class. For further illustration: assume that the typical idiom would not be
Please don't jump to conclusions 😄 - we have a non-trivial test suite demonstrating how it works. Each test case carefully crafted. |
I wasn't clear enough, I'll try again. TestAutoCloseable t2 = Optional.ofNullable(t).orElseGet(() -> TestAutoCloseable.newInstance());
t2.toString(); The problem in this statement is not how the Closeable is obtained by/inside a method/lambda/expression. The problem is that an instance of Closeable is assigned to a local variable and that's never explicitly closed. That said, we can safely assume that a Closeable assigned to a local variable is always open when the assignment occurs. Let's focus on the local variable: InputStream inputStream2 = Files.newInputStream(null);
InputStream inputStream3 = Channels.newInputStream((ReadableByteChannel) null); A warning is reported for
I don't know what it means that the receiver is a class. The return value is just unassigned, I assume the receiver is simply "none".
Are you sure it would be wrong?
I'm sorry, didn't mean to be rude. |
Clarification of terminology:
Sorry, "receiver" is our (the compiler's) name for the thing you invoke a method on, i.e., the thing left of the dot in a method invocation (terminology leaked from when Eclipse was Visual Age, i.e., SmallTalk terminology, where a method invocation is call a "message send" - then "receiver" is who you send the message to). |
When verbose explanations fail, perhaps code helps: import java.util.zip.ZipEntry;
import java.util.zip.ZipFile;
class ZipCache {
ZipFile get(String handle) {
// FIXME: insert your logic to find a zip file by the given handle,
// * either from a cache,
// * or freshly read from disk (in that case it's added to the cache)
return null; //
}
}
public class SharingDemo {
ZipCache cache;
void compare(String handle, String entry1, String entry2) {
ZipEntry zEntry1 = cache.get(handle).getEntry(entry1);
ZipEntry zEntry2 = cache.get(handle).getEntry(entry2);
doCompare(zEntry1, zEntry2);
}
void doCompare(ZipEntry zEntry1, ZipEntry zEntry2) {
if (zEntry1.getSize() > zEntry2.getSize())
System.out.println("Larger: "+zEntry1.getName());
}
} In it's current form the program will compare sizes of two zip entries, given that class ZipCache has a reasonable implementation (should I explain further)? If I understand your reasoning correctly, then focus is on local variable, so let's perfrom "Extract Local Variable" twice (not the smartest thing to do, but not a bug): void compare(String handle, String entry1, String entry2) {
ZipFile zipFile1 = cache.get(handle);
ZipEntry zEntry1 = zipFile1.getEntry(entry1);
ZipFile zipFile2 = cache.get(handle);
ZipEntry zEntry2 = zipFile2.getEntry(entry2);
doCompare(zEntry1, zEntry2);
} You seem to want ecj to report a definite resource leak against each void compare(String handle, String entry1, String entry2) {
ZipFile zipFile1 = cache.get(handle);
ZipEntry zEntry1 = zipFile1.getEntry(entry1);
zipfile1.close();
ZipFile zipFile2 = cache.get(handle);
ZipEntry zEntry2 = zipFile2.getEntry(entry2);
doCompare(zEntry1, zEntry2);
} Guess what: now the call Admittedly, the example is contrived, but with just a bit of extra code the locations where zipFile1 and zipFile2 are used could be far away from each other. But even in this contrived example it would be very bad, if the compiler advises the user to add a close statement that will introduce a bug. |
…ipse-jdt#2257) fixes eclipse-jdt#2207 + treat body of an expression lambda like a return statement + fetch owning bits from the lambda descriptor
fixes eclipse-jdt#2207 Don't run tests using lambda below compliance 1.8
…ipse-jdt#2257) fixes eclipse-jdt#2207 + treat body of an expression lambda like a return statement + fetch owning bits from the lambda descriptor
fixes eclipse-jdt#2207 Don't run tests using lambda below compliance 1.8
Hello,
I upgraded Eclipse from 2023-12 to 2024-03 and found my projects filled with (not potential) resource-leak detection warnings (by configuration).
It seems the detection is wrong in some situation:
Here is a simple class that shows all the findings:
The text was updated successfully, but these errors were encountered: