-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix: simplify types before checking if implicit is underspecified #24659
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
base: main
Are you sure you want to change the base?
Conversation
| tp.isRef(defn.NothingClass) || tp.isRef(defn.NullClass) || (tp eq NoPrefix) | ||
|
|
||
| private def isUnderspecified(tp: Type): Boolean = tp.stripTypeVar match | ||
| private def isUnderspecified(tp: Type): Boolean = tp.simplified match |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm willing to bet that we also need a dealiasing here.
| || isUnderSpecifiedArgument(tp.argType.widen) | ||
| case _ => | ||
| tp.isAny || tp.isAnyRef | ||
| case tp => tp.isAny || tp.isAnyRef || tp.isAnyVal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a specification point of view, it would have been nice to check tp.isTopLevel but that means we need to also include scala.Matchable in the list of underspecified types.
| | One of the following imports might fix the problem: | ||
| | | ||
| | import scala.math.BigDecimal.int2bigDecimal | ||
| | import scala.math.BigInt.int2bigInt | ||
| | import scala.math.Numeric.IntIsIntegral.mkNumericOps | ||
| | import scala.math.Numeric.IntIsIntegral.mkOrderingOps | ||
| | import scala.math.Ordering.Int.mkOrderingOps | ||
| | import scala.math.Integral.Implicits.infixIntegralOps | ||
| | import scala.math.Ordered.orderingToOrdered | ||
| | import scala.math.Ordering.Implicits.infixOrderingOps | ||
| | import scala.math.Numeric.Implicits.infixNumericOps | ||
| | import scala.reflect.Selectable.reflectiveSelectable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None of them will ever help. An improvement would be to get rid of this list when we are underspecified.
| implicit def _2: Int = 0 | ||
|
|
||
| println(implicitly[AnyVal]) | ||
| println(implicitly[AnyVal]) // error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test was completely bogus before, not only it should have been underspecified but also, if we say AnyVal are not underspecified, it should have been an ambiguous resolution (what actually Scala 2.13.16 says).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the changes in the Hylolib suite, my changes somehow triggered the checks for infix in
| assert(h0 eq 1) |
which is actually a legitimate error. I went ahead and migrate those tests to use infix def since this code base should set an example of coding styles.
Closes #21304