Potentially misleading error message for incomplete generic return types

jeremiah-corrado commented 2 months ago

Summary

The following proc has an array return type with an unspecified domain. Throwing causes an "illegal use of function that does not return a value" error, originating from the last line of this program:

proc throwsError(): [] int throws {
  throw new Error("asdf");
}

var x = throwsError();

Specifying a domain value in the return type allows the program to compile:

proc throwsError(d: domain(?)): [d] int throws {
  throw new Error("asdf");
}

var x = throwsError({1..10});

EDIT: an improved error message like: "unable to determine concrete return type for the routine: 'throwsError'" or "unable to determine type of 'x' because procedure 'throwsError' doesn't have a concrete return type" would make it easier for users to understand why the first program fails to compile.

Example

The following is a more realistic example of how this issue could come up:


proc transpose(a: [?d] ?t): [] t throws
    where d.rank >= 2
{
    var dims = d.dims();
    dims[d.rank - 2] <=> dims[d.rank - 1];
    var ret: [{(...dims)}] t;

    forall idx in d {
        var rIdx = idx;
        rIdx[d.rank-1] <=> rIdx[d.rank-2];
        ret[rIdx] = a[idx];
    }

    return ret;
}

proc transpose(a: [?d] ?t): [] t throws
    where d.rank < 2
{
    throw new Error("rank must be at least 2 for transpose");
}

var x: [1..10, 1..5] int;
var y: [1..10] int;

// should work
var xx = transpose(x);

// should throw
try {
    var yy = transpose(y);
} catch e {
    writeln(e);
}

Here, the compiler emits an "illegal use of function that does not return a value" error from the var yy = transpose(y); line.

Discussion

The root cause of this issue is likely related to the compiler being unable to create a fully instantiated return type with a runtime-valued domain. Because the return type is missing, the compiler proceeds as if the function does not return a value when resolving expressions like var yy = transpose(y);.

bradcray commented 2 months ago

@jeremiah-corrado : The error for the original code doesn't seem completely unreasonable to me since the routine is declared as returning something, being used as though it returns something, yet doesn't return anything. I believe that since the compiler is trying to determine the type of x, it's finding that it's unable to do so since it can't determine the return type of the routine, and imagine the same behavior might occur with a routine returning a generic. This example seems to validate that guess:

proc foo(): integral throws {
  throw new Error("asdf");
}

var x = foo();

Making the original routine return something also seems to fix the issue:

config const coinflip = true;

proc throwsError(): [] int throws {
  if coinflip then
    throw new Error("asdf");
  else
    return [1, 2, 3];
}

var x = throwsError();
writeln(x);

and I imagine you could use a similar trick to make your rank-1 overload of transpose() work (?)—or just declare it over a bogus domain. That said, since this information is statically known, a better approach would be to change the throw into a compilerError() as shown here.

jeremiah-corrado commented 2 months ago

The error for the original code doesn't seem completely unreasonable to me since the routine is declared as returning something, being used as though it returns something, yet doesn't return anything

I think a more helpful error message would be something like: "unable to determine concrete return type for the routine: 'throwsError'".

I say this because the current error doesn't arise when the proc's return type is generic but fully instantiated:

proc throwsError(type t): t throws {
  throw new Error("asdf");
}

var x = throwsError(int);

This function is still being used as though it returns something, even though it doesn't; however, there is no need for an error because (as we were both saying above), the compiler is able to successfully determine x's type. This example might show what I mean more clearly.

Making the original routine return something also seems to fix the issue ... and I imagine you could use a similar trick to make your rank-1 overload of transpose() work (?)

When I came across this issue in the transpose case, my first instinct was to change the body of the error overload to:

throw new Error("rank must be at least 2 for transpose");
var ret: [1..1] t;
return ret;

This didn't resolve the issue though (potentially because the compiler stops resolving the function body after it see's the throws, so it never ends up inferring the proc's return type?). The same kind of thing happens if you change coinflip to a config param in your second example above.

Overall, after thinking about this some more, I don't disagree that the code examples I showed above should be illegal; however, I think a more targeted error message would have helped me resolve the issue more quickly. (solution was to include the queried domain d in the return type [d] t on the second overload, but not the first.)

That said, since this information is statically known, a better approach would be to change the throw into a compilerError()

I completely agree. Unfortunately this wasn't an option in the particular Arkouda context where I was working on this (due to a limitation with the way that function registration is set up at the moment).

bradcray commented 2 months ago

I think a more helpful error message would be something like: "unable to determine concrete return type for the routine: 'throwsError'".

I agree. Or even better might be to connect it to the declaration of x as being the problem, like "unable to determine type of x because procedurethrowsError() doesn't have a concrete return type."

the current error doesn't arise when the proc's return type is generic but fully instantiated

Good point, I was being sloppy and should have called the return type incomplete rather than generic.\

potentially because the compiler stops resolving the function body after it see's the throws, so it never ends up inferring the proc's return type?

Yeah, I think that's correct.

chapel-lang / chapel