Bad srcloc for some combinations built-in table functions

[blerner]

t = table: x, y
  row: 2, 4
  row: 3, 5
end

fun num-is-even(n, x): num-modulo(n, 2) == 0 end

t.build-column("oops", num-is-even)

yields the absurd-looking output

[jswrenn]

There's some weird stuff going on here.

I think this is the result of the implementation not enforcing its contracts correctly. The above nonsense is actually one of the best-case failures of this method. The implementation of build-column doesn't properly enforce any type contracts on its parameters. For most values, this causes internal errors (e.g., try t.build-column("oops", "uhoh")). In the code you posted, the argument num-is-even is close enough to what is expected that it makes it a ways into the implementation before blowing up.

OTOH, [list: 1, 2, 3].map({(n, m): n + m}) produces a completely reasonable error, and the implementation of raw_list_map also uses checkArgsInternal2:

pyret-lang/src/js/base/runtime.js

Lines 4363 to 4364 in ba9b471

	thisRuntime.checkArgsInternal2("Lists", "raw-list-map",
	f, thisRuntime.Function, lst, thisRuntime.List);

[blerner]

You missed the part where "1 argument" corresponds to two highlighted expressions... That's not good, either.

[jswrenn]

I saw that. Take a look at how the error for [list: 1, 2, 3].map({(n, m): n + m}) is rendered. Since the application site for the argument occurs in javascript function raw_list_map, it's not highlighted (or even mentioned in the error message).

An error like that is what I would expect of this build-column call.

When this error is thrown in the context of map, the application site is detected correctly as occurring inside the implementation of map, and determined to be something that cannot be shown to the user, and is thus elided from the error message.

However, when this error is thrown in the context of build-column, the application site is not detected as occurring inside the implementation of build-column but rather as being another stack frame up (which is the stack frame for the application of build-column) which does appear in using code, and thus should be mentioned in the error message.

Because the wrong application site is detected, the highlights get wonky because it tries to graft the error for an arity mismatch on num-is-even onto the syntax of an arity mismatch for build-column.

I can think of three points of failure.

First, perhaps maybe-stack-loc is being called with the wrong arguments:

pyret-lang/src/arr/trove/error.arr

Lines 1746 to 1747 in ba9b471

	application-loc =
	maybe-stack-loc(if self.fun-def-loc.is-builtin(): 0 else: 1 end, false)

Second, there might be a bug in maybeStackLoc:
https://github.com/brownplt/code.pyret.org/blob/805789145ee09670a50ef034b8b25f10f5f5071c/src/web/js/output-ui.js#L562-L574

Third, the stack might be in a weird state.

[blerner]

All that is useful info, but I'm also emphasizing the grammar error between plural 1 vs 2 things. The message itself needs editing, independent of the locations being strange.

[jswrenn]

I don't see anything in the text of the message that has a plurality issue. The n in "n argument(s) were passed" part of the message comes from the error struct itself:

pyret-lang/src/arr/trove/error.arr

Lines 1688 to 1690 in ba9b471

	| arity-mismatch(fun-def-loc, fun-def-arity, fun-app-args, is-method) with:
	method render-fancy-reason(self, maybe-stack-loc, src-available, maybe-ast) block:
	fun-app-arity = self.fun-app-args.length()

The highlights come from inspecting the application site. These two sources of information are grafted together by this function:

pyret-lang/src/arr/trove/error.arr

Lines 1816 to 1822 in ba9b471

	arguments = destructured-application
	.and-then(
	lam(v):
	{app-loc; op-loc; args} = v
	ED.highlight(_, args, 3)
	end)
	.or-else({(v): v })

Rather than calculate n by self.fun-app-args.length(), I suppose we could compute it by looking at how many arguments are observed when the application site is parsed, and fall back on the definitely-known length of self.fun-app-args.length() when the application site isn't parsable.

But:

1. I don't know what the advantages of this would be.
2. I don't know what the edge-cases of this would be.

Doing so would make this horribly wrong error message less obviously wrong, but I don't know that doing so would be a good thing.