Skip to content

Commit

Permalink
Ensure type ascriptions are correctly transformed to facilitate check…
Browse files Browse the repository at this point in the history
…ing of more complex return types (#10229)

- Fixes #9980
- Adds some tests to ensure types like `|` or `&` (in addition to `!` from the ticket) correctly work in return type check.
- Fixes a weird behaviour where we used to avoid processing type related IR transformations inside of type ascriptions.
- Adds parentheses to type representations if they are more complex: `A | B & C` is unclear as it can either mean `A | (B & C)` or `(A | B) & C` which have different meanings. If we now have an operation with such nesting, the sub expressions are wrapped in parentheses to disambiguate.
  • Loading branch information
radeusgd authored Jun 11, 2024
1 parent a2c4d94 commit c4b0ca8
Show file tree
Hide file tree
Showing 7 changed files with 224 additions and 27 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -89,15 +89,17 @@ type Encoding
Convert an Encoding to it's corresponding Java Charset, if applicable.
This method should be used in places not aware of special logic for the
Default encoding. In such places, usage of Default encoding will be forbidden.
to_java_charset self -> Charset ! Illegal_Argument =
to_java_charset : Charset ! Illegal_Argument
to_java_charset self =
self.to_java_charset_or_null.if_nothing <|
warning = Illegal_Argument.Error "The Default encoding has been used in an unexpected place (e.g. Write operation). It will be replaced with UTF-8. Please specify the desired encoding explicitly)"
Warning.attach warning (Encoding.utf_8.to_java_charset)

## PRIVATE
Convert an Encoding to it's corresponding Java Charset or null if it is the Default encoding.
This method should only be used in places where a null Charset is expected - i.e. places aware of the Default encoding.
to_java_charset_or_null self -> Charset ! Illegal_Argument = case self of
to_java_charset_or_null : Charset | Nothing ! Illegal_Argument
to_java_charset_or_null self = case self of
Encoding.Value charset_name ->
Panic.catch UnsupportedCharsetException (Charset.forName charset_name) _->
Error.throw (Illegal_Argument.Error ("Unknown Character Set: " + charset_name))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ package org.enso.compiler.pass.desugar

import org.enso.compiler.context.{InlineContext, ModuleContext}
import org.enso.compiler.core.ir.expression.{Application, Operator}
import org.enso.compiler.core.ir.{Expression, Module, Type}
import org.enso.compiler.core.ir.{Expression, Module}
import org.enso.compiler.pass.IRPass
import org.enso.compiler.pass.analyse.{
AliasAnalysis,
Expand Down Expand Up @@ -71,8 +71,6 @@ case object OperatorToFunction extends IRPass {
inlineContext: InlineContext
): Expression =
ir.transformExpressions {
case asc: Type.Ascription =>
asc.copy(typed = runExpression(asc.typed, inlineContext))
case Operator.Binary(l, op, r, loc, passData, diag) =>
Application.Prefix(
op,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,16 +106,14 @@ case object TypeFunctions extends IRPass {
* @return `expr`, with any typing functions resolved
*/
def resolveExpression(expr: Expression): Expression = {
expr.transformExpressions {
case asc: Type.Ascription => asc
case app: Application =>
val result = resolveApplication(app)
app
.getMetadata(DocumentationComments)
.map(doc =>
result.updateMetadata(new MetadataPair(DocumentationComments, doc))
)
.getOrElse(result)
expr.transformExpressions { case app: Application =>
val result = resolveApplication(app)
app
.getMetadata(DocumentationComments)
.map(doc =>
result.updateMetadata(new MetadataPair(DocumentationComments, doc))
)
.getOrElse(result)
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -897,6 +897,33 @@ public void andConversions() throws Exception {
assertFalse("false & false", compute.execute(false, false).asBoolean());
}

@Test
public void unionInArgument() throws Exception {
var uri = new URI("memory://binary.enso");
var src =
Source.newBuilder(
"enso",
"""
from Standard.Base import all
foo (arg : Integer | Text) = arg
""",
uri.getAuthority())
.uri(uri)
.buildLiteral();
var module = ctx.eval(src);

var ok1 = module.invokeMember(MethodNames.Module.EVAL_EXPRESSION, "foo 42");
assertEquals(42, ok1.asInt());
var ok2 = module.invokeMember(MethodNames.Module.EVAL_EXPRESSION, "foo 'Hi'");
assertEquals("Hi", ok2.asString());
try {
var v = module.invokeMember(MethodNames.Module.EVAL_EXPRESSION, "foo []");
fail("Expecting an error, not " + v);
} catch (PolyglotException ex) {
assertTypeError("`arg`", "Integer | Text", "Vector", ex.getMessage());
}
}

@Test
public void unresolvedReturnTypeSignature() throws Exception {
final URI uri = new URI("memory://neg.enso");
Expand Down Expand Up @@ -1209,6 +1236,127 @@ public void returnTypeCheckOptInTCO2() throws Exception {
}
}

@Test
public void returnTypeCheckErrorSignature() throws Exception {
final URI uri = new URI("memory://rts.enso");
final Source src =
Source.newBuilder(
"enso",
"""
from Standard.Base import Integer
import Standard.Base.Errors.Illegal_State.Illegal_State
foo a -> Integer ! Illegal_State =
a+a
""",
uri.getAuthority())
.uri(uri)
.buildLiteral();

var module = ctx.eval(src);
var foo = module.invokeMember(MethodNames.Module.EVAL_EXPRESSION, "foo");
assertEquals(20, foo.execute(10).asInt());
try {
var res = foo.execute(".");
fail("Expecting an exception, not: " + res);
} catch (PolyglotException e) {
assertContains("expected the result of `foo` to be Integer, but got Text", e.getMessage());
}
}

/**
* The `!` part in the type signature is currently only for documentation purposes - it is not
* checked. Other kinds of dataflow errors may be returned as well and this is not an error. In
* particular, we don't distinguish errors raised by the function from errors propagate from its
* inputs.
*/
@Test
public void returnTypeCheckErrorSignatureAllowsAllErrors() throws Exception {
final URI uri = new URI("memory://rts.enso");
final Source src =
Source.newBuilder(
"enso",
"""
from Standard.Base import Integer, Error
import Standard.Base.Errors.Illegal_Argument.Illegal_Argument
import Standard.Base.Errors.Illegal_State.Illegal_State
foo a -> Integer ! Illegal_State =
Error.throw (Illegal_Argument.Error "foo: "+a.to_text)
""",
uri.getAuthority())
.uri(uri)
.buildLiteral();

var module = ctx.eval(src);
var foo = module.invokeMember(MethodNames.Module.EVAL_EXPRESSION, "foo");
var result = foo.execute(10);
assertTrue(result.isException());
assertContains("Illegal_Argument.Error 'foo: 10'", result.toString());
}

@Test
public void returnTypeCheckSum() throws Exception {
final URI uri = new URI("memory://rts.enso");
final Source src =
Source.newBuilder(
"enso",
"""
from Standard.Base import Integer, Text
foo a -> Integer | Text =
a+a
""",
uri.getAuthority())
.uri(uri)
.buildLiteral();

var module = ctx.eval(src);
var foo = module.invokeMember(MethodNames.Module.EVAL_EXPRESSION, "foo");
assertEquals(20, foo.execute(10).asInt());
assertEquals("..", foo.execute(".").asString());

try {
Object vec = new Integer[] {1, 2, 3};
var res = foo.execute(vec);
fail("Expecting an exception, not: " + res);
} catch (PolyglotException e) {
assertContains(
"expected the result of `foo` to be Integer | Text, but got Vector", e.getMessage());
}
}

@Test
public void returnTypeCheckProduct() throws Exception {
final URI uri = new URI("memory://rts.enso");
final Source src =
Source.newBuilder(
"enso",
"""
from Standard.Base import Integer, Text
type Clazz
Value a
Clazz.from (that : Integer) = Clazz.Value that
foo a -> (Integer | Text) & Clazz =
a+a
""",
uri.getAuthority())
.uri(uri)
.buildLiteral();

var module = ctx.eval(src);
var foo = module.invokeMember(MethodNames.Module.EVAL_EXPRESSION, "foo");
assertEquals(20, foo.execute(10).asInt());

try {
var res = foo.execute(".");
fail("Expecting an exception, not: " + res);
} catch (PolyglotException e) {
assertContains(
"expected the result of `foo` to be (Integer | Text) & Clazz, but got Text",
e.getMessage());
}
}

static void assertTypeError(String expArg, String expType, String realType, String msg) {
assertEquals(
"Type error: expected " + expArg + " to be " + expType + ", but got " + realType + ".",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import org.enso.compiler.core.Implicits.AsMetadata
import org.enso.compiler.core.ir.Expression
import org.enso.compiler.core.ir.Function
import org.enso.compiler.core.ir.Module
import org.enso.compiler.core.ir.Literal
import org.enso.compiler.core.ir.expression.Application
import org.enso.compiler.core.ir.expression.errors
import org.enso.compiler.core.ir.module.scope.Definition
Expand Down Expand Up @@ -265,14 +264,24 @@ class TypeSignaturesTest extends CompilerTest {

"associate the signature with the typed expression" in {
ir shouldBe an[Application.Prefix]
ir.getMetadata(TypeSignatures) shouldBe defined
val outerSignature = ir.getMetadata(TypeSignatures)
outerSignature shouldBe defined
outerSignature.get.signature.showCode() shouldEqual "Double"
}

"work recursively" in {
val arg2Value = ir.asInstanceOf[Application.Prefix].arguments(1).value
arg2Value shouldBe an[Application.Prefix]
val snd = arg2Value.asInstanceOf[Application.Prefix]
snd.arguments(0).value shouldBe an[Literal.Number]
val arg2Value = ir.asInstanceOf[Application.Prefix].arguments(1).value
val arg2Signature = arg2Value.getMetadata(TypeSignatures)
arg2Signature shouldBe defined
arg2Signature.get.signature.showCode() shouldEqual "Int"

// But arg1 has no signature:
val arg1Signature = ir
.asInstanceOf[Application.Prefix]
.arguments(0)
.value
.getMetadata(TypeSignatures)
arg1Signature shouldBe empty
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,38 @@ public final Object handleCheckOrConversion(VirtualFrame frame, Object value) {

abstract String expectedTypeMessage();

protected final String joinTypeParts(List<String> parts, String separator) {
assert !parts.isEmpty();
if (parts.size() == 1) {
return parts.get(0);
}

var separatorWithSpace = " " + separator + " ";
var builder = new StringBuilder();
boolean isFirst = true;
for (String part : parts) {
if (isFirst) {
isFirst = false;
} else {
builder.append(separatorWithSpace);
}

// If the part contains a space, it means it is not a single type but already a more complex
// expression with a separator.
// So to ensure we don't mess up the expression layers, we need to add parentheses around it.
boolean needsParentheses = part.contains(" ");
if (needsParentheses) {
builder.append("(");
}
builder.append(part);
if (needsParentheses) {
builder.append(")");
}
}

return builder.toString();
}

final PanicException panicAtTheEnd(Object v) {
if (expectedTypeMessage == null) {
CompilerDirectives.transferToInterpreterAndInvalidate();
Expand Down Expand Up @@ -195,9 +227,11 @@ Object executeCheckOrConversion(VirtualFrame frame, Object value) {

@Override
String expectedTypeMessage() {
return Arrays.stream(checks)
.map(n -> n.expectedTypeMessage())
.collect(Collectors.joining(" & "));
var parts =
Arrays.stream(checks)
.map(ReadArgumentCheckNode::expectedTypeMessage)
.collect(Collectors.toList());
return joinTypeParts(parts, "&");
}
}

Expand Down Expand Up @@ -239,9 +273,11 @@ Object executeCheckOrConversion(VirtualFrame frame, Object value) {

@Override
String expectedTypeMessage() {
return Arrays.stream(checks)
.map(n -> n.expectedTypeMessage())
.collect(Collectors.joining(" | "));
var parts =
Arrays.stream(checks)
.map(ReadArgumentCheckNode::expectedTypeMessage)
.collect(Collectors.toList());
return joinTypeParts(parts, "|");
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -768,6 +768,12 @@ class IrToTruffle(
comment,
context.getTopScope().getBuiltins().function()
)
case typeWithError: Tpe.Error =>
// When checking a `a ! b` type, we ignore the error part as it is only used for documentation purposes and is not checked.
extractAscribedType(comment, typeWithError.typed)
case typeInContext: Tpe.Context =>
// Type contexts aren't currently really used. But we should still check the base type.
extractAscribedType(comment, typeInContext.typed)
case t => {
t.getMetadata(TypeNames) match {
case Some(
Expand Down

0 comments on commit c4b0ca8

Please sign in to comment.