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

Loosen Typeable restriction #900

Merged

Conversation

jeremyrsmith
Copy link
Contributor

Fixes #812

Don't reject materializing a Typeable instance due to the presence of a non-case-accessor value, unless its type is abstract. The abstract type is what makes it unsafe, so we should only reject if that's the case.

Fixes milessabin#812

Don't reject materializing a `Typeable` instance due to the presence of
a non-case-accessor value, unless its type is abstract.
@codecov-io
Copy link

codecov-io commented Jun 11, 2019

Codecov Report

Merging #900 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #900   +/-   ##
======================================
  Coverage    87.9%   87.9%           
======================================
  Files          65      65           
  Lines        1497    1497           
  Branches        5       5           
======================================
  Hits         1316    1316           
  Misses        181     181
Impacted Files Coverage Δ
core/src/main/scala/shapeless/typeable.scala 0% <ø> (ø) ⬆️

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 468ac7e...66f403a. Read the comment docs.

@jeremyrsmith
Copy link
Contributor Author

Interestingly isAbstract seems not to work in Scala 2.10 (hence the failure). Could be a macro-compat issue worth looking at (if Scala 2.10 still matters)

@jeremyrsmith jeremyrsmith force-pushed the fix-aggressive-typeable-rejection branch from 7f08f9b to 66f403a Compare June 11, 2019 10:15
@jeremyrsmith
Copy link
Contributor Author

To clarify the issue, this check was added in #575:

val nonCaseAccessor = tpe.decls.exists {
          case sym: TermSymbol if !sym.isCaseAccessor && (sym.isVal || sym.isVar ||
              (sym.isParamAccessor && !(sym.accessed.isTerm && sym.accessed.asTerm.isCaseAccessor))) => true
          case _ => false
        }
        if (nonCaseAccessor) {
          // there is a symbol, which is not a case accessor but a val,
          // var or param, so we won't be able to type check it safely:
          c.abort(c.enclosingPosition, s"No default Typeable for parametrized type $tpe")
        }

What this amounts to is rejecting any case class that defines a val in its body. For example, I won't be able to get a Typeable instance for:

case class Foo(a: Int) {
  val foo = 22
}

This is a bit too strict (and leads to bug #812). Instead, there should also be a check as to whether the value's type is parametric. So this PR adds a check that this is the case.

It's possible that this is still even too strict, as in here:

case class Foo[A](a: A) {
  val aa = a
}

Here, the presence of aa does not render the whole thing un-Typeable, because aa depends only on a which will be checked during a cast. This will still fail to get an instance after this PR. But, it should be a good improvement.

@milessabin
Copy link
Owner

Yup, this LGTM! You've won your t-shirt!

@milessabin milessabin merged commit 1c7125d into milessabin:master Jun 11, 2019
joroKr21 pushed a commit to joroKr21/shapeless that referenced this pull request Apr 23, 2021
* Loosen typeable restrictions

Fixes milessabin#812

Don't reject materializing a `Typeable` instance due to the presence of
a non-case-accessor value, unless its type is abstract.

* Fix Scala 2.10 issue
@joroKr21 joroKr21 added this to the shapeless-2.4.0 milestone Apr 23, 2021
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

Successfully merging this pull request may close these issues.

Typeable failing in 2.3.3 for code that works in 2.3.2
4 participants