This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.
Eclipse supports a hint in the following scenario, and I have been sorely missing it since moving to Netbeans. Given the code Set<String> set = getSet(); for (entry : set){ } the variable "entry" has not been defined, but is clearly of type "String". Netbeans 6.1 currently throws up an error on this code and offers the unhelpful information "not a statement ; expected" An autofix/hint to add the type declaration would be very useful Set<String> set = getSet(); for (String entry : set){ }
Yes. That would be nice. I'll have a look at it for next release, thanks.
msauer, next release being post 6.5? If it was at all possible, I'd be really really overjoyed if this made it into 6.5!
it'd be crazy if this didn't make 6.5... the current behaviour is insane.
I'm working on this... almost there, just need to infer the type of the variable from the EnchancedForLoopTree expression!
The fix is already part of the Experimental Java Hints module available on the Development Update Center: http://hg.netbeans.org/main/contrib/file/tip/javahints/src/org/netbeans/modules/javahints/TypeInForEachLoop.java http://hg.netbeans.org/main/contrib/file/tip/javahints/test/unit/src/org/netbeans/modules/javahints/TypeInForEachLoopTest.java
Oh. Would have been nice to have known this (I was the one who flagged it as STARTED yesterday), as I spent several hours on this... but that aside: The experimental fix is not particularly elegant and results in a lot of code duplication with existing fixes. My suggestion would be to augment CreateElementUtilities.computeVariableDeclaration() with a context check such as (inside the current second if statement) TreePath context = parent.getParentPath(); if (context != null) { switch (context.getLeaf().getKind()) { case ENHANCED_FOR_LOOP: types.add(ElementKind.LOCAL_VARIABLE); // infer the type and return it, whilst adding LOCAL_VARIABLE to results // handle standard for loop If it can be proved that context is never null, it actually makes the current code much easier to read... and of course is much more versatile for future features such as supporting other loop types. This is then a 5 to 10 line patch to the existing functionality rather than a whole new hint of over 200 lines! In the infamous words of Magnus Magnusson, I've started, so I'll finish. I'll use the relevant parts of dcaoyuan's experimental code in the switch and attach a patch shortly.
Just spotted the "@author Jan Lahoda" in the file... OK, I'll look at your code (not dcaoyuan's) and see if my approach works ;-)
bug 143318 seems to be influencing my fix for this
Ignore previous comment regarding bug 143318... misunderstanding on my behalf. I'm attaching a patch which outlines the "in situ" approach to this hint, rather than creating an entirely new hint class. The patch is across 2 files:- * CreateElementUtilities.computeVariableDeclaration - discover that a LOCAL_VARIABLE is necessary and calculate the required type * AddParameterOrLocalFix.resolveLocalVariable - place the local variable in the enhanced for loop's VariableTree (current behaviour is to define it in the closest BlockTree) At the moment, CreateElementUtilities.computeVariableDeclaration always returns java.lang.Object. jlahoda's patch seems to do a good job of that inference so I'll look at copying his code in here.
Created attachment 79974 [details] strawman fix which currently always returns "Object" as the type
Previous patch could do with some more work to ensure that the variable replacement only happens when the error is with the variable tree.
Created attachment 79976 [details] strawman fix, only execute when problem is with EFL variable declaration
Created attachment 79977 [details] diff against main-golden using combination of fommil and jlahoda's code
Latest patch continues in the philosophy of editing:- * CreateElementUtilities.computeVariableDeclaration - discover that a LOCAL_VARIABLE is necessary and calculate the required type * AddParameterOrLocalFix.resolveLocalVariable - place the local variable in the enhanced for loop's VariableTree but also includes jlahoda's type calculation code from: http://hg.netbeans.org/main/contrib/file/tip/javahints/src/org/netbeans/modules/javahints/TypeInForEachLoop.java No test cases are included in this patch, but jlahoda has prepared some for the standalone fix: http://hg.netbeans.org/main/contrib/file/tip/javahints/test/unit/src/org/netbeans/modules/javahints/TypeInForEachLoopTest.java
The advantage of a standalone hint/fix is that people can use it even if it is not yet stable enough to be placed into a production build. Comments to the last patch: -the fix misbehaves if Tools/Options/Editor/Hints/ErrorFixes/Create Local Variable/Create Local Variable in Place in unchecked. -the fix misbehaves in cases like: for (Date d : someMethod()) { } if Date is not imported, and someMethod() returns Iterable<java.util.Date>. Note that in this case the Create class fix is not proposed, although it was before the fix, IIRC. -do not expect that info.getElements().getTypeElement("java.lang.Iterable") returns non-null value. Ideally, test the fix provider with all combinations of JDK5, JDK1.4 and source level 1.4 and 1.5 - it does not have to propose anything in "meaningless" cases, but should not throw an exception (it does for JDK5+source level 1.4, which is a mistake inherited from my experimental fix). -the fix should really be accompanied with tests. Very difficult cases (like the previous point) needn't be tested automatically, but the simple ones should be easy enough to test.
Thanks for the feedback! Say we get these issues fixed, are we looking at a standalone hint as the final solution, or is the structure proposed here likely to be realistic? I don't want to waste my time. There really is an awful amount of code duplication that goes into creating new hints for compile errors. It seems to me that "calculating the generic type of an Iterable TypeMirror" is functionality that others could benefit from, and therefore be placed in a Utilities static method somewhere. Where do you think would be the best class for this? Re: "Local Variable in Place" I had thought of this, but had forgotten that of course the enhanced for loop always demands a new variable, not reusing an old one! Will fix this. Re: un-imported class. I think the misbehaviour is actually worse than this... when I have 2 enhanced for loops, one after the other... the local variable wants to be placed in the BLOCK, not in the ENHANCED_FOR_LOOP -> VARIABLE. Will investigate! See end of post for example*. Are you suggesting that if the class is not imported, the hint to create one should be presented? I'm not sure if that hint makes sense. Re: non-null getTypeElement... noted! Re: tests. I am new to nb contributions, so the test system is a little confusing. Your tests are good... but I'll need to spend more time to understand how to port them into the CreateElements tests. ErrorHintsTestBase in particular is confusing to me. ============ *Misbehaviour on: public void getBlah(Collection<String> strings, Iterable<Date> dates){ for (String bar : strings){ String local = bar; } for ( bar : dates) { Date local = bar; } }
Created attachment 80002 [details] address some points by jlahoda
@jlahoda please try latest patch, should fix your first 3 concerns.
Reusing existing infrastructure is of course better in the long term. The main issue is that NetBeans 6.7 is in bugfixing phase, and I am not sure if the hint can be accepted. This is up to Max (and others in charge of 6.7). Re: utility method. If you feel this should be done in a utility method, it could be placed without any problems (IMO) into o.n.m.j.h.errors.Utilities (in java.hints) or o.n.m.e.j.Utilities (in java.editor). Could be also placed into API (o.n.api.java.source.ElementUtilities (in java.source)), but then the usecases should clearer. Re tests: I think you can reuse the tests from the experimental hint - the CreateElement(Utilities) and associated classes are tested in several classes (CreateClassTest, etc.), so introducing a new test class should not be a problem (except the IDE will not jump to it automatically). The tests in CreateElementTest are actually "second generation" of tests, which is still not very user friendly. You will need to slightly adjust the tests anyway. Alternatively, we could rename CreateElementTest to OrigCET and create new CreateElementTest using the contemporary test infrastructure (i.e. the one used in the experimental tests).
The inclusion in 6.7 would be superb... but regardless of how it's implemented, msauer will have to make that call. We can only beg by saying "pretty pretty please with cherries on top" ;-) Will investigate the tests, next patch will include them.
I have the tests working nicely in the AddParameterOrLocalFixTest, however I'm not yet happy with this patch because the generic type inference is a little ropey... the current way to do it is to grab the first method from the Iterable interface, hoping it is the "next" method. Then we obtain the return type (which is the generic type) and use that. What we should really be doing is asking for the TypeElement associated to the TreePath and then call getTypeParameters(). However, I'm completely at a loss how to obtain the TypeElement! Advice welcome, but I'll continue to ponder on this...
Created attachment 80414 [details] add tests
Latest patch adds jlahoda's tests Ignore my previous comment regarding how the generic type is calculated. jlahoda's code was looking for the "Iterable" method and obtaining the type from the return method, not trying to find "next" from the Iterator interface. So long as "Iterable" remains a single method (or another is added with the same return type), this code is solid.
sorry, previous patch included some edits to java.hints/nbproject/project.properties, please ignore them... they set the per-project formatting options to expand tabs to spaces.
Thanks for your contribution Sam and Honza. I've fixed one more use case: for (date : someMethodReturningIterableDate()) { Date local = |date; //caret stands here } and added some more tests. Integrated in: http://hg.netbeans.org/jet-main/rev/29536c797046
Integrated into 'main-golden', will be available in build *200904210201* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress) Changeset: http://hg.netbeans.org/main-golden/rev/29536c797046 User: Max Sauer <msauer@netbeans.org> Log: #142381: Hint for variable in enhanced for loop
I'm claiming this one in the name of NetFIX ;-)