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

Using irrefutable extractors for Scala structs #340

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

ncreep
Copy link

@ncreep ncreep commented Nov 22, 2020

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
Copy link

CLAassistant commented Nov 22, 2020

CLA assistant check
All committers have signed the CLA.

@ryanoneill
Copy link
Contributor

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
Copy link
Author

ncreep commented Nov 24, 2020

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

@codecov-io
Copy link

codecov-io commented Nov 25, 2020

Codecov Report

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

Impacted file tree graph

@@           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
Copy link
Contributor

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
Copy link
Author

ncreep commented Nov 25, 2020

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
Copy link
Contributor

ryanoneill commented Nov 25, 2020

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
Copy link
Author

ncreep commented Nov 25, 2020

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.

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

Successfully merging this pull request may close these issues.

4 participants