Restrict attemptNarrow to subtypes of Throwable

[travisbrown]

This change makes it impossible to call attemptNarrow with a type parameter that isn't Throwable or one of its subtypes. This makes it slightly less easy to shoot yourself in the foot with it (see #3360), while still supporting its most common use cases.

The change doesn't break binary compatibility, but definitely does break source compatibility. It only breaks dangerous code, though, so I think that's fine for 2.2.0.

Personally I'd prefer to scrap it entirely (by making it package-private), since even in this restricted form it breaks parametricity in a way that feels un-Cats-like. I've chosen this route because of the precedent of catchOnly, which does a similar thing with runtime reflection but makes it relatively safe by restricting to Throwable.

[codecov-io]

Codecov Report

Merging #3361 into master will not change coverage by %.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3361   +/-   ##
=======================================
  Coverage   93.34%   93.34%           
=======================================
  Files         378      378           
  Lines        7692     7692           
  Branches      206      203    -3     
=======================================
  Hits         7180     7180           
  Misses        512      512           

Flag	Coverage Δ
#scala_version_212	93.40% <ø> (ø)
#scala_version_213	93.10% <ø> (-0.03%)	⬇️

Impacted Files	Coverage Δ
core/src/main/scala/cats/ApplicativeError.scala	86.20% <ø> (ø)
.../src/main/scala/cats/syntax/applicativeError.scala	76.47% <ø> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b03df99...a32c7aa. Read the comment docs.

[tpolecat]

As a practical matter this is an improvement because exceptions don't seem to have parameterized types very often, but as you said this doesn't fix the underlying issue. I apologize for not lying down in front of the bulldozer for #2863 because this really shouldn't have made it into cats in the first place.

[travisbrown]

@tpolecat Out of curiosity, do you feel the same way about our catchOnly for Either and Validated, which does pretty much exactly the same thing as the version of attemptNarrow in this PR? In principle I definitely hate them both, but I also know catchOnly at least can be useful.

[tpolecat]

@travisbrown yes, same. I dislike them both, but I can accept that utility in the common case probably outweighs unsoundness in the general case. But it does seem like we should mention it in the doc.

[rossabaker]

If the eventual goal is to remove it, would it make more sense to just deprecate it?

[travisbrown]

@rossabaker I see this PR is a first step, to get attemptNarrow to the same level of safety as catchOnly now, for inclusion in the 2.2.0 release.

That puts us in a position to communicate the risks of attemptNarrow and catchOnly together (since they'll represent the same kind of use of runtime reflection), and to decide what to do about them together in the future (deprecate them both, make them both package-private, leave them, etc.).

For what it's worth I was looking at attemptNarrow yesterday because a colleague said "I just found attemptNarrow and it's really useful". It might be reasonable to decide it has a place in Cats—for now I just think we should make our two uses of class tags look the same, instead of having one be mostly safe except for really weird situations and the other completely unsafe.