Fix issue 263

[johnynek]

close #263

I made sure all the previous failures are no longer failures.

This fixes in a couple of ways:

1. add some of the static optimizations that we hit on some, but not all code paths
2. change some of the laws to relax them somewhat (mostly around overly strict equivalences in error cases)

This should be performance neutral or possibly slightly faster.

[codecov-commenter]

Codecov Report

Merging #296 (08e8254) into main (94cc55f) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head 08e8254 differs from pull request most recent head a9a1eed. Consider uploading reports for the commit a9a1eed to get more accurate results

@@            Coverage Diff             @@
##             main     #296      +/-   ##
==========================================
- Coverage   96.43%   96.41%   -0.03%     
==========================================
  Files           8        8              
  Lines        1011     1031      +20     
  Branches       81       95      +14     
==========================================
+ Hits          975      994      +19     
- Misses         36       37       +1     

Impacted Files	Coverage Δ
core/shared/src/main/scala/cats/parse/Parser.scala	96.29% <100.00%> (-0.04%)	⬇️

---

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 94cc55f...a9a1eed. Read the comment docs.

[regadas]

LGTM only that 🎨 nits

Also, we could update this one as well

cats-parse/core/shared/src/main/scala/cats/parse/Parser.scala

Lines 1651 to 1654 in a9a1eed

	case f @ (Impl.Fail() | Impl.FailWith(_)) =>
	// these are really Parser[Nothing]
	// but scala can't see that, so we cast
	f.asInstanceOf[Parser[String]]

[regadas]

nit 🎨

Suggested change

	case f @ Impl.Fail() => f.widen
	case f @ Impl.FailWith(_) => f.widen
	case f: Impl.Fail[A] => f.widen
	case f: Impl.FailWith[A] => f.widen

[regadas]

There are a few more cases below

[johnynek]

Curious why you like this change? I prefer the opposite. I think the compile to the same code don't they?

I don't like general reflective matching and to a reviewer the : style might be that, so they have to read a bit carefully.

But I'm interested to know your view on it.

[regadas]

You got me curious as well because my main motivation for this was based on the fact that this generated different bytecode before. However, after a quick check with this use case, I remembered things badly and they do in fact generate the same bytecode. That said, I'm totally ok with it 😄

You might be right on it needing careful reading but the current one imo has a slight "disadvantage" of keeping track of the number of args. It's not worth changing because of this.

[johnynek]

going to merge now... if you want to replace pattern matches with class name matches, we can certainly do that later but there are many examples of pattern matches and I'd like to understand better the motivation.