Comments (13)
Testing on JDK-17 the compiler informs me
The resource type ExecutorService does not implement java.lang.AutoCloseable
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.
from eclipse.jdt.core.
All test cases with a lambda like x -> Executors.newCachedThreadPool(x)
are special:
Currently, the analysis fails to see that the expression Executors.newCachedThreadPool(x)
is an abridged return statement. I'll provide a PR to fix this, but then the analysis can hardly know how the lambda will be used.
For any added precision we probably need an @Owning
or @NotOwning
annotation on the functional type of the lambda, in order to decide whether some piece of code will eventually sign as responsible for closing.
I doubt, though, that we have a realistic chance to trace the data flow through any methods of Optional
or such.
from eclipse.jdt.core.
@stephan-herrmann I changed the test class to be compatible with java 11 on, the result is the same using:
- Eclipse Adoptium jdk-21.0.2.13-hotspot
- Eclipse Adoptium jdk-17.0.7.7-hotspot
- Eclipse Adoptium jdk-11.0.18+10
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.
Conversely, the effective-detector seems to report potential-leaks.
Maybe in this version of Eclipse the two detectors have been incorrectly swapped?
Here you are a screenshot for brevity (resource leaks: WARN - yellow | potential: INFO - teal):
Here is the summary table:
+----------------------+-------------+---------------+----------+----------+----------+
| type | notAssigned | localVariable | managed | field | returned |
+----------------------+-------------+---------------+----------+----------+----------+
| expected | WARN | WARN | NO-WARN | NO-WARN | NO-WARN |
+----------------------+-------------+---------------+----------+----------+----------+
| direct | FalseNeg | FalseNeg | | | |
| supplier_lambdaExpr | (misplaced) | (misplaced) | FalsePos | FalsePos | FalsePos |
| supplier_lambdaExpr2 | FalseNeg | FalseNeg | | | |
| supplier_lambdaBlock | FalseNeg | FalseNeg | | | |
| supplier_methodRef | FalseNeg | FalseNeg | | | |
| function_lambdaExpr | (misplaced) | (misplaced) | FalsePos | | FalsePos |
| function_lambdaExpr2 | FalseNeg | FalseNeg | | | |
| function_lambdaBlock | FalseNeg | FalseNeg | | | |
| function_methodRef | FalseNeg | FalseNeg | | | |
+----------------------+-------------+---------------+----------+----------+----------+
All test cases with a lambda like x -> Executors.newCachedThreadPool(x) are special:
Currently, the analysis fails to see that the expression Executors.newCachedThreadPool(x) is an abridged return statement. I'll provide a PR to fix this, but then the analysis can hardly know how the lambda will be used.
Indeed. Please note the singular behavior of the effective-detector in method public void t_function_lambdaExpr_field(String str)
: it is, correctly, not signaling the warning (while its counterpart public void t_supplier_lambdaExpr_field(TestAutoCloseable t)
is).
It should be sufficient to check if the lambda is returning the AutoCloseable
(just like a Supplier
or Function
will do), if it is possible to do so, to suppress the warning, because you won't kwow if/how the caller would close it.
For any added precision we probably need an @owning or @NotOwning annotation on the functional type of the lambda, in order to decide whether some piece of code will eventually sign as responsible for closing.
I doubt, though, that we have a realistic chance to trace the data flow through any methods of Optional or such.
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 Optional
and lambdas, focus on t2
: it is an AutoCloseable
local variable assigned somehow.
We can just assume it has not been closed.
from eclipse.jdt.core.
@stephan-herrmann I changed the test class to be compatible with java 11 on, the result is the same using:
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 TestAutoCloseable.newInstance()
the analysis cannot know, whether:
- the current method is responsible for closing the resource, or
- the resource has been retrieved from some cache of resources for long time use, and some other part of the code will eventually close it (perhaps on application shut down).
That's why none of the problems reported are as definite resource leaks. This is by design.
I agree, nevertheless I think the main requisite is to avoid false positives ...
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 @Owning
/ @NotOwning
. If you haven't already seen I recommend you read https://eclipse.dev/eclipse/news/4.31/jdt.php#annotation-based-resource-analysis
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?
from eclipse.jdt.core.
FYI, test cases in #2257 already contain a few examples how annotations can help for some lambda situations.
Given that the functional interfaces for many such situations are library classes this entails that we want #2258 sooner rather than later :) - I'll try to schedule this for, say, 2024-09.
from eclipse.jdt.core.
#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.
from eclipse.jdt.core.
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
from eclipse.jdt.core.
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
Thanks.
Obviously it doesn't make sense to run tests which use a lambda expression with compliance 1.7.
Reminder to self: new tests must be run locally against all compliance levels since jenkins PR jobs test less then what production builds do.
Sorry for the noise.
@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/
from eclipse.jdt.core.
@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 t2
: the message highlighted is NOT a potential-resource-leak, it is effective!
Conversely, the message on lambda, that is a potential
Here, too: how can this be a potential
-resource-leak? It is quite obvious it is an effective one and no lambda is involved.
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.
from eclipse.jdt.core.
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: t2
is acquired from the return value of calling Optional.get()
. Without annotations the analysis has no way to see if the message receiver (an instance of Optional) will keep the reference to the resource and take care of closing it at a later point, or if calling get()
is meant as transferring ownership / responsibility to the caller. That's why it says "potential", in order to signal to users: the analysis result is essentially inconclusive.
Here, too: how can this be a potential-resource-leak? It is quite obvious it is an effective one and no lambda is involved.
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 System.out.println()
but System.getOut().println()
. According to your reasoning every client calling System.getOut()
would be forced to close the out stream. That would be wrong.
Without going deeper with lambdas & co. detectors are a mess when just methods are involved:
Please don't jump to conclusions 😄 - we have a non-trivial test suite demonstrating how it works. Each test case carefully crafted.
from eclipse.jdt.core.
Or did you mean to say "the leak is effective"? In that case, here's the explanation: t2 is acquired from the return value of calling Optional.get(). Without annotations the analysis has no way to see if the message receiver (an instance of Optional) will keep the reference to the resource and take care of closing it at a later point, or if calling get() is meant as transferring ownership / responsibility to the caller. That's why it says "potential", in order to signal to users: the analysis result is essentially inconclusive.
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.
It's not important how the Closeable is obtained, when it is assigned (the very last operation of the line of code), it is open.
I'm not aware of a method, constructor, any way in general (that has no other serious problem), which returns a closed Closeable.
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 inputStream2
, but only an info is reported for inputStream3
.
This is the point.
There should be no difference, both are (open) Closeables assigned to local variables, both variables should be reported with warnings.
Please explain to me what is the reason for this difference because I can't really grasp it (actually, I think I know the reason, but I also think the reason is wrong).
As you say: same issue, except that here the message receiver is not an instance but a class.
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".
In any other language without a GC this should probably be a leak (memory, handle, descriptor, ...) by itself (a resource is allocated but there's no pointer to access that resource - thus cannot be freed).
For further illustration: assume that the typical idiom would not be System.out.println() but System.getOut().println(). According to your reasoning every client calling System.getOut() would be forced to close the out stream. That would be wrong.
Are you sure it would be wrong?
Theoretically speaking, once the client code obtains an instance of sysout it should close it when finshed.
Nevertheless, in practice, we know that sysout is special and there's no need to close it.
Doesn't the same special treatment apply to Streams too? With only a few exceptions such as Streams returned by java.nio.Files methods.
Please don't jump to conclusions 😄 - we have a non-trivial test suite demonstrating how it works. Each test case carefully crafted.
I'm sorry, didn't mean to be rude.
But, honestly, I don't see a strong logic behind the different behaviors of the detector as shown in the last screenshot.
All of the statements are assignments of Closeables to local variables and they should be equivalent from the point of view of the detector.
from eclipse.jdt.core.
Clarification of terminology:
As you say: same issue, except that here the message receiver is not an instance but a class.
I don't know what it means that the receiver is a class.
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).
from eclipse.jdt.core.
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.
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 zipfile1
and zipfile2
, so the user will add zipFile1.close()
immediately after the local variable has been used (or the equivalent using t-w-r):
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 zipFile2.getEntry(entry2)
will throw IllegalArgumentException
because it is called on a closed zip file :(
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.
from eclipse.jdt.core.
Related Issues (20)
- Several tests are failing in unhooked junit file org.eclipse.jdt.core.tests.model.SealedTypeModelTests
- Compilation for JavaSE-1.8 does not guarantee Java 8 class files HOT 13
- [23] ECJ rejects 'module' as top level package in an import statement HOT 1
- JDK 21 preview features and maven HOT 5
- No results for jdt.core.tests.model and dependent tests in I20240619-1800 HOT 9
- Eclipse Java Editor hover completly broken now! HOT 4
- BasicBuildTests.testCustomerCompilerFactoryWithAP fails since I20240620-0440 HOT 7
- Faulty/Undesired CodeCompletion behaviour HOT 9
- Investigation new issues due to the Unnamed Patterns and Variables HOT 2
- OperandStack throws "java.lang.AssertionError: Attempt to push null on operand stack!" HOT 5
- Bug 545605 - Adjust ecj to change in JVMS 4.7.18./19. HOT 6
- [dom ast][rewrite] org.eclipse.jdt.core.tests.rewrite.describing.ASTRewritingEitherOrMultiPatternNodeTest test failures HOT 3
- False Positive Potential Resource Leak Warnings - @NotOwning Not Working Correctly HOT 3
- Bug 562845 - remove left over code from times when ModuleDeclaration was a subtype of TypeDeclaration
- Revise usage of `encoding` in more CompilationUnitResolver methods HOT 7
- False positive warning "Potential resource leak" inside "try with resources" HOT 3
- Class leak in org.eclipse.jdt.internal.compiler.util.CharDeduplication$CacheReference HOT 15
- Random failing Java9ElementTests.test009
- Random failing JavaSearchBugsTests.testBug231622
- Prevent excessive creation of intermediate SplitPackageBinding instances
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from eclipse.jdt.core.