Bug 348944

Comment 1 Deepak Azad 2011-06-10 09:08:59 EDT

Created attachment 197774 [details]
fix + tests - I

MethodExitsFinder and ExceptionOccurrencesFinder now handle try with resources. Note that in both cases the closing } will be marked if close() throws an exception which is not caught.

Comment 2 Deepak Azad 2011-06-14 06:04:11 EDT

When try with resources is on more than one resource, it is easy to know which resource's close call will throw an exception. Hence we can probably mark the resource variable name along with the closing brace of the try block. Though I am not sure if it will be confusing to the user to see a variable name highlighted in case of Method exits or Throwing Exceptions..

Comment 3 Dani Megert 2011-06-14 06:13:06 EDT

(In reply to comment #2)
> When try with resources is on more than one resource, it is easy to know which
> resource's close call will throw an exception. Hence we can probably mark the
> resource variable name along with the closing brace of the try block. Though I
> am not sure if it will be confusing to the user to see a variable name
> highlighted in case of Method exits or Throwing Exceptions..

It could be that more than one is causing it, right? I still lean towards my first opinion i.e. that it might be too confusing to highlight the variable(s) in this case. It could also be confused with for/switch highlighting.

Comment 4 Deepak Azad 2011-06-14 06:21:39 EDT

(In reply to comment #3)
> It could be that more than one is causing it, right? 
Yes. However if out of say 5 resources an exception is thrown only on close call of 2 resources, then I can highlight only those 2.

Comment 5 Deepak Azad 2011-06-15 02:41:11 EDT

Created attachment 197999 [details]
fix + tests - II

Fix for extract method refactoring.

Comment 6 Deepak Azad 2011-06-16 05:32:53 EDT

See also Bug 348708.

Comment 7 Deepak Azad 2011-06-17 06:19:00 EDT

Created attachment 198171 [details]
fix + tests - III

Disable 'Split variable declaraiton' quick assist for resources declared in a try statement.

Comment 8 Deepak Azad 2011-06-24 06:29:49 EDT

Created attachment 198516 [details]
fix + tests - IV

- Disable the removal of try with resources block when when the last catch block is removed.
- Do not offer 'Remove surrounding 'try' block' in case of try with resources.

Comment 9 Deepak Azad 2011-06-24 07:02:13 EDT

(In reply to comment #1)
> Created attachment 197774 [details] [diff]
> fix + tests - I
> 
> MethodExitsFinder and ExceptionOccurrencesFinder now handle try with resources.
> Note that in both cases the closing } will be marked if close() throws an
> exception which is not caught.

Last things that remains is 
1. improve tooltip message for the closing } of try block
2. decide whether we also highlight the name of the resource whose close()
method can throw an exception 
3. tooltip for resource name if we do 2. I don't think we can do this, as
currently the Javadoc hover is shown in the editor.

I think we should simply do 1 and change the tooltip to - "'Exception' is
thrown from the implicit close() method of 'resource1', 'resource2'. In my
opinion too many things are highlighted if resource name is also highlighted,
also we cannot explain the highlighting in this case via a tooltip.

Markus, what do you think ?

Comment 10 Markus Keller 2011-06-24 10:32:32 EDT

> 1. improve tooltip message for the closing } of try block

Sounds good. But the } is still hard to hit with the mouse, so I don't think many people will discover the additional info in the hover.

> 2. decide whether we also highlight the name of the resource

I found the closing } of the TryStatement block not informative enough, so I added highlighting of the resource names to see how it looks.

Here's an example:

package tryresources;
import java.io.FileNotFoundException;
import java.io.FileReader;
class A {
	void foo() throws Throwable {
		try (
				ACloseable closeable = new ACloseable();
				FileReader reader = new FileReader("file1");
				) {
			int ch;
			while ((ch = reader.read()) != -1) {
				System.out.println(ch);
			}
		} catch (IllegalArgumentException e) {
			throw new Exception(e);
		} catch (FileNotFoundException e) {
			throw new Exception(e);
		}
	}
}
class ACloseable implements AutoCloseable {
	@Override
	public void close() throws IllegalArgumentException { }
}

I see your point about too many highlights, but I still think the names are valuable, since they make the hidden exit points obvious. Exit behavior is complicated in these situations, and the user should be pestered with the details.

> 3. tooltip for resource name

Would be nice, but that would need a special case in BestMatchHover#getHoverInfo2(ITextViewer, IRegion) to compute and boost this AnnotationHover, but not all annotation hovers -- e.g. the Debug Current Instruction Pointer is not that interesting.

Comment 11 Deepak Azad 2011-06-24 11:41:44 EDT

I guess it is better to err on the side of providing too much information than to provide less information, so I am OK with highlighting the variable name.

(In reply to comment #10)
> > 3. tooltip for resource name
> 
> Would be nice, but that would need a special case in
> BestMatchHover#getHoverInfo2(ITextViewer, IRegion) to compute and boost this
> AnnotationHover, but not all annotation hovers -- e.g. the Debug Current
> Instruction Pointer is not that interesting.

The hover needs a boost only for resource names, in case of method calls it is good to show the Javadoc hover as it contains info on the exact exception thrown. e.g. if you are looking for thrown occurrences of IOException it is sometimes useful to know that a method actually throws FileNotFoundException.

If this is too complicated I OK with letting it be as is.

Comment 12 Deepak Azad 2011-06-27 04:49:54 EDT

Created attachment 198629 [details]
fix - V

> 1. improve tooltip message for the closing } of try block
Done.

Comment 13 Deepak Azad 2011-06-28 07:22:44 EDT

I am done here, hence marking this bug as fixed. Any issues found in future can
be handled in separate bugs.

Comment 14 Michael Rennie 2011-07-20 14:35:05 EDT

verified