Skip to content

Fix/18681 unreduced summon from fallback #19490

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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Fix/18681 unreduced summon from fallback #19490

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6deac90
draft: better error message for inline summonfrom
i10416 Jan 19, 2024
eb6db96
Merge branch 'master' into feature/implicits-not-found-for-summonfrom
i10416 Jan 19, 2024
c148144
fix(#18681): un-reduced summonFrom fallback
i10416 Jan 19, 2024
b344eb6
Merge branch 'master' into fix/18681-un-reduced-summonFrom-fallback
i10416 Feb 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
166 changes: 106 additions & 60 deletions compiler/src/dotty/tools/dotc/inlines/InlineReducer.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import Names.TermName
import NameKinds.{InlineAccessorName, InlineBinderName, InlineScrutineeName}
import config.Printers.inlining
import util.SimpleIdentityMap

import dotty.tools.dotc.reporting.Message
import collection.mutable

/** A utility class offering methods for rewriting inlined code */
Expand Down Expand Up @@ -146,17 +146,16 @@ class InlineReducer(inliner: Inliner)(using Context):
}
binding1.withSpan(call.span)
}

/** The result type of reducing a match. It consists optionally of a list of bindings
* for the pattern-bound variables and the RHS of the selected case.
* Returns `None` if no case was selected.
* Returns `Left` with an optional message for implicitNotFound if no case was selected.
*/
type MatchRedux = Option[(List[MemberDef], Tree)]
type MatchRedux = Either[Option[Message],(List[MemberDef], Tree)]

/** Same as MatchRedux, but also includes a boolean
* that is true if the guard can be checked at compile time.
*/
type MatchReduxWithGuard = Option[(List[MemberDef], Tree, Boolean)]
type MatchReduxWithGuard = Either[Option[Message],(List[MemberDef], Tree, Boolean)]

/** Reduce an inline match
* @param mtch the match tree
Expand All @@ -173,13 +172,14 @@ class InlineReducer(inliner: Inliner)(using Context):
val isImplicit = scrutinee.isEmpty

/** Try to match pattern `pat` against scrutinee reference `scrut`. If successful add
* bindings for variables bound in this pattern to `caseBindingMap`.
* bindings for variables bound in this pattern to `caseBindingMap` and return None.
* Otherwise, it returns optional error message.
*/
def reducePattern(
caseBindingMap: mutable.ListBuffer[(Symbol, MemberDef)],
scrut: TermRef,
pat: Tree
)(using Context): Boolean = {
)(using Context): Option[Option[Message]] = {

/** Create a binding of a pattern bound variable with matching part of
* scrutinee as RHS and type that corresponds to RHS.
Expand All @@ -193,22 +193,28 @@ class InlineReducer(inliner: Inliner)(using Context):
val copied = sym.copy(info = TypeAlias(alias), coord = sym.coord).asType
caseBindingMap += ((sym, TypeDef(copied)))
}

def searchImplicit(sym: TermSymbol, tpt: Tree) = {

/**
* @return
* None if implicit search finds an instance or fails with a hard error.
* Otherwise, return the error message for missing implicit instance.
*/
def searchImplicit(sym: TermSymbol, tpt: Tree): Option[Message] = {
val evTyper = new Typer(ctx.nestingLevel + 1)
val evCtx = ctx.fresh.setTyper(evTyper)
inContext(evCtx) {
val evidence = evTyper.inferImplicitArg(tpt.tpe, tpt.span)
evidence.tpe match {
case fail: Implicits.AmbiguousImplicits =>
report.error(evTyper.missingArgMsg(evidence, tpt.tpe, ""), tpt.srcPos)
true // hard error: return true to stop implicit search here
None // hard error: return None to stop implicit search here
case fail: Implicits.SearchFailureType =>
false
val noImplicitMsg = evTyper.missingArgMsg(evidence, tpt.tpe, "")
Some(noImplicitMsg)
case _ =>
//inlining.println(i"inferred implicit $sym: ${sym.info} with $evidence: ${evidence.tpe.widen}, ${evCtx.gadt.constraint}, ${evCtx.typerState.constraint}")
newTermBinding(sym, evidence)
true
None
}
}
}
Expand Down Expand Up @@ -280,48 +286,62 @@ class InlineReducer(inliner: Inliner)(using Context):
case Typed(pat1, tpt) =>
val typeBinds = getTypeBindsMap(pat1, tpt)
registerAsGadtSyms(typeBinds)
scrut <:< tpt.tpe && {
if (scrut <:< tpt.tpe) then
addTypeBindings(typeBinds)
reducePattern(caseBindingMap, scrut, pat1)
}
else
Some(None)
case pat @ Bind(name: TermName, Typed(_, tpt)) if isImplicit =>
val typeBinds = getTypeBindsMap(tpt, tpt)
registerAsGadtSyms(typeBinds)
searchImplicit(pat.symbol.asTerm, tpt) && {
addTypeBindings(typeBinds)
true
}
searchImplicit(pat.symbol.asTerm, tpt) match
// found or hard error
case None =>
addTypeBindings(typeBinds)
None
case Some(msg) =>
Some(Some(msg))

case pat @ Bind(name: TermName, body) =>
reducePattern(caseBindingMap, scrut, body) && {
if (name != nme.WILDCARD) newTermBinding(pat.symbol.asTerm, ref(scrut))
true
}
reducePattern(caseBindingMap,scrut,body) match
case None if name != nme.WILDCARD =>
newTermBinding(pat.symbol.asTerm, ref(scrut))
None
case None =>
None
case Some(msg) =>
Some(msg)

case Ident(nme.WILDCARD) =>
true
None
case pat: Literal =>
scrut.widenTermRefExpr =:= pat.tpe
if scrut.widenTermRefExpr =:= pat.tpe then
None
else Some(None)
case pat: RefTree =>
scrut =:= pat.tpe ||
scrut.classSymbol.is(Module) && scrut.widen =:= pat.tpe.widen && {
scrut.prefix match {
case _: SingletonType | NoPrefix => true
case _ => false
}
}
if(scrut =:= pat.tpe) then
None
else if (scrut.classSymbol.is(Module) && scrut.widen =:= pat.tpe.widen) then
scrut.prefix match
case _: SingletonType | NoPrefix => None
case _ => Some(None)
else Some(None)
case UnApply(unapp, _, pats) =>
unapp.tpe.widen match {
case mt: MethodType if mt.paramInfos.length == 1 =>

def reduceSubPatterns(pats: List[Tree], selectors: List[Tree]): Boolean = (pats, selectors) match {
case (Nil, Nil) => true
def reduceSubPatterns(pats: List[Tree], selectors: List[Tree]): Option[Option[Message]] = (pats, selectors) match {
case (Nil, Nil) => None
case (pat :: pats1, selector :: selectors1) =>
val elem = newSym(InlineBinderName.fresh(), Synthetic, selector.tpe.widenInlineScrutinee).asTerm
val rhs = constToLiteral(selector)
elem.defTree = rhs
caseBindingMap += ((NoSymbol, ValDef(elem, rhs).withSpan(elem.span)))
reducePattern(caseBindingMap, elem.termRef, pat) &&
reduceSubPatterns(pats1, selectors1)
case _ => false
reducePattern(caseBindingMap, elem.termRef, pat) match
case None => reduceSubPatterns(pats1, selectors1)
case Some(msg) => Some(msg)

case _ => Some(None)
}

val paramType = mt.paramInfos.head
Expand All @@ -333,17 +353,42 @@ class InlineReducer(inliner: Inliner)(using Context):
val selectors =
for (accessor <- caseAccessors)
yield constToLiteral(reduceProjection(ref(scrut).select(accessor).ensureApplied))
caseAccessors.length == pats.length && reduceSubPatterns(pats, selectors)
if caseAccessors.length == pats.length then reduceSubPatterns(pats, selectors)
else Some(None)
}
else false
else Some(None)
case _ =>
false
Some(None)
}
case Alternative(pats) =>
pats.exists(reducePattern(caseBindingMap, scrut, _))
/**
* Apply `reducePattern` on `caseBindingMap` until it finds a match.
*
* @param ps a list of patterns to match
* @param theLastMsg the last message for missing implicit instance
*
* @return `None` if it finds a match. Otherwise, it returns optional error message.
*/
def applyReduceUntilFind(
ps: List[Tree],
theLastMsg: Option[Message] = None
): Option[Option[Message]] =
ps match
case Nil => Some(theLastMsg)
case p :: ps =>
reducePattern(caseBindingMap, scrut, p) match
// it finds a match or fails with hard error
case None => None
// implicit search fails with message
case Some(msg @ Some(_)) =>
applyReduceUntilFind(ps, msg)
case Some(None) =>
applyReduceUntilFind(ps, theLastMsg)
end applyReduceUntilFind
applyReduceUntilFind(pats)
case tree: Inlined if tree.inlinedFromOuterScope =>
reducePattern(caseBindingMap, scrut, tree.expansion)
case _ => false
case _ => Some(None)
}
}

Expand All @@ -368,29 +413,30 @@ class InlineReducer(inliner: Inliner)(using Context):

if (!isImplicit) caseBindingMap += ((NoSymbol, scrutineeBinding))
val gadtCtx = ctx.fresh.setFreshGADTBounds.addMode(Mode.GadtConstraintInference)
if (reducePattern(caseBindingMap, scrutineeSym.termRef, cdef.pat)(using gadtCtx)) {
val (caseBindings, from, to) = substBindings(caseBindingMap.toList, mutable.ListBuffer(), Nil, Nil)
val (guardOK, canReduceGuard) =
if cdef.guard.isEmpty then (true, true)
else typer.typed(cdef.guard.subst(from, to), defn.BooleanType) match {
case ConstantValue(v: Boolean) => (v, true)
case _ => (false, false)
}
if guardOK then Some((caseBindings.map(_.subst(from, to)), cdef.body.subst(from, to), canReduceGuard))
else if canReduceGuard then None
else Some((caseBindings.map(_.subst(from, to)), cdef.body.subst(from, to), canReduceGuard))
}
else None
reducePattern(caseBindingMap,scrutineeSym.termRef,cdef.pat)(using gadtCtx) match
case Some(msg) => Left(msg)
case None => {
val (caseBindings, from, to) = substBindings(caseBindingMap.toList, mutable.ListBuffer(), Nil, Nil)
val (guardOK, canReduceGuard) =
if cdef.guard.isEmpty then (true, true)
else typer.typed(cdef.guard.subst(from, to), defn.BooleanType) match {
case ConstantValue(v: Boolean) => (v, true)
case _ => (false, false)
}
if guardOK then Right((caseBindings.map(_.subst(from, to)), cdef.body.subst(from, to), canReduceGuard))
else if canReduceGuard then Left(None)
else Right((caseBindings.map(_.subst(from, to)), cdef.body.subst(from, to), canReduceGuard))
}
}

def recur(cases: List[CaseDef]): MatchRedux = cases match {
case Nil => None
def recur(cases: List[CaseDef], theLastMsg: Option[Message] = None): MatchRedux = cases match
case Nil => Left(theLastMsg)
case cdef :: cases1 =>
reduceCase(cdef) match
case None => recur(cases1)
case r @ Some((caseBindings, rhs, canReduceGuard)) if canReduceGuard => Some((caseBindings, rhs))
case _ => None
}
case Left(None) => recur(cases1, theLastMsg)
case Left(msg @Some(_)) => recur(cases1, msg)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it fail fast when a case results in missing implicit instance rather than continuing remaining cases?

case r @ Right((caseBindings, rhs, canReduceGuard)) if canReduceGuard => Right((caseBindings, rhs))
case _ => Left(theLastMsg)

recur(cases)
}
Expand Down
22 changes: 13 additions & 9 deletions compiler/src/dotty/tools/dotc/inlines/Inliner.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package dotty.tools
package dotc
package inlines

import dotty.tools.dotc.reporting.NoExplanation
import ast.*, core.*
import Flags.*, Symbols.*, Types.*, Decorators.*, Constants.*, Contexts.*
import StdNames.nme
Expand Down Expand Up @@ -861,7 +862,7 @@ class Inliner(val call: tpd.Tree)(using Context):
}
val selType = if (sel.isEmpty) wideSelType else selTyped(sel)
reduceInlineMatch(sel, selType, cases.asInstanceOf[List[CaseDef]], this) match {
case Some((caseBindings, rhs0)) =>
case Right((caseBindings, rhs0)) =>
// drop type ascriptions/casts hiding pattern-bound types (which are now aliases after reducing the match)
// note that any actually necessary casts will be reinserted by the typing pass below
val rhs1 = rhs0 match {
Expand All @@ -887,17 +888,20 @@ class Inliner(val call: tpd.Tree)(using Context):
|--- to:
|$rhs""")
typedExpr(rhs, pt)
case None =>
case Left(maybeMeg) =>

def guardStr(guard: untpd.Tree) = if (guard.isEmpty) "" else i" if $guard"
def patStr(cdef: untpd.CaseDef) = i"case ${cdef.pat}${guardStr(cdef.guard)}"
val msg =
if (tree.selector.isEmpty)
em"""cannot reduce summonFrom with
| patterns : ${tree.cases.map(patStr).mkString("\n ")}"""
else
em"""cannot reduce inline match with
| scrutinee: $sel : ${selType}
val msg = maybeMeg match
case Some(noImplicitMsg) => em"${noImplicitMsg.message}"
case None =>
if (tree.selector.isEmpty)
em"""cannot reduce summonFrom with
| patterns : ${tree.cases.map(patStr).mkString("\n ")}"""
else
em"""cannot reduce inline match with
| scrutinee: $sel : ${selType}
| patterns : ${tree.cases.map(patStr).mkString("\n ")}"""
errorTree(tree, msg)
}
}
Expand Down
24 changes: 24 additions & 0 deletions tests/neg/summonFrom-implicit-not-found.check
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
-- Error: tests/neg/summonFrom-implicit-not-found.scala:8:8 ------------------------------------------------------------
8 |val x = summonMissing // error
| ^^^^^^^^^^^^^
| there is no Missing!
|---------------------------------------------------------------------------------------------------------------------
|Inline stack trace
|- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
|This location contains code that was inlined from summonFrom-implicit-not-found.scala:4
4 |inline def summonMissing = compiletime.summonFrom {
| ^
5 | case m: Missing => m
6 |}
---------------------------------------------------------------------------------------------------------------------
-- [E172] Type Error: tests/neg/summonFrom-implicit-not-found.scala:9:8 ------------------------------------------------
9 |val y = summonMissing2 // error
| ^^^^^^^^^^^^^^
| there is no Missing!
|---------------------------------------------------------------------------------------------------------------------
|Inline stack trace
|- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
|This location contains code that was inlined from summonFrom-implicit-not-found.scala:7
7 |inline def summonMissing2 = compiletime.summonInline[Missing]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
---------------------------------------------------------------------------------------------------------------------
9 changes: 9 additions & 0 deletions tests/neg/summonFrom-implicit-not-found.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
@annotation.implicitNotFound("there is no Missing!")
trait Missing

inline def summonMissing = compiletime.summonFrom {
case m: Missing => m
}
Copy link
Contributor

@nicolasstucki nicolasstucki Jan 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder what should be the error for

@annotation.implicitNotFound("there is no Missing1!")
trait Missing1
@annotation.implicitNotFound("there is no Missing2!")
trait Missing2
inline def summonMissing = compiletime.summonFrom {
  case m: Missing1 => m
  case m: Missing2 => m
}
val x = summonMissing

We can choose the first one, last one, concatenate the messages. Not sure what would be best. This might also mean that we loose information in which implicits are accepted here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case it might be better to do

inline def summonMissing = compiletime.summonFrom {
  case m: Missing1 => m
  case m: Missing2 => m
  case _ => compiletime.error("no Missing1 or Missing2 found")
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently I think that we should use compiletime.error over implicitNotFound. The error messages in the implicitNotFound usually assume that this is the only implicit beeing summoned. Reusing implicitNotFound might lead to error messages that are out of context.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I also wonder which error message(s) it should show.

I choosed the last message rather than accumulating errors with the aim of minimizing diff in the dotty codebase.

Copy link
Contributor Author

@i10416 i10416 Jan 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reusing ...　that are out of context

I agree with you. If we concat multiple implicitNotFounds that encourage users to import something at the same time, users could follow the instruction only to find ambiguous implicits, which could confuse users, but suggest all the possible actions they can take.

Copy link
Contributor Author

@i10416 i10416 Jan 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After all, it feels reasonable to keep the current behavior, doesn't it?

Should/Could we report unexhaustiveness as error in summonFrom with an error message better than "cannot reduce summonFrom with ..." in the similar way as we warn on unexhaustive pattern matches to enums?
(e.g. This does not compile because no instance of Missing1 or Missing2 is found.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reminiscent of implicitNotFound on parent classes. IIRC scala 2 appends (accumulates); there is an open dotty ticket.

Copy link
Contributor Author

@i10416 i10416 Jan 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you think it is better to keep unreduced summon from... to prevent users from getting confused, I will close this PR and address other issues. Otherwise, I will add changes to accumulate error messages from implicitsNotFound annotations.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should/Could we report unexhaustiveness as error in summonFrom with an error message better than "cannot reduce summonFrom with ..." in the similar way as we warn on unexhaustive pattern matches to enums?
(e.g. This does not compile because no instance of Missing1 or Missing2 is found.)

That sounds like a better solution. I assume that we will still see the summonForm in the inlining stack trace. Therefore we would have a simple error message followed by the more detailed explanation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the feedback. I'll address this.

inline def summonMissing2 = compiletime.summonInline[Missing]
val x = summonMissing // error
val y = summonMissing2 // error