New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable IntellijIdea inspection: 'throw' inside 'finally' block #3301

Closed
romani opened this Issue Jun 23, 2016 · 6 comments

Comments

Projects
4 participants
@romani
Member

romani commented Jun 23, 2016

From IntelijIdea: Error handling: 'throw' inside 'finally' block (Errors) (5)
good explanations: http://programmers.stackexchange.com/questions/188858/throwing-an-exception-inside-finally

Issue is blocked by: cobertura/cobertura#289

Details:

 src/main/java/com/puppycrawl/tools/checkstyle/api
 LocalizedMessage.java (1)
 src/main/java/com/puppycrawl/tools/checkstyle/checks
 NewlineAtEndOfFileCheck.java (1)
 UniquePropertiesCheck.java (1)
 src/main/java/com/puppycrawl/tools/checkstyle
 PropertyCacheFile.java (2)

Example for one file - LocalizedMessage :

 src/main/java/com/puppycrawl/tools/checkstyle/api
 LocalizedMessage.java (1)
424: newBundle() IOException might be thrown inside 'finally' block

current code:

final Reader streamReader = new InputStreamReader(stream, "UTF-8");
try {
    // Only this line is changed to make it to read properties files as UTF-8.
    resourceBundle = new PropertyResourceBundle(streamReader);
}
finally {
    stream.close();
}

Expected code:

    try (Reader streamReader = new InputStreamReader(stream, "UTF-8")) {
        // Only this line is changed to make it to read properties files as UTF-8.
        resourceBundle = new PropertyResourceBundle(streamReader);
    }

results to:

[INFO] --- cobertura-maven-plugin:2.7:check (default) @ checkstyle ---
[INFO] Cobertura 2.1.1 - GNU GPL License (NO WARRANTY) - See COPYRIGHT file
[INFO] Cobertura: Loaded information on 382 classes.
[ERROR] com.puppycrawl.tools.checkstyle.api.LocalizedMessage$Utf8Control failed coverage check. Branch coverage rate of 83.3% is below 100.0%

romani added a commit that referenced this issue Jun 23, 2016

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Jun 23, 2016

Contributor

@romani

[ERROR] com.puppycrawl.tools.checkstyle.api.LocalizedMessage$Utf8Control failed coverage check. Branch coverage rate of 83.3% is below 100.0%

Such code will be transpormed into something like:

Reader streamReader;
try {
   streamReader = new InputStreamReader(stream, "UTF-8");
}
finally {
  if(streamReader != null) {
     try {
       streamReader.close();
     } catch (Exception ex) {
        throw ex;
     }
  }
}

That is why Cobertura is not happy. I remember @Vladlis having similar problems while implementing some UTs.

FYI:
http://stackoverflow.com/questions/21727663/cobertura-java7-try-with-resource
http://stackoverflow.com/questions/17354150/8-branches-for-try-with-resources-jacoco-coverage-possible

Contributor

MEZk commented Jun 23, 2016

@romani

[ERROR] com.puppycrawl.tools.checkstyle.api.LocalizedMessage$Utf8Control failed coverage check. Branch coverage rate of 83.3% is below 100.0%

Such code will be transpormed into something like:

Reader streamReader;
try {
   streamReader = new InputStreamReader(stream, "UTF-8");
}
finally {
  if(streamReader != null) {
     try {
       streamReader.close();
     } catch (Exception ex) {
        throw ex;
     }
  }
}

That is why Cobertura is not happy. I remember @Vladlis having similar problems while implementing some UTs.

FYI:
http://stackoverflow.com/questions/21727663/cobertura-java7-try-with-resource
http://stackoverflow.com/questions/17354150/8-branches-for-try-with-resources-jacoco-coverage-possible

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 23, 2016

Member

By such code it will suppress original exception from try block. Instead of throw ex we need to create new exception and put original exception as suppressed in exception ctor. Such code will be correct but it is too ugly to make it.

I decided to think about it a bit later.

Member

romani commented Jun 23, 2016

By such code it will suppress original exception from try block. Instead of throw ex we need to create new exception and put original exception as suppressed in exception ctor. Such code will be correct but it is too ugly to make it.

I decided to think about it a bit later.

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Jun 23, 2016

Contributor

@romani
No, what I meant was that Cobertura is correct as try-with-rosources will be automatically replaced with the code I had mentioned, so new UTs should be created:

  1. Test for conditional block in finally block (2 UTs).
  2. Test for try-catch block in finally block (2 UTs).

1 and 2 are minimum requirements to satisfy coverage threshold.

Contributor

MEZk commented Jun 23, 2016

@romani
No, what I meant was that Cobertura is correct as try-with-rosources will be automatically replaced with the code I had mentioned, so new UTs should be created:

  1. Test for conditional block in finally block (2 UTs).
  2. Test for try-catch block in finally block (2 UTs).

1 and 2 are minimum requirements to satisfy coverage threshold.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 23, 2016

Member

No matter what it will result to, it is not simple issue, so need to be done separately.

Member

romani commented Jun 23, 2016

No matter what it will result to, it is not simple issue, so need to be done separately.

@romani romani added the approved label Jun 27, 2016

@rnveach rnveach added the medium label Mar 2, 2017

@romani romani added the GSoC2017 label Jul 4, 2017

@romani romani changed the title from Enable IntelijIdea inspection: 'throw' inside 'finally' block to Enable IntellijIdea inspection: 'throw' inside 'finally' block Jul 9, 2017

@romani romani moved this from To Do to In Progress in Practice What You Preach Jul 9, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 11, 2017

Member

@Nimfadora , I doubt that we can resolve this now, we are stuck with cobertura usage at it keeps our code at 100% level.
We cannot use try-with-resources till we switch to jacoco, some day in future, as try-with-resources it challenge even for jacoco (it is not done yet).

So lets suppress Idea inspection by till we switch to jacoco that support try-with-resources. Commit should have reference to this issue.

Member

romani commented Jul 11, 2017

@Nimfadora , I doubt that we can resolve this now, we are stuck with cobertura usage at it keeps our code at 100% level.
We cannot use try-with-resources till we switch to jacoco, some day in future, as try-with-resources it challenge even for jacoco (it is not done yet).

So lets suppress Idea inspection by till we switch to jacoco that support try-with-resources. Commit should have reference to this issue.

Nimfadora added a commit to Nimfadora/checkstyle that referenced this issue Jul 15, 2017

romani added a commit that referenced this issue Jul 15, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 15, 2017

Member

suppress comment is provided

Member

romani commented Jul 15, 2017

suppress comment is provided

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment