Using irrefutable extractors for Scala structs

[ncreep]

Problem

Up until Scala 2.13.4, having custom extractors in a pattern match disables exhaustivity checks by the compiler. This is true, unless the extractor is "irrefutable", which means that its type-signature contains a Some rather than an Option. For more details, see this.

As currently implemented, Thrift structs generate Scala code with "refutable" custom extractors, which can lead to inadvertent and surprising cases where people expect the compiler to do full exhaustivity checking, but that doesn't happen, leading to runtime exceptions.

Solution

This pull request updates the relevant mustache template to have Some in the unapply signature, thus turning the extractor into an irrefutable one.

(Note, I'm not quite sure how to go about tests in this project, any pointers would be welcome.)

Thanks

[CLAassistant]

All committers have signed the CLA.

[ryanoneill]

Hi @ncreep. I ran this through an initial pass internally, and there are some significant challenges for us (Twitter) to work through in our own code for this to ship. I'm not sure yet what that will fully entail, but I don't see this as something which will be quick to resolve. We will keep you posted as we learn more.

[ncreep]

Thanks for looking into this, let me know if there's anything I can do to help.

[codecov-io]

Codecov Report

Merging #340 (5134fb0) into develop (39870f0) will not change coverage. The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop     #340   +/-   ##
========================================
  Coverage    51.85%   51.85%           
========================================
  Files           40       40           
  Lines         1130     1130           
  Branches        83       83           
========================================
  Hits           586      586           
  Misses         544      544           

---

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 39870f0...5134fb0. Read the comment docs.

[ryanoneill]

On first pass, this causes tens of thousands of failures internally. I'm seeing if I can knock that number down to something reasonable quite quickly. If not, we will be unable to ship this change for now. We should know more sometime early next week.

[ncreep]

Wow...

Out of curiosity, what sort of failures does this cause? I would've imagined that it will only trigger pattern-match exhaustiveness warnings and maybe some type-inference issues (unless reflection is somehow involved).

[ryanoneill]

For us, it causes pattern-match exhaustiveness failures. In a few cases that I've seen so far, I've seen code that handles Return(xyz(something)) and Throw(exc) and that logically covers all scenarios, yet the exhaustivity checker will complain that that code isn't handling Return(_) which is right, but not going to happen in that particular case.

One instance of that in a shared library used by many leaf nodes can lead to ~5000 target failures. If that's the case, and it's really less than a few hundred files causing problems, then we maybe can ship this. If it's really thousands of instances of this occurring though, it's likely not something we can do at the moment.

[ncreep]

Ah, that makes sense.

Too bad if this won't go in. But seeing how you probably invested more time into this then I spent on my changes, I appreciate the effort.