Skip to content

Commit

Permalink
Better context info in Type_Error raised from return type checks (#…
Browse files Browse the repository at this point in the history
…8566)

- Followup to #8502 that adds better error messages
  • Loading branch information
radeusgd authored Dec 20, 2023
1 parent d56b800 commit dfdb547
Show file tree
Hide file tree
Showing 19 changed files with 115 additions and 89 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,9 @@ type Type_Error
Arguments:
- expected: The expected type at the error location.
- actual: The actual type at the error location.
- name: The name of the argument whose type is mismatched.
Error expected actual name
- comment: Description of the value that was being checked,
e.g. function argument, result or an arbitrary expression.
Error expected actual comment

## PRIVATE
Convert the Type_Error error to a human-readable format.
Expand All @@ -78,7 +79,7 @@ type Type_Error
_ -> ". Try to apply " + (missing_args.join ", ") + " arguments"
_ -> tpe.to_text

"Type error: expected `"+self.name+"` to be "+self.expected.to_display_text+", but got "+type_of_actual+"."
"Type error: expected "+self.comment+" to be "+self.expected.to_display_text+", but got "+type_of_actual+"."

@Builtin_Type
type Compile_Error
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
@Persistable(clazz = GatherDiagnostics.DiagnosticsMeta.class, id = 1114)
@Persistable(clazz = DocumentationComments.Doc.class, id = 1115)
@Persistable(clazz = AliasAnalysis$Info$Occurrence.class, id = 1116)
@Persistable(clazz = TypeSignatures.Signature.class, id = 1117)
@Persistable(clazz = TypeSignatures.Signature.class, id = 2117)
@Persistable(clazz = ModuleAnnotations.Annotations.class, id = 1118)
@Persistable(clazz = AliasAnalysis$Info$Scope$Root.class, id = 1120)
@Persistable(clazz = DataflowAnalysis$DependencyInfo$Type$Static.class, id = 1121)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -488,7 +488,7 @@ final class SuggestionBuilder[A: IndexedSource](
typeSignature: Option[TypeSignatures.Metadata]
): Vector[TypeArg] =
typeSignature match {
case Some(TypeSignatures.Signature(typeExpr)) =>
case Some(TypeSignatures.Signature(typeExpr, _)) =>
buildTypeSignature(typeExpr)
case _ =>
Vector()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -393,7 +393,7 @@ case object DataflowAnalysis extends IRPass {
*/
def analyseType(typ: Type, info: DependencyInfo): Type = {
typ match {
case asc @ Type.Ascription(typed, signature, _, _, _) =>
case asc @ Type.Ascription(typed, signature, _, _, _, _) =>
val ascrDep = asStatic(asc)
val typedDep = asStatic(typed)
val sigDep = asStatic(signature)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ case object ComplexType extends IRPass {
): List[Definition] = {
var unusedSig: Option[Type.Ascription] = None
val sig = lastSignature match {
case Some(Type.Ascription(typed, _, _, _, _)) =>
case Some(Type.Ascription(typed, _, _, _, _, _)) =>
typed match {
case literal: Name.Literal =>
if (name.name == literal.name) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ case object SuspendedArguments extends IRPass {
method.body match {
case lam @ Function.Lambda(args, body, _, _, _, _) =>
method.getMetadata(TypeSignatures) match {
case Some(Signature(signature)) =>
case Some(Signature(signature, _)) =>
val newArgs = computeSuspensions(args.drop(1), signature)
if (newArgs.head.suspended) {
errors.Conversion(
Expand Down Expand Up @@ -163,7 +163,7 @@ case object SuspendedArguments extends IRPass {
body match {
case lam @ Function.Lambda(args, lamBody, _, _, _, _) =>
explicit.getMetadata(TypeSignatures) match {
case Some(Signature(signature)) =>
case Some(Signature(signature, _)) =>
val newArgs = computeSuspensions(
args.drop(1),
signature
Expand Down Expand Up @@ -214,7 +214,7 @@ case object SuspendedArguments extends IRPass {
expression.transformExpressions {
case bind @ Expression.Binding(_, expr, _, _, _) =>
val newExpr = bind.getMetadata(TypeSignatures) match {
case Some(Signature(signature)) =>
case Some(Signature(signature, _)) =>
expr match {
case lam @ Function.Lambda(args, body, _, _, _, _) =>
lam.copy(
Expand All @@ -229,7 +229,7 @@ case object SuspendedArguments extends IRPass {
bind.copy(expression = newExpr)
case lam @ Function.Lambda(args, body, _, _, _, _) =>
lam.getMetadata(TypeSignatures) match {
case Some(Signature(signature)) =>
case Some(Signature(signature, _)) =>
lam.copy(
arguments = computeSuspensions(args, signature),
body = resolveExpression(body)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ case object TypeFunctions extends IRPass {

name.name match {
case Type.Ascription.name =>
Type.Ascription(leftArg, rightArg, location)
Type.Ascription(leftArg, rightArg, None, location)
case Type.Context.name =>
Type.Context(leftArg, rightArg, location)
case Type.Error.name =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,8 @@ case object TypeNames extends IRPass {
new MetadataPair(
TypeSignatures,
TypeSignatures.Signature(
resolveSignature(typeParams, bindingsMap, s.signature)
resolveSignature(typeParams, bindingsMap, s.signature),
s.comment
)
)
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ case object TypeSignatures extends IRPass {
case _ =>
}
val res = lastSignature match {
case Some(asc @ Type.Ascription(typed, sig, _, _, _)) =>
case Some(asc @ Type.Ascription(typed, sig, comment, _, _, _)) =>
val methodRef = meth.methodReference
val newMethodWithDoc = asc
.getMetadata(DocumentationComments)
Expand All @@ -121,7 +121,7 @@ case object TypeSignatures extends IRPass {
if (ref isSameReferenceAs methodRef) {
Some(
newMethodWithAnnotations.updateMetadata(
new MetadataPair(this, Signature(sig))
new MetadataPair(this, Signature(sig, comment))
)
)
} else {
Expand Down Expand Up @@ -246,7 +246,9 @@ case object TypeSignatures extends IRPass {
private def resolveAscription(sig: Type.Ascription): Expression = {
val newTyped = sig.typed.mapExpressions(resolveExpression)
val newSig = sig.signature.mapExpressions(resolveExpression)
newTyped.updateMetadata(new MetadataPair(this, Signature(newSig)))
newTyped.updateMetadata(
new MetadataPair(this, Signature(newSig, sig.comment))
)
}

/** Resolves type signatures in a block.
Expand All @@ -271,7 +273,7 @@ case object TypeSignatures extends IRPass {
case binding: Expression.Binding =>
val newBinding = binding.mapExpressions(resolveExpression)
val res = lastSignature match {
case Some(asc @ Type.Ascription(typed, sig, _, _, _)) =>
case Some(asc @ Type.Ascription(typed, sig, comment, _, _, _)) =>
val name = binding.name
val newBindingWithDoc = asc
.getMetadata(DocumentationComments)
Expand All @@ -287,7 +289,7 @@ case object TypeSignatures extends IRPass {
if (typedName.name == name.name) {
Some(
newBindingWithDoc.updateMetadata(
new MetadataPair(this, Signature(sig))
new MetadataPair(this, Signature(sig, comment))
)
)
} else {
Expand Down Expand Up @@ -321,8 +323,10 @@ case object TypeSignatures extends IRPass {
/** A representation of a type signature.
*
* @param signature the expression for the type signature
* @param comment an optional comment from which the potential error message will be derived
*/
case class Signature(signature: Expression) extends IRPass.IRMetadata {
case class Signature(signature: Expression, comment: Option[String] = None)
extends IRPass.IRMetadata {
override val metadataName: String = "TypeSignatures.Signature"

/** @inheritdoc */
Expand All @@ -345,6 +349,6 @@ case object TypeSignatures extends IRPass {

/** @inheritdoc */
override def duplicate(): Option[IRPass.IRMetadata] =
Some(this.copy(signature = signature.duplicate()))
Some(this.copy(signature = signature.duplicate(), comment = comment))
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ Option<Expression> translateInline(Tree ast) {
methodReference = translateExpression(sig.getVariable());
}
var signature = translateType(sig.getType());
var ascription = new Type.Ascription(methodReference, signature, getIdentifiedLocation(sig), meta(), diag());
var ascription = new Type.Ascription(methodReference, signature, Option.empty(), getIdentifiedLocation(sig), meta(), diag());
yield ascription;
}
default -> translateExpression(exprTree);
Expand Down Expand Up @@ -250,7 +250,8 @@ private List<Definition> translateModuleSymbolImpl(Tree inputAst, List<Definitio
yield join(error, appendTo);
}

var ascribedBody = addTypeAscription(body, returnSignature, loc);
String functionName = fn.getName().codeRepr();
var ascribedBody = addTypeAscription(functionName, body, returnSignature, loc);
var binding = new Method.Binding(
methodRef,
args,
Expand Down Expand Up @@ -317,7 +318,7 @@ methodRef, args, def, getIdentifiedLocation(inputAst), meta(), diag()
case Tree.TypeSignature sig -> {
var methodReference = translateMethodReference(sig.getVariable(), true);
var signature = translateType(sig.getType());
var ascription = new Type.Ascription(methodReference, signature, getIdentifiedLocation(sig), meta(), diag());
var ascription = new Type.Ascription(methodReference, signature, Option.empty(), getIdentifiedLocation(sig), meta(), diag());
yield join(ascription, appendTo);
}

Expand Down Expand Up @@ -507,6 +508,7 @@ private Expression translateFunction(Tree fun, Name name, java.util.List<Argumen

var loc = getIdentifiedLocation(fun);
var body = translateExpression(treeBody);
String functionName = name.name();
if (args.isEmpty()) {
if (body instanceof Expression.Block block) {
// suspended block has a name and no arguments
Expand All @@ -524,14 +526,14 @@ private Expression translateFunction(Tree fun, Name name, java.util.List<Argumen
body = translateSyntaxError(fun, Syntax.UnexpectedExpression$.MODULE$);
}

var ascribedBody = addTypeAscription(body, returnType, loc);
var ascribedBody = addTypeAscription(functionName, body, returnType, loc);
return new Expression.Binding(name, ascribedBody, loc, meta(), diag());
} else {
if (body == null) {
return translateSyntaxError(fun, Syntax.UnexpectedDeclarationInType$.MODULE$);
}

var ascribedBody = addTypeAscription(body, returnType, loc);
var ascribedBody = addTypeAscription(functionName, body, returnType, loc);
return new Function.Binding(name, args, ascribedBody, loc, true, meta(), diag());
}
}
Expand All @@ -551,17 +553,18 @@ private Expression resolveReturnTypeSignature(Tree.Function fun) {
* <p>
* If the type is {@code null}, the body is returned unchanged.
*/
private Expression addTypeAscription(Expression body, Expression type, Option<IdentifiedLocation> loc) {
private Expression addTypeAscription(String functionName, Expression body, Expression type, Option<IdentifiedLocation> loc) {
if (type == null) {
return body;
}

return new Type.Ascription(body, type, loc, meta(), diag());
String comment = "the result of `" + functionName + "`";
return new Type.Ascription(body, type, Option.apply(comment), loc, meta(), diag());
}

private Type.Ascription translateTypeSignature(Tree sig, Tree type, Expression typeName) {
var fn = translateType(type);
return new Type.Ascription(typeName, fn, getIdentifiedLocation(sig), meta(), diag());
return new Type.Ascription(typeName, fn, Option.empty(), getIdentifiedLocation(sig), meta(), diag());
}


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -122,13 +122,15 @@ object Type {
*
* @param typed the expression being ascribed a type
* @param signature the signature being ascribed to `typed`
* @param comment a comment that may be used to add context to the type error
* @param location the source location that the node corresponds to
* @param passData the pass metadata associated with this node
* @param diagnostics compiler diagnostics for this node
*/
sealed case class Ascription(
typed: Expression,
signature: Expression,
comment: Option[String],
override val location: Option[IdentifiedLocation],
override val passData: MetadataStorage = new MetadataStorage(),
override val diagnostics: DiagnosticStorage = DiagnosticStorage()
Expand All @@ -141,6 +143,7 @@ object Type {
*
* @param typed the expression being ascribed a type
* @param signature the signature being ascribed to `typed`
* @param comment a comment that may be used to add context to the type error
* @param location the source location that the node corresponds to
* @param passData the pass metadata associated with this node
* @param diagnostics compiler diagnostics for this node
Expand All @@ -150,12 +153,14 @@ object Type {
def copy(
typed: Expression = typed,
signature: Expression = signature,
comment: Option[String] = comment,
location: Option[IdentifiedLocation] = location,
passData: MetadataStorage = passData,
diagnostics: DiagnosticStorage = diagnostics,
id: UUID @Identifier = id
): Ascription = {
val res = Ascription(typed, signature, location, passData, diagnostics)
val res =
Ascription(typed, signature, comment, location, passData, diagnostics)
res.id = id
res
}
Expand Down Expand Up @@ -205,6 +210,7 @@ object Type {
s"""Type.Ascription(
|typed = $typed,
|signature = $signature,
|comment = $comment,
|location = $location,
|passData = ${this.showPassData},
|diagnostics = $diagnostics,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
import java.util.List;
import java.util.stream.Collectors;
import java.util.stream.Stream;
import org.enso.compiler.core.ir.Name;
import org.enso.interpreter.EnsoLanguage;
import org.enso.interpreter.node.BaseNode.TailStatus;
import org.enso.interpreter.node.EnsoRootNode;
Expand Down Expand Up @@ -49,11 +48,11 @@
import org.graalvm.collections.Pair;

public abstract class ReadArgumentCheckNode extends Node {
private final String name;
private final String comment;
@CompilerDirectives.CompilationFinal private String expectedTypeMessage;

ReadArgumentCheckNode(String name) {
this.name = name;
ReadArgumentCheckNode(String comment) {
this.comment = comment;
}

/** */
Expand All @@ -62,7 +61,7 @@ public static ExpressionNode wrap(ExpressionNode original, ReadArgumentCheckNode
}

/**
* Executes check or conversion of the value.abstract
* Executes check or conversion of the value.
*
* @param frame frame requesting the conversion
* @param value the value to convert
Expand All @@ -89,12 +88,12 @@ final PanicException panicAtTheEnd(Object v) {
expectedTypeMessage = expectedTypeMessage();
}
var ctx = EnsoContext.get(this);
var msg = name == null ? "expression" : name;
var err = ctx.getBuiltins().error().makeTypeError(expectedTypeMessage, v, msg);
var msg = comment == null ? "expression" : comment;
var err = ctx.getBuiltins().error().makeTypeErrorOfComment(expectedTypeMessage, v, msg);
throw new PanicException(err, this);
}

public static ReadArgumentCheckNode allOf(Name argumentName, ReadArgumentCheckNode... checks) {
public static ReadArgumentCheckNode allOf(String argumentName, ReadArgumentCheckNode... checks) {
var list = Arrays.asList(checks);
var flatten =
list.stream()
Expand All @@ -105,27 +104,25 @@ public static ReadArgumentCheckNode allOf(Name argumentName, ReadArgumentCheckNo
return switch (arr.length) {
case 0 -> null;
case 1 -> arr[0];
default -> new AllOfNode(argumentName.name(), arr);
default -> new AllOfNode(argumentName, arr);
};
}

public static ReadArgumentCheckNode oneOf(Name argumentName, List<ReadArgumentCheckNode> checks) {
public static ReadArgumentCheckNode oneOf(String comment, List<ReadArgumentCheckNode> checks) {
var arr = toArray(checks);
return switch (arr.length) {
case 0 -> null;
case 1 -> arr[0];
default -> new OneOfNode(argumentName.name(), arr);
default -> new OneOfNode(comment, arr);
};
}

public static ReadArgumentCheckNode build(Name argumentName, Type expectedType) {
var n = argumentName == null ? null : argumentName.name();
return ReadArgumentCheckNodeFactory.TypeCheckNodeGen.create(n, expectedType);
public static ReadArgumentCheckNode build(String comment, Type expectedType) {
return ReadArgumentCheckNodeFactory.TypeCheckNodeGen.create(comment, expectedType);
}

public static ReadArgumentCheckNode meta(Name argumentName, Object metaObject) {
var n = argumentName == null ? null : argumentName.name();
return ReadArgumentCheckNodeFactory.MetaCheckNodeGen.create(n, metaObject);
public static ReadArgumentCheckNode meta(String comment, Object metaObject) {
return ReadArgumentCheckNodeFactory.MetaCheckNodeGen.create(comment, metaObject);
}

public static boolean isWrappedThunk(Function fn) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,6 @@ protected String getConstructorName() {

@Override
protected List<String> getConstructorParamNames() {
return List.of("expected", "actual", "name");
return List.of("expected", "actual", "comment");
}
}
Loading

0 comments on commit dfdb547

Please sign in to comment.