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

Improve error messages for the Focus macro. #1413

Merged
merged 5 commits into from
Dec 21, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
11 changes: 9 additions & 2 deletions build.sbt
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,12 @@ lazy val buildSettings = Seq(
Compile / console / scalacOptions -= "-Ywarn-unused:imports",
scalacOptions ++= {
if (tlIsScala3.value)
Seq("-source:3.0-migration", "-Ykind-projector", "-language:implicitConversions,higherKinds,postfixOps")
Seq(
"-source:3.0-migration",
"-Ykind-projector",
"-language:implicitConversions,higherKinds,postfixOps",
"-Wunused:all"
)
else
Seq(
"-Ymacro-annotations",
Expand Down Expand Up @@ -158,7 +163,9 @@ lazy val core = crossProject(JVMPlatform, JSPlatform, NativePlatform)
ProblemFilters.exclude[MissingClassProblem]("monocle.syntax.AsPrism"),
ProblemFilters.exclude[MissingClassProblem]("monocle.syntax.AsPrism$"),
ProblemFilters.exclude[MissingClassProblem]("monocle.syntax.AsPrismImpl"),
ProblemFilters.exclude[MissingClassProblem]("monocle.syntax.AsPrismImpl$")
ProblemFilters.exclude[MissingClassProblem]("monocle.syntax.AsPrismImpl$"),
// ignore mima for classes only used by `Focus` macro
ProblemFilters.exclude[DirectMissingMethodProblem]("monocle.internal.focus.*")
)
else Nil
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,19 +3,52 @@ package monocle.internal.focus
private[focus] trait ErrorHandling {
this: FocusBase =>

def errorMessage(error: FocusError): String = error match {
case FocusError.NotACaseClass(fromClass, fieldName) =>
s"Cannot generate Lens for field '$fieldName', because '$fromClass' is not a case class"
def errorReport(error: FocusError): (String, Option[Position]) = error match {
Copy link
Collaborator Author

@yilinwei yilinwei Dec 20, 2023

Choose a reason for hiding this comment

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

The dotty macro API has better support for errors now.

As a tangent, I need to see whether we can use the -explain flag which would be helpful to expand internal errors.

case FocusError.NotACaseClass(fromClass, fieldName, pos) =>
(
s"Cannot generate Lens for field '$fieldName', because '$fromClass' is not a case class",
Some(pos)
)
case FocusError.NotAConcreteClass(fromClass) =>
s"Expecting a concrete case class in the 'From' position; cannot reify type $fromClass"
(
s"Expecting a concrete case class in the 'From' position; cannot reify type $fromClass",
None
)
case FocusError.NotASimpleLambdaFunction =>
s"Expecting a lambda function that directly accesses a field. Example: `Focus[Address](_.streetNumber)`"
case FocusError.CouldntUnderstandKeywordContext => s"Internal error in monocle.Focus; cannot access special syntax."
(
s"Expecting a lambda function that directly accesses a field. Example: `Focus[Address](_.streetNumber)`",
None
)
case FocusError.CouldntUnderstandKeywordContext =>
(
s"Internal error in monocle.Focus; cannot access special syntax.",
None
)
case FocusError.DidNotDirectlyAccessArgument(argName) =>
s"Expecting a lambda function that directly accesses the argument; other variable `$argName` found. Example: `Focus[Address](_.streetNumber)`"
case FocusError.ComposeMismatch(type1, type2) => s"Could not compose $type1.andThen($type2)"
case FocusError.UnexpectedCodeStructure(code) => s"Unexpected code structure: $code"
case FocusError.CouldntFindFieldType(fromType, fieldName) => s"Couldn't find type for $fromType.$fieldName"
case FocusError.InvalidDowncast(fromType, toType) => s"Type '$fromType' could not be cast to '$toType'"
(
s"Expecting a lambda function that directly accesses the argument; other variable `$argName` found. Example: `Focus[Address](_.streetNumber)`",
None
)
case FocusError.ComposeMismatch(type1, type2) =>
(
s"Could not compose $type1.andThen($type2)",
None
)
case FocusError.UnexpectedCodeStructure(code) =>
(
s"Unexpected code structure: $code",
None
)
case FocusError.CouldntFindFieldType(fromType, fieldName, pos) =>
(
s"Couldn't find type for $fromType.$fieldName",
Some(pos)
)
case FocusError.InvalidDowncast(fromType, toType) =>
(
s"Type '$fromType' could not be cast to '$toType'",
None
)
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ private[focus] trait FocusBase {

type Term = macroContext.reflect.Term
type TypeRepr = macroContext.reflect.TypeRepr
type Position = macroContext.reflect.Position

case class LambdaConfig(argName: String, lambdaBody: Term)

Expand Down Expand Up @@ -43,13 +44,13 @@ private[focus] trait FocusBase {
}

enum FocusError {
case NotACaseClass(className: String, fieldName: String)
case NotACaseClass(className: String, fieldName: String, pos: Position)
case NotAConcreteClass(className: String)
case DidNotDirectlyAccessArgument(argName: String)
case NotASimpleLambdaFunction
case CouldntUnderstandKeywordContext
case UnexpectedCodeStructure(code: String)
case CouldntFindFieldType(fromType: String, fieldName: String)
case CouldntFindFieldType(fromType: String, fieldName: String, pos: Position)
case ComposeMismatch(type1: String, type2: String)
case InvalidDowncast(fromType: String, toType: String)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,9 @@ private[focus] class FocusImpl(val macroContext: Quotes)

generatedCode match {
case Right(code) => code.asExpr
case Left(error) => report.error(errorMessage(error)); '{ ??? }
case Left(error) =>
val (msg, pos) = errorReport(error)
report.errorAndAbort(msg, pos.getOrElse(lambda.asTerm.pos))
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,16 +26,24 @@ private[focus] trait SelectParserBase extends ParserBase {
case None => FocusError.NotAConcreteClass(tpe.show).asResult
}

def getFieldType(fromType: TypeRepr, fieldName: String): FocusResult[TypeRepr] = {
// We need to do this to support tuples, because even though they conform as case classes in other respects,
// for some reason their field names (_1, _2, etc) have a space at the end, ie `_1 `.
def getTrimmedFieldSymbol(fromTypeSymbol: Symbol): Symbol =
fromTypeSymbol.memberFields.find(_.name.trim == fieldName).getOrElse(Symbol.noSymbol)
private val tupleFieldPattern = "^_[0-9]+$".r

def getFieldType(fromType: TypeRepr, fieldName: String, pos: Position): FocusResult[TypeRepr] = {
def getFieldSymbol(fromTypeSymbol: Symbol): Symbol = {
// We need to do this to support tuples, because even though they conform as case classes in other respects,
// for some reason their field names (_1, _2, etc) have a space at the end, ie `_1 `.
val f: String => String =
Copy link
Collaborator Author

@yilinwei yilinwei Dec 20, 2023

Choose a reason for hiding this comment

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

This fixes a silent error if we have something like

case class Foo(x: Int, `x `: Int)

since identifiers with leading/trailing spaces are not equivalent.

if (fromType <:< TypeRepr.of[Tuple] && tupleFieldPattern.matches(fieldName))
_.trim
else
identity
fromTypeSymbol.fieldMembers.find(s => f(s.name) == fieldName).getOrElse(Symbol.noSymbol)
}

getClassSymbol(fromType).flatMap { fromTypeSymbol =>
getTrimmedFieldSymbol(fromTypeSymbol) match {
getFieldSymbol(fromTypeSymbol) match {
case FieldType(possiblyTypeArg) => Right(swapWithSuppliedType(fromType, possiblyTypeArg))
case _ => FocusError.CouldntFindFieldType(fromType.show, fieldName).asResult
case _ => FocusError.CouldntFindFieldType(fromType.show, fieldName, pos).asResult
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,19 +14,18 @@ private[focus] trait SelectFieldParser {

case Select(CaseClass(remainingCode), fieldName) =>
val fromType = getType(remainingCode)
val action = getFieldAction(fromType, fieldName)
val action = getFieldAction(fromType, fieldName, term.pos)
val remainingCodeWithAction = action.map(a => (RemainingCode(remainingCode), a))
Some(remainingCodeWithAction)

case Select(remainingCode, fieldName) =>
Some(FocusError.NotACaseClass(remainingCode.tpe.show, fieldName).asResult)

Some(FocusError.NotACaseClass(remainingCode.tpe.widen.show, fieldName, term.pos).asResult)
case _ => None
}
}

private def getFieldAction(fromType: TypeRepr, fieldName: String): FocusResult[FocusAction] =
getFieldType(fromType, fieldName).flatMap { toType =>
private def getFieldAction(fromType: TypeRepr, fieldName: String, pos: Position): FocusResult[FocusAction] =
getFieldType(fromType, fieldName, pos).flatMap { toType =>
Right(FocusAction.SelectField(fieldName, fromType, getSuppliedTypeArgs(fromType), toType))
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,17 +14,17 @@ private[focus] trait SelectOnlyFieldParser {

case Select(CaseClass(remainingCode), fieldName) if hasOnlyOneField(remainingCode) =>
val fromType = getType(remainingCode)
val action = getFieldAction(fromType, fieldName)
val action = getFieldAction(fromType, fieldName, term.pos)
val remainingCodeWithAction = action.map(a => (RemainingCode(remainingCode), a))
Some(remainingCodeWithAction)

case _ => None
}
}

private def getFieldAction(fromType: TypeRepr, fieldName: String): FocusResult[FocusAction] =
private def getFieldAction(fromType: TypeRepr, fieldName: String, pos: Position): FocusResult[FocusAction] =
for {
toType <- getFieldType(fromType, fieldName)
toType <- getFieldType(fromType, fieldName, pos)
companion <- getCompanionObject(fromType)
supplied = getSuppliedTypeArgs(fromType)
} yield FocusAction.SelectOnlyField(fieldName, fromType, supplied, companion, toType)
Expand Down