jckarter / clay

The Clay programming language
http://claylabs.com/clay
Other
403 stars 34 forks source link

Very minimal error reporting on missing return type in external functions #460

Open Blei opened 11 years ago

Blei commented 11 years ago

The following program:

private external foo() {
    return 1;
}

main() {
    foo();
}

doesn't compile, which is expected. The displayed error message is maybe a bit too minimal though:

error: expected 0 values, but received 1 value

No backtrace, no mention of the problem being a missing return type declaration. It feels like this might actually be a compiler error and not a user facing error.

Blei commented 11 years ago

Something similar happens for non-external procedures (it displays an incorrect location: the call site instead of the return statement), which made me suspect that there was a missing pushLocation() call somewhere. And indeed, with this hacky little patch the error reporting is improved greatly.

diff --git a/compiler/codegen.cpp b/compiler/codegen.cpp
index 666c181..756cc46 100644
--- a/compiler/codegen.cpp
+++ b/compiler/codegen.cpp
@@ -3909,7 +3909,9 @@ bool codegenStatement(StatementPtr stmt,
         MultiPValuePtr mpv = safeAnalyzeMulti(x->values, env, 1);
         MultiCValuePtr mcv = new MultiCValue();
         llvm::ArrayRef<CReturn> returns = ctx->returnLists.back();
+        pushLocation(x->location);
         ensureArity(mpv, returns.size());
+        popLocation();
         for (unsigned i = 0; i < mpv->size(); ++i) {
             PVData const &pv = mpv->values[i];
             bool byRef = returnKindToByRef(x->returnKind, pv);

I'm sure there's a more correct location for this call though, so I won't submit this as a pull request.

galchinsky commented 11 years ago

the call site instead of the return statement

Then if you write foo(println("x"));, you'll get to library source and the compiler will tell you that "println misses return value". Weird. At least there should be 2 places (caller and called)

Blei commented 11 years ago

No, it won't. The error that is reported happens at the return statement, so it should show the return statement as the error location. Your example still works as before.