Skip to content
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

Why X.class? #3

Open
oliviercailloux opened this issue Jan 27, 2021 · 4 comments
Open

Why X.class? #3

oliviercailloux opened this issue Jan 27, 2021 · 4 comments

Comments

@oliviercailloux
Copy link

I love your approach! This is the way to go for those who, like me, don’t like sneaky throws.

I just reviewed several popular SO questions that deal with the “streams and checked exceptions” issue, and found only your library as a solution for those who want no library that sneaky throw among their dependencies, among the ones that these questions reference (though I may have missed some):

Now to my question. Why do you require a class parameter designating the exception thrown, when creating a ThrowingStream? This seems unnecessary to me, and makes usage less elegant from the caller’s point of view. At least, the example in the wiki can be modified so as to avoid usage of the parameter Class<X> x in the StreamAdapter constructor (I just did it). Is there something that has escaped me?

@JeffFaer
Copy link
Owner

It's been a while since I wrote this code, so I don't remember the original reason (has it really been 5+ years, ouch).

I think it's to maintain type-safety with the checked exception. Java doesn't let you catch/throw exceptions generically, you need an actual class instance to do it. The following isn't allowed:

try {
  ...
} catch (X e) {
  ...
}

Instead you have to do something like:

try {
  ...
} catch (Throwable e) {
  if (x.isInstance(e)) {
    X exception = x.cast(e);
    ...
  } else {
    ...
  }
}

What did you do to get rid of the parameter in StreamAdapter?

@oliviercailloux
Copy link
Author

oliviercailloux commented Jan 28, 2021

try {
  ...
} catch (RuntimeException unchecked) {
  throw unchecked;
} catch (Exception checked) {
  // here we know that checked is of type X
  X exception = (X) checked;
  …
}

The cast is safe because 1) checked is a checked exception and 2) by language conventions (that is, assuming no sneaky throws), we are authorized to deduce that what we call in the try can only throw X as a checked exception. This requires an annotation for defusing the safety warning of the compiler, but it is provably safe, by construction.

@JeffFaer
Copy link
Owner

assuming no sneaky throws

I'd rather be safe than sorry in that situation. Getting a ClassCastException because someone did a sneaky throw would be pretty annoying to debug

Also X extends Throwable, so if someone used an exception type that inherited from RuntimeException, this might not work as expected (it's possible it does, but it's not a case I've considered). I suppose we could restrict X to X extends Exception to get around this.

I'm not against improving this API to make it more usable, I'm just not going to have the time to do it. I'd be happy to review PRs if you're feeling up to it

@oliviercailloux
Copy link
Author

I'd rather be safe than sorry in that situation. Getting a ClassCastException because someone did a sneaky throw would be pretty annoying to debug

There would be no ClassCastException in such a situation, IMHO: the runtime would simply throw the exception. The cast serves no other purpose than satisfying the compiler.

Also X extends Throwable, so if someone used an exception type that inherited from RuntimeException, this might not work as expected (it's possible it does, but it's not a case I've considered). I suppose we could restrict X to X extends Exception to get around this.

I don’t understand. Which problem would that cause? Also, an exception type that inherits from RuntimeException also extends Exception, so how is the restriction getting around this? Did you mean, “if someone used an exception type that inherited from Throwable but that is not an Exception”? Which problem would that cause?

I'm not against improving this API to make it more usable, I'm just not going to have the time to do it. I'd be happy to review PRs if you're feeling up to it

Thanks for your replies so far and willingness to review. TBH currently I am rather discouraged by the size of the library. I consider implementing only pieces of it for my own use for the moment, and later perhaps release an improvement to it, or even a totally new one that would be more modest. Anyway the API would be incompatible, I suppose.

Speaking of this, I am very surprised to see that ThrowingStream seems to be used by zero Maven artifacts, according to mvnrepositories. Is this correct? I find it hard to believe that there would be no open source library for such a use case that would be popular among Maven publishers. Perhaps another one is getting the favor of the users? Do you know of any library similar to yours, i.e. that helps working with streams and checked exceptions but without sneaky throws?

Also, relatedly, you posted somewhere that your library uses Brian Goetz’s recommended approach, or something similar. I have read his opinion (voiced numerous times) against sneaky throws, but I am not aware that he recommended a specific approach to solve the problem, similar to what this library does. Do you have a pointer supporting this? I’d be most curious to read his thoughts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants