Community
Participate
Working Groups
Following example: void method() throws IOException { try(InputStream in = getClass().getResourceAsStream("file")) { System.out.println("in is never used."); } } Expected: "The value of the local variable in is not used" warning should be shown. Actual: No warning appears.
Going by the letter, the variable *is* used: for the (invisible) in.close() ;p I understand, though, that this usage is not interesting wrt this warning. Should be simple to fix.
That's good news. Thank you.
New Gerrit change created: https://git.eclipse.org/r/159273
Gerrit change https://git.eclipse.org/r/159273 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=f75ba1319d83aca6f56f1c9b61eae3fd3810d561
(In reply to Eclipse Genie from comment #4) > Gerrit change https://git.eclipse.org/r/159273 was merged to [master]. > Commit: > http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/ > ?id=f75ba1319d83aca6f56f1c9b61eae3fd3810d561 Fix released to master for 4.16 M1
This is causing two issues in the jdt debug build: 22:51:22 * Marker [on: /org.eclipse.jdt.launching/launching/org/eclipse/jdt/internal/launching/ClasspathShortener.java, id: 0, type: org.eclipse.jdt.core.problem, attributes: [arguments: 1:target, categoryId: 120, charEnd: 14069, charStart: 14063, id: 536870973, lineNumber: 356, message: The value of the local variable target is not used, severity: 2, sourceId: JDT], created: 3/13/20 9:51 PM] 22:51:22 * Marker [on: /org.eclipse.jdt.launching/launching/org/eclipse/jdt/launching/SocketUtil.java, id: 26, type: org.eclipse.jdt.core.problem, attributes: [arguments: 1:s, categoryId: 120, charEnd: 1850, charStart: 1849, id: 536870973, lineNumber: 48, message: The value of the local variable s is not used, severity: 2, sourceId: JDT], created: 3/13/20 9:51 PM] See this code in ClasspathShortener.createClasspathOnlyJar(String classpath) ... Manifest manifest = new Manifest(); manifest.getMainAttributes().put(Attributes.Name.MANIFEST_VERSION, "1.0"); //$NON-NLS-1$ manifest.getMainAttributes().put(Attributes.Name.CLASS_PATH, manifestClasspath.toString()); try (JarOutputStream target = new JarOutputStream(new FileOutputStream(jarFile), manifest)) { } ... This code is valid because the header is written in the constructor of JarOutputStream, isn't it? Not sure how to go on with https://git.eclipse.org/r/#/c/153269/...
(In reply to Stephan Herrmann from comment #5) > (In reply to Eclipse Genie from comment #4) > > Gerrit change https://git.eclipse.org/r/159273 was merged to [master]. > > Commit: > > http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/ > > ?id=f75ba1319d83aca6f56f1c9b61eae3fd3810d561 > > Fix released to master for 4.16 M1 Stephan, the change makes some perfectly valid code to show compilation errors in few SDK and other Eclipse projects, where "unused" warnings are configured as errors. Also the example in comment 0 is not convincing either. Why should "in" be flagged as unused if it *is* used? Most of the cases I've now seen where this new warning is reported, the code intentionally opened some resource in try() only to avoid extra burden to have finally {} block - now we force developers to do exactly that or add ugly annotations, see https://git.eclipse.org/r/159365 for example. Looking on possible fixes I'm not really happy to add them. Could we configure it as a new optional preference that is switched off by default? Because of this fix I have errors in org.eclipse.jdt.launching org.eclipse.jgit org.eclipse.egit.core org.eclipse.core.resources and numerous errors in tests. Most of them I would classify as "false positive".
(In reply to Andrey Loskutov from comment #7) > Because of this fix I have errors in > > org.eclipse.jdt.launching > org.eclipse.jgit > org.eclipse.egit.core > org.eclipse.core.resources > > and numerous errors in tests. Most of them I would classify as "false > positive". Stephan, it would be great if you could either revert the JDT change to discuss how to proceed or to push some kind of fix for that, because errors on projects above prevents people from contributing or at least makes them unhappy because of compilation errors in valid code.
Argh, once again we are bitten by "creative" coding. The code declares a variable 'target' which it doesn't use, BUT technically the variable is still needed for the invisible close. What can we do? (0) Revert the change (1) Make the "unused" warning for this location configurable (2) Live with an undesired warning once in a blue moon and advise users to use @SuppressWarnings("unused") directly on the resource declaration: try (@SuppresWarnings("unused") JarOutputStream target = ...) { ... (0) means that we will never detect resource variables that were meant to be used but aren't. (1) means that some projects that happen to contain one or more occurrences of the doubtful code pattern can disable the new warning (and thus possibly miss other interesting reports) (2) to me sounds like the fairest option: we still raise warnings, and users can silence the warning per case (after inspection). We still have (4): hardcode the fact that this particular constructor (from a system class) does more than opening the stream, and internally mark the resource variable as "really used".
@Andrey, I had a mid-air collision with your comments. Please educate me: are there many other constructors of resource classes, that actually do more then opening the resource? In my book, the constructor typically only opens the resource and if we do this in t-w-r without accessing the variable then this is a strong indication that this is useless code.
As to "unused" configured as error: do those projects enable suppressing optional errors with @SuppressWarnings? IMHO they should. Warnings that are disabled by default are not worth the effort of implementation.
I agree that this should be configurable - this causes hundreds of warnings in our code. We have a logging framework that uses the close() call to measure the runtime of some code. It looks like this (note we name the helper class and variable already "__" to make clear that they are not used elsewhere): try (__ __ = ThreadLogger.block(SomeClass.class, "someAction")) { // some code }
An alternative would be to not report the warning based on the variable name (e.g. starts with "_|unused")
Created attachment 282112 [details] 33 new errors in my SDK workspace (In reply to Stephan Herrmann from comment #10) > Please educate me: are there many other constructors of resource classes, > that actually do more then opening the resource? See screenshot, below text for easier navigation. It is not always constructor, and not all constructors can be used only to open a resource. In tests for example, opening streams on files locks them, or checks if the opened resource of the expected type, etc. > In my book, the constructor typically only opens the resource and if we do > this in t-w-r without accessing the variable then this is a strong > indication that this is useless code. Most of it is *not* useless. Creative or not, the code is valid and the new warning is a PITA right now, because adding SuppressWarnings or finally blocks is ugly. ostream is not used SafeChunkyOutputStream.java /org.eclipse.core.resources/src/org/eclipse/core/internal/localstore line 86 in is not used Bug_303517.java /org.eclipse.core.tests.resources/src/org/eclipse/core/tests/resources/regression line 123 in is not used Bug_303517.java /org.eclipse.core.tests.resources/src/org/eclipse/core/tests/resources/regression line 133 in is not used Bug_303517.java /org.eclipse.core.tests.resources/src/org/eclipse/core/tests/resources/regression line 142 scope is not used UnitOfWork.java /org.eclipse.egit.core/src/org/eclipse/egit/core line 80 scope is not used UnitOfWork.java /org.eclipse.egit.core/src/org/eclipse/egit/core line 95 scope is not used UnitOfWork.java /org.eclipse.egit.core/src/org/eclipse/egit/core line 114 scope is not used UnitOfWork.java /org.eclipse.egit.core/src/org/eclipse/egit/core line 131 target is not used ClasspathShortener.java /org.eclipse.jdt.launching/launching/org/eclipse/jdt/internal/launching line 356 s is not used SocketUtil.java /org.eclipse.jdt.launching/launching/org/eclipse/jdt/launching line 48 git is not used CleanTest.java /org.eclipse.jgit.pgm.test/tst/org/eclipse/jgit/pgm line 24 fis is not used CheckoutCommandTest.java /org.eclipse.jgit.test/tst/org/eclipse/jgit/api line 162 fis is not used CheckoutCommandTest.java /org.eclipse.jgit.test/tst/org/eclipse/jgit/api line 181 fis is not used CheckoutCommandTest.java /org.eclipse.jgit.test/tst/org/eclipse/jgit/api line 795 repo is not used CleanCommandTest.java /org.eclipse.jgit.test/tst/org/eclipse/jgit/api line 231 stdInStream is not used IndexDiffWithSymlinkTest.java /org.eclipse.jgit.test/tst/org/eclipse/jgit/indexdiff line 149 repo is not used FileRepositoryBuilderTest.java /org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file line 52 repo is not used FileRepositoryBuilderTest.java /org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file line 65 repo is not used FileRepositoryBuilderTest.java /org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file line 80 unused is not used AbbreviationTest.java /org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file line 158 unused is not used T0003_BasicTest.java /org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file line 366 fis is not used MergerTest.java /org.eclipse.jgit.test/tst/org/eclipse/jgit/merge line 1042 ins is not used SimpleMergeTest.java /org.eclipse.jgit.test/tst/org/eclipse/jgit/merge line 61 c is not used ReceivePackAdvertiseRefsHookTest.java /org.eclipse.jgit.test/tst/org/eclipse/jgit/transport line 161 git is not used WalkEncryptionTest.java /org.eclipse.jgit.test/tst/org/eclipse/jgit/transport line 1050 s is not used SideBandOutputStreamTest.java /org.eclipse.jgit.test/tst/org/eclipse/jgit/transport line 168 revWalk is not used ListTagCommand.java /org.eclipse.jgit/src/org/eclipse/jgit/api line 47 oldDb is not used RepositoryCache.java /org.eclipse.jgit/src/org/eclipse/jgit/lib line 255 e is not used ReceivePack.java /org.eclipse.jgit/src/org/eclipse/jgit/transport line 2172 fc is not used FileUtils.java /org.eclipse.jgit/src/org/eclipse/jgit/util line 954 in is not used ExportArchiveFileOperationTest.java /org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/datatransfer line 406 restore is not used ImportExistingProjectsWizardTest.java /org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/datatransfer line 1159 restore is not used ImportExistingProjectsWizardTest.java /org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/datatransfer line 1202
New Gerrit change created: https://git.eclipse.org/r/159369
(In reply to Eclipse Genie from comment #15) > New Gerrit change created: https://git.eclipse.org/r/159369 OK, I see no easy win. We have more important tasks. Reverting and planning to close WONTFIX, sorry Björn.
It‘s a pity. I really like these kind of warnings when they are valid... To really work in all cases it would need to detect if there is code executed as part of the constructor or close and then do not create a warning, right?
(In reply to Carsten Hammer from comment #17) > It‘s a pity. I really like these kind of warnings when they are valid... > To really work in all cases it would need to detect if there is code > executed as part of the constructor or close and then do not create a > warning, right? Not really. The variable *is* used, the close() or constructor implementation might eben not be known at compile time, so in most cases one need to *think* *if* this is a false positive or not. (In reply to Stephan Herrmann from comment #16) > (In reply to Eclipse Genie from comment #15) > > New Gerrit change created: https://git.eclipse.org/r/159369 > > OK, I see no easy win. We have more important tasks. Reverting and planning > to close WONTFIX, sorry Björn. Many thanks Stephan. Might be we could add a new option enabled by default - for this, we could disable it in related projects and let committers decide if they want that enabled or not - if yes, they will be in charge to fix the creative code.
Gerrit change https://git.eclipse.org/r/159369 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=a1ebb241d08b63d5c03db6c6cb0f731a4356f894
(In reply to Eclipse Genie from comment #19) > Gerrit change https://git.eclipse.org/r/159369 was merged to [master]. > Commit: > http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/ > ?id=a1ebb241d08b63d5c03db6c6cb0f731a4356f894 Reverted for 4.16 M1. As mentioned, I don't think a warning hidden behind yet-another off-by-default configuration option doesn't carry it's own weight, but aids complexity to the configuration UI. The situation will of course change when / if Oracle decides to provide means to avoid an explicit variable in the first place, or make it unmentionable by supporting '_' in this position.
> don't think ... doesn't ... Remember: don't use no double negations. oops. Scratch whichever one you prefer
The story behind this issue... Recently I encounter this lines of code and everything seems ok in Eclipse IDE try (InputStream resource = clazz.getResourceAsStream("edimappingconfig.xml")) { final Edimap edimap = EDIConfigDigester.digestConfig(clazz.getResourceAsStream("edimappingconfig.xml")); return edimap.getDelimiters(); } catch (final Exception e) { throw new IllegalStateException("Error on reading Edifact delimiters", e); } but another popular IDE kindly warns me about *resource* is not used. I would expect that Eclipse compiler warns me about this issue too. It's hard to understand that some false positives mainly caused by "creative" *test* code block this feature. The above usage patterns like file locking or time measurements resp. require at least a comment to document the hidden magic (someone could call it misuse too). One could consider configuration of unused declarations as errors doubtful additionally. I totally agree with you that a @SuppressWarnings would be the fairest option.
In order to give this feature a chance, someone may want to write down the rules how to distinguish the cases where a warning is desired vs. those where it isn't. It might be tempting to say, we just have to analyze the constructor or method on the right hand side of the resource assigment, but (a) JDT doesn't (yet?) have any infra structure for performing flow analysis on byte code. (b) It's far from obvious what such analysis should look for. I get the impression, that also the close() method would need to be analyzed to see if *that* perhaps justifies allocation of the resource. If it were only rare situations where the warning is raised but is not desired, then surely I would fight for @SuppressWarnings to be accepted as the solution. Two problems: (1) the number of cases seems to be higher than I expected (2) projects having configured unused locals as errors would need to be convinced to enable @SuppressWarnings for optional errors. A propos (2): It's hard to think of any optional error, that is reported with 100% accuracy, i.e., whenever *any* warning is configured as error, the project should *expect* the need to still use @SuppressWarnings for it. From a language design p.o.v. the situation is indeed odd: it's the only situation that I'm aware of, where it's semantically relevant to declare a local variable (so that the invisible close() can use the variable's value) even if the source code has no reference to that value. Hence my pondering about given such variables the unmentionable name '_'. Anyone keen on contributing to the evolution of Java may indeed suggest this to some expert group that is likely to feel responsible for this (I'm not sure which this would be). Alternatively, supporting arbitrary expressions instead of a variable declaration (or more recently: reference to an existing variable) would resolve this issue likewise.
(In reply to Stephan Herrmann from comment #23) > (2) projects having configured unused locals as errors would need to be > convinced to enable @SuppressWarnings for optional errors. > > A propos (2): It's hard to think of any optional error, that is reported > with 100% accuracy, i.e., whenever *any* warning is configured as error, the > project should *expect* the need to still use @SuppressWarnings for it. I gave this discussion it's own home: bug 561188
Verified for 4.16 M1 using build I20200407-1800
