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.

Bug 142381 - Hint for variable in enhanced for loop
Summary: Hint for variable in enhanced for loop
Status: RESOLVED FIXED
Alias: None
Product: java
Classification: Unclassified
Component: Hints (show other bugs)
Version: 6.x
Hardware: All All
: P3 blocker (vote)
Assignee: Max Sauer
URL:
Keywords: NETFIX
Depends on:
Blocks:
 
Reported: 2008-07-31 11:11 UTC by fommil
Modified: 2009-04-22 21:14 UTC (History)
0 users

See Also:
Issue Type: DEFECT
Exception Reporter:


Attachments
strawman fix which currently always returns "Object" as the type (2.94 KB, patch)
2009-04-13 13:33 UTC, fommil
Details | Diff
strawman fix, only execute when problem is with EFL variable declaration (2.93 KB, patch)
2009-04-13 13:53 UTC, fommil
Details | Diff
diff against main-golden using combination of fommil and jlahoda's code (4.42 KB, patch)
2009-04-13 14:11 UTC, fommil
Details | Diff
address some points by jlahoda (6.41 KB, text/plain)
2009-04-13 19:06 UTC, fommil
Details
add tests (11.57 KB, patch)
2009-04-18 16:30 UTC, fommil
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description fommil 2008-07-31 11:11:03 UTC
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){
}
Comment 1 Max Sauer 2008-07-31 11:36:53 UTC
Yes. That would be nice. I'll have a look at it for next release, thanks.
Comment 2 fommil 2008-09-05 11:52:37 UTC
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!
Comment 3 fommil 2008-10-16 20:27:20 UTC
it'd be crazy if this didn't make 6.5... the current behaviour is insane.
Comment 4 fommil 2009-04-13 00:19:54 UTC
I'm working on this... almost there, just need to infer the type of the variable from the EnchancedForLoopTree expression!
Comment 6 fommil 2009-04-13 10:46:34 UTC
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.
Comment 7 fommil 2009-04-13 10:51:57 UTC
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 ;-)
Comment 8 fommil 2009-04-13 12:11:20 UTC
bug 143318 seems to be influencing my fix for this
Comment 9 fommil 2009-04-13 13:32:25 UTC
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.
Comment 10 fommil 2009-04-13 13:33:35 UTC
Created attachment 79974 [details]
strawman fix which currently always returns "Object" as the type
Comment 11 fommil 2009-04-13 13:36:59 UTC
Previous patch could do with some more work to ensure that the variable replacement only happens when the error is with the variable tree.
Comment 12 fommil 2009-04-13 13:53:26 UTC
Created attachment 79976 [details]
strawman fix, only execute when problem is with EFL variable declaration
Comment 13 fommil 2009-04-13 14:11:39 UTC
Created attachment 79977 [details]
diff against main-golden using combination of fommil and jlahoda's code
Comment 14 fommil 2009-04-13 14:16:44 UTC
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
Comment 15 Jan Lahoda 2009-04-13 16:48:31 UTC
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.
Comment 16 fommil 2009-04-13 18:51:43 UTC
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;
        }
    }
Comment 17 fommil 2009-04-13 19:06:31 UTC
Created attachment 80002 [details]
address some points by jlahoda
Comment 18 fommil 2009-04-13 19:07:43 UTC
@jlahoda please try latest patch, should fix your first 3 concerns.
Comment 19 Jan Lahoda 2009-04-13 19:32:04 UTC
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).
Comment 20 fommil 2009-04-13 20:44:04 UTC
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.
Comment 21 fommil 2009-04-16 23:21:13 UTC
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...
Comment 22 fommil 2009-04-18 16:30:55 UTC
Created attachment 80414 [details]
add tests
Comment 23 fommil 2009-04-18 16:32:57 UTC
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.
Comment 24 fommil 2009-04-18 16:37:46 UTC
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.
Comment 25 Max Sauer 2009-04-20 16:29:51 UTC
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

Comment 26 Quality Engineering 2009-04-21 08:10:18 UTC
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
Comment 27 fommil 2009-04-22 21:14:32 UTC
I'm claiming this one in the name of NetFIX ;-)