Enable Error Prone check: CheckedExceptionNotThrown #8227

ksobolew · 2021-06-08T11:04:02Z

@kokosing keeps wishing that this was on.

The issue with this is that:

This is an experimental checker
It has a huge number of false negatives

Regarding the second point, it may be that making it more accurate would make it unviably slow (the equivalent inspection in IntelliJ IDEA takes a lot of time, has to look at the whole code base - it's definitely not local - and has to be done repeatedly until nothing more is found). But perhaps it's still useful.

findepi · 2021-06-08T20:15:29Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/LazyConnectionFactory.java

@@ -40,7 +40,6 @@ public LazyConnectionFactory(@ForLazyConnectionFactory ConnectionFactory delegat

    @Override
    public Connection openConnection(ConnectorSession session)
-            throws SQLException


Keeping this helps update the code in the future, would i want to throw a checked exception (as super/interface allows me to)

If you only call this method via the interface, you should be safe

i mean a future self who will be changing the implementation.

Then your future self will add this exception back? It doesn't affect the interface, or its callers, only the override.

Yes, except

except the method is not allowed to throw a checked exception (i need to consult parent for that, which i may not do), so i may e.g. wrap IOException in Runtime unnecessarily and reviewer may miss that either

someone can call the impl directly (not via interface), so such legit reintroduction of a checked exception may still be a breaking change for callers

Should we suppress it here, as other cases where throws is removed seem valid.

I suppressed the error in this method

is this considered an errorprone bug and reported upstream?

I'm personally still on the fence on this. On the one hand I'd love it to lead to removal of dead code more (if none of the implementations throw this, we can remove the throws from the interface declaration!), but on the other I guess you're right that it's better to be conservative here.

I can create an issue upstream to see what they think.

google/error-prone#3425

ksobolew · 2022-07-13T13:10:21Z

Refreshing this old PR. It looks like this check is getting more precise (and thus useful), though it still misses lots of cases.

testing/trino-testing/src/main/java/io/trino/testing/AbstractTestQueryFramework.java

ksobolew · 2022-07-14T09:09:08Z

Weird:

2022-07-13T08:46:40.993-0500	INFO	pool-53-thread-1	stderr	FATAL: io.trino.testng.services.LogTestDurationListener: onBeforeClass: 
java.lang.IllegalStateException: There already is a start record for test: io.trino.execution.TestSqlTask
	at com.google.common.base.Preconditions.checkState(Preconditions.java:590)
	at io.trino.testng.services.LogTestDurationListener.beginTest(LogTestDurationListener.java:245)
	at io.trino.testng.services.LogTestDurationListener.onBeforeClass(LogTestDurationListener.java:179)
	at org.testng.internal.TestMethodWorker.invokeBeforeClassMethods(TestMethodWorker.java:167)
	at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:108)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
	at java.base/java.lang.Thread.run(Thread.java:829)
2022-07-13T08:46:40.993-0500	INFO	pool-53-thread-1	stderr	JVM will be terminated

ksobolew · 2022-07-14T15:32:42Z

The Pinot failure is #13165.

For Redis:

Trying to resolve the latest version from remote
Error: socket hang up

Looks like a hiccup in the environment.

There are some code changes in the PR, but they should not affect any tests anyway.

kokosing · 2022-07-15T18:09:28Z

Re started failed jobs. Please report flaky failures as github issues.

ksobolew · 2022-07-18T14:36:42Z

The build is green, thanks. Not sure if there's anything to report if it was some instability in the build infra?

findepi · 2022-07-19T15:35:57Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/LazyConnectionFactory.java

@@ -39,6 +39,7 @@ public LazyConnectionFactory(@ForLazyConnectionFactory ConnectionFactory delegat
    }

    @Override
+    @SuppressWarnings("CheckedExceptionNotThrown") // throws in the interface declaration; keep it to prevent false sense of security


i don't understand the comment message

BTW this seems not violate the check

public interface AnInterface { void foo() throws Exception; } public class AnImplementationThatCurrentlyDoesNotThrow implements AnInterface { @Override public void foo() throws Exception {} }

so why do we need suppression here?
is it a bug in error-prone?

It's difficult to tell why it's not reported. Maybe there's an exception for Exception (but there are some places where it reports Exception, so likely not). I don't know what logic it uses to find those cases, but I'm pretty sure it doesn't find everything.

Turns out that this check does trigger in this case if the class is final, otherwise it does not. See this comment: google/error-prone#3425 (comment)

findepi · 2022-07-19T15:40:08Z

plugin/trino-cassandra/src/test/java/io/trino/plugin/cassandra/TestingScyllaServer.java

@@ -52,13 +52,11 @@
    private final CassandraSession session;

    public TestingScyllaServer()
-            throws Exception


This is test code. In tests is often more convenient to just declare throws Exception
and keep it, so that you won't need to add it ever again

Can we exclude the check on test resources?

I don't think that's possible per-check. Maybe we could create a separate configuration in maven-compiler-plugin for tests?

...in/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/transactionlog/AddFileEntry.java

testing/trino-testing/src/main/java/io/trino/testing/AbstractTestQueryFramework.java

martint · 2022-07-19T16:48:12Z

Forcing the removal of a checked exception declaration from a method is a source-level backward incompatible change. Any callers that are calling such methods within a try-catch block will have to rewrite their code before they can compile. This is not good for the SPI, and also not good in keeping code changes localized.

For example, given:

class A
{
  public static void main(String... args)
  {
    try {
      B.run();
    }
    catch (java.io.IOException e) {
    }
}

class B
{
  public static void run()
    throws java.io.IOException
  {
    ...
  }
}

Forcing the removal of throws java.io.IOException from B will cause A to no longer compile.

ksobolew · 2022-07-19T18:00:12Z

Forcing the removal of a checked exception declaration from a method is a source-level backward incompatible change. Any callers that are calling such methods within a try-catch block will have to rewrite their code before they can compile. This is not good for the SPI, and also not good in keeping code changes localized.

I agree that it is bad for interfaces and SPI, but other than that I would consider that an advantage (except for the bad change locality). If we know that some piece of code cannot throw a declared checked exception, this allows us to remove some potentially flaky error handling code, which was dead anyway. There is still an issue of unchecked exceptions, though.

The issue with this is that: * This is an experimental checker * It has a huge number of false negatives Regarding the second point, it may be that making it more accurate would make it unviably slow (the equivalent inspection in IntelliJ IDEA takes a lot of time, has to look at the whole code base - it's definitely not local - and has to be done repeatedly until nothing more is found). But perhaps it's still useful.

colebow · 2022-10-27T03:36:52Z

👋 @ksobolew - this PR has become inactive. If you're still interested in working on it, please let us know, and we can try to get reviewers to help with that.

We're working on closing out old and inactive PRs, so if you're too busy or this has too many merge conflicts to be worth picking back up, we'll be making another pass to close it out in a few weeks.

ksobolew · 2022-10-27T10:04:23Z

Hi @colebow, I think this change still has merit, but it looks like the Error Prone checker is still not precise enough to satisfy the community :) I'm going to close it, at least for now.

cla-bot bot added the cla-signed label Jun 8, 2021

ksobolew requested review from findepi, kokosing and Praveen2112 June 8, 2021 11:04

electrum approved these changes Jun 8, 2021

View reviewed changes

findepi reviewed Jun 8, 2021

View reviewed changes

findepi force-pushed the master branch from 8538e49 to 1f896ea Compare July 30, 2021 22:13

ksobolew force-pushed the ksob/enable-error-prone-check-CheckedExceptionNotThrown branch from 1cde497 to 200412a Compare July 13, 2022 13:08

github-actions bot added jdbc Relates to Trino JDBC driver tests:hive labels Jul 13, 2022

ksobolew commented Jul 13, 2022

View reviewed changes

testing/trino-testing/src/main/java/io/trino/testing/AbstractTestQueryFramework.java Outdated Show resolved Hide resolved

ksobolew force-pushed the ksob/enable-error-prone-check-CheckedExceptionNotThrown branch from 200412a to 3764d0f Compare July 13, 2022 13:15

kokosing approved these changes Jul 13, 2022

View reviewed changes

ksobolew force-pushed the ksob/enable-error-prone-check-CheckedExceptionNotThrown branch from 3764d0f to ee022be Compare July 14, 2022 13:42

findepi reviewed Jul 19, 2022

View reviewed changes

ksobolew force-pushed the ksob/enable-error-prone-check-CheckedExceptionNotThrown branch from ee022be to 8e7c166 Compare July 20, 2022 10:39

ksobolew force-pushed the ksob/enable-error-prone-check-CheckedExceptionNotThrown branch from 8e7c166 to 9d1c7a6 Compare August 30, 2022 08:30

ksobolew closed this Oct 27, 2022

ksobolew deleted the ksob/enable-error-prone-check-CheckedExceptionNotThrown branch October 27, 2022 10:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable Error Prone check: CheckedExceptionNotThrown #8227

Enable Error Prone check: CheckedExceptionNotThrown #8227

ksobolew commented Jun 8, 2021

findepi Jun 8, 2021

ksobolew Jun 9, 2021

findepi Jun 9, 2021

ksobolew Jun 9, 2021

findepi Jun 9, 2021

kokosing Jun 21, 2021 •

edited

Loading

ksobolew Jul 13, 2022

findepi Aug 30, 2022

ksobolew Aug 30, 2022

ksobolew Aug 30, 2022

ksobolew commented Jul 13, 2022

ksobolew commented Jul 14, 2022

ksobolew commented Jul 14, 2022

kokosing commented Jul 15, 2022

ksobolew commented Jul 18, 2022

findepi Jul 19, 2022

findepi Jul 19, 2022

ksobolew Jul 20, 2022

ksobolew Nov 18, 2022

findepi Jul 19, 2022

ksobolew Jul 20, 2022

martint commented Jul 19, 2022

ksobolew commented Jul 19, 2022

colebow commented Oct 27, 2022

ksobolew commented Oct 27, 2022

Enable Error Prone check: CheckedExceptionNotThrown #8227

Enable Error Prone check: CheckedExceptionNotThrown #8227

Conversation

ksobolew commented Jun 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kokosing Jun 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ksobolew commented Jul 13, 2022

ksobolew commented Jul 14, 2022

ksobolew commented Jul 14, 2022

kokosing commented Jul 15, 2022

ksobolew commented Jul 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martint commented Jul 19, 2022

ksobolew commented Jul 19, 2022

colebow commented Oct 27, 2022

ksobolew commented Oct 27, 2022

kokosing Jun 21, 2021 •

edited

Loading