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

Open
Blei opened this Issue Dec 15, 2012 · 3 comments

Comments

Projects
None yet
2 participants
@Blei
Contributor

Blei commented Dec 15, 2012

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

This comment has been minimized.

Show comment
Hide comment
@Blei

Blei Dec 15, 2012

Contributor

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.

Contributor

Blei commented Dec 15, 2012

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

This comment has been minimized.

Show comment
Hide comment
@galchinsky

galchinsky Dec 15, 2012

Contributor

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)

Contributor

galchinsky commented Dec 15, 2012

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

This comment has been minimized.

Show comment
Hide comment
@Blei

Blei Dec 15, 2012

Contributor

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.

Contributor

Blei commented Dec 15, 2012

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment