Bug 21175 - Incorrectly identified: Catch block is hidden by another one in the same try statement
Summary: Incorrectly identified: Catch block is hidden by another one in the same try ...
Status: RESOLVED WORKSFORME
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 2.0   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: 2.1 M2   Edit
Assignee: Philipe Mulet CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2002-07-01 21:30 EDT by Jon Christiansen CLA
Modified: 2003-09-04 10:53 EDT (History)
0 users

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jon Christiansen CLA 2002-07-01 21:30:09 EDT
I have a try catch block like this:

      try
        {
            list = Agent.getList();
        }
        catch (RequestTimeoutException toEx) 
        {
            reason = toEx.getMsg();
        }
        catch (AgentException agentEx) 
        {
            reason = agentEx.getMsg();
        }
        catch (ServletException e) 
        {
            reason = e.getMessage();
        }

The first two catch exceptions are subclasses of ServletException

But Eclipse is incorrectly telling me: "Catch block is hidden by another one 
in the same try statement" and is pointing to the line with the open bracket 
for the final catch block.
Comment 1 Philipe Mulet CLA 2002-07-03 15:55:45 EDT
Thanks, but this warning is intentional to let you know that the lastcatch 
block is unreachable since the first ones capture thrown exception scenarii.
You can disable this warning through the compiler options 'Hidden Catch Block'.

If you did write your code in 2 nested try-statements, an error would be 
reported saying that the ServletException catch block would be unreachable. 
When both catch blocks are part of the same try statement, no error is 
diagnosed (as per specs), but we figured we should issue a 'hidden catch block' 
warning still.

try {
    try {
	list = Agent.getList();
   } catch (RequestTimeoutException toEx) {
     ...
   } catch (AgentException agentEx) {
     ...
   }
} catch (ServletException e){ //---> unreachable
   ...
}
Comment 2 John Bollinger CLA 2002-10-08 09:42:25 EDT
I'm sorry, but Mr. Mulet's response is partially incorrect and rather weak.  He
writes "the lastcatch block is unreachable since the first ones capture thrown
exception scenarii."  It is true that the first two catch blocks capture _some_
exceptions that the last one would catch in their absence, but there are other
ServletException subclasses that would be caught by the final catch block, thus
that block is not unreachable.  In my case I have:
      try {
         showDocument(new URL(getDocumentBase(), script + "?" + sdata));
      } catch (MalformedURLException e) {
         reportError("Error", "Script URL mis- or un-configured.");
      } catch (IOException e) {
         reportError("IO exception", e.getMessage());
      }
which exhibits the same pattern.  I assure you that the reachability of that
second catch block has been empirically proven.

What's worse is that Mr. Mulet is correct that Eclipse flags the corresponding
nested try/catch construct as an error.  The outer catch block is no less
reachable there.  Moreover, Eclipse appears to compile the construct
successfully either way.

If you want to insist on flagging a warning here, then please change the
wording.  "Partially hidden" would be much better than simply "hidden" as far as
I'm concerned.  Also, this should not be flagged as an error in the nested case.

Please do note that this case should be distinguished from the one where the
order in which the catch blocks appear in the source is reversed -- in the
latter case Mr. Mulet's comments would be absolutely correct.
Comment 3 Philipe Mulet CLA 2002-10-08 12:30:02 EDT
Sorry, I was trying to be helpful, and you did not give me steps to reproduce 
this issue (Agent and other types aren't part of standard JDK). This for 
the "partially incorrect and rather weak" comment. 

Now, this warning is only giving a hint about the fact that if the combined try 
statement is expanded in nested try statements, there would be an error.
If you are in a situation where for instance another tool, like javac, does not 
complain with nested try statements, where we do, then yes I would consider we 
have a problem. I believe though we implement the spec correctly.

When you claim reaching the catch block empirically, are you sure you are not 
rather seeing binary compatibility issues (like some sources did not get 
recompiled against matching binaries) ? Is the Agent.getList() method throwing 
more exceptions than it declares ?

Can you provide a simple testcase that reproduces the problem ?

Comment 4 Philipe Mulet CLA 2002-10-10 06:58:27 EDT
Can you please provide a testcase ?
Comment 5 Philipe Mulet CLA 2002-10-11 06:47:51 EDT
Removing milestone until requested information is provided.
Comment 6 Philipe Mulet CLA 2002-10-14 11:58:47 EDT
Closing, please reopen with steps.
Comment 7 Philipe Mulet CLA 2002-10-18 04:55:48 EDT
Also see bug 24956 for similar user reaction, but user agreed our diagnosis was 
right.
Comment 8 Mike Gleen CLA 2003-04-08 12:12:36 EDT
The construct where a catch of a subclass exception precedes a catch of a 
superclass exception is clearly documented in, at least, "Java 2, The Complete 
Reference" (Osbourne) as correct semantics.

While I agree that it is sensible to flag dubious (although correct) code, in 
this case, the code that is marked with a warning is the best way to express 
the programmer's intent.  Thus there is no way to stop this spurious warning 
message without disabling it for cases where it may be correct.

As a slight aside, I don't understand why the discussion about nested try 
statements is relevant in this case.  The error appears in the simple flat 
try/catch/catch shown below. Are you really giving a diagnostic saying "if you 
changed this code, it would be wrong"?  Or am I missing something?

I believe that the statement below is only partly true:
    "Thanks, but this warning is intentional to let you know that
     the last catch block is unreachable since the first ones
     capture thrown exception scenarii."

The last block is not unreachable if some ServletException exception is thrown 
other than the ones explicitly catered for.

I believe it is not good to have a stream of spurious diagnostics (hints!) 
generated on every compile - this makes the code much more difficult to 
maintain.

Here is a sample full program that reproduces the diagnostic (the code is 
hacked from a real program so don't take points off for not being sensible).


NOTE: The code below doesn't re-throw the exception (as reported in bug
      24956),but still gets the diagnostic.  However, I, too, have seen 
      cases where the diagnostic is not given and cannot explain the
      difference.
********************************* Begin Code Sample
import java.util.Properties;
import java.util.StringTokenizer;
import java.util.*;
import java.io.*;

public  class TestCatch {
  public static HashMap getConfigTable(String title, String filename)
  {
    String stemp = null;
    boolean isFound = false;
    BufferedReader in = null;
    HashMap titleConf = null;
    if (filename == null)
      return null;

    try
    {
        in = new BufferedReader(new FileReader(filename));
    }

 	catch ( FileNotFoundException e)
    {

     System.exit(0);
    }

    catch (IOException e)
    {

      return null;
    }
     return null;
  }

}
 
Comment 9 Christian Schweer (only bugzilla-mails accepted) CLA 2003-09-04 10:53:14 EDT
I think the behaviour is correct - although IMHO the warning's description is
not optimal.

The code in comment #8 does NOT declare to throw an IOException! It declares
only to throw a FileNotFoundExcpetion, which in fact is caught before the
IOException!

Look at the following 2 code samples:

########################### Begin code sample 1
package com.csc.test;

import java.io.FileNotFoundException;
import java.io.IOException;

public class ExceptionTest {

    public static void main(String[] args) {
        try {
            test();
        } catch (FileNotFoundException fExc) {
            System.out.println("FileNotFoundException caught");
        } catch (IOException iExc) {
            System.out.println("IOException caught");
        }
    }
    
    private static void test() throws IOException {
    }
}
########################### End code sample 1

########################### Begin code sample 2
package com.csc.test;

import java.io.FileNotFoundException;
import java.io.IOException;

public class ExceptionTest {

    public static void main(String[] args) {
        try {
            test();
        } catch (FileNotFoundException fExc) {
            System.out.println("FileNotFoundException caught");
        } catch (IOException iExc) {
            System.out.println("IOException caught");
        }
    }
    
    private static void test() throws FileNotFoundExcpetion {
    }
}
########################### End code sample 2

In sample 1 no warning is generated - which is correct, because test() declares
to throw an IOException (the subclass FileNotFoundException would be catched first).
In sample 2 a warning is generated, because test does only declare to throw a
FileNotFoundException and no OTHER IOException. So ALL IOException test()
declared to throw are catched by the first block --> the second block is hidden!!