jmoenig / Snap

a visual programming language inspired by Scratch
http://snap.berkeley.edu
GNU Affero General Public License v3.0
1.5k stars 744 forks source link

invoke ignores errors #1790

Open ToonTalk opened 7 years ago

ToonTalk commented 7 years ago

I've been happily using 'invoke' since learning about it in #1402

I tried invoking 'think 1/0' and no error appeared or could be caught by a try statement. Personally I'd be happy with the usual talk balloon error reporting connected to the block culprit.

I see that invoke has a 'suppressErrors' argument but since that is false when not provided that isn't the explanation of the lack of error reporting or handling.

jmoenig commented 7 years ago

zero division doesn't cause an error in JavaScript but instead returns Infinity, so that wouldn't cause and error. You could try invoking a list operation on a non-list, that should throw an actual error.

ToonTalk commented 7 years ago

When I tried item 1 of a variable bound to 0 I get this confusing error message:

image

I used the example of 1/0 thinking that would be a simpler example than what I encountered. In my case I was using a JavaScript block. Just now testing the simplest example I see a strange error message:

image

And when the JavaScript block is invoked by another JavaScript block there is no error (on the screen or the console).

towerofnix commented 7 years ago

When I tried item 1 of a variable bound to 0 I get this confusing error message:

"list.at is not a function"

It's confusing, but makes sense if you know what's going on.. the "item of" block's JS code is (with some unrelated stuff removed) this:

Process.prototype.reportListItem = function (index, list) {
    return list.at(index);
};

So, when you passed it 0, it tried to run (0).at(index).. and, of course the number 0 doesn't have a .at method! So it threw "list.at is not a function". (That is, doing list.at gets JavaScript the value undefined, and undefined is not a function.)

ToonTalk commented 7 years ago

Right. But one of the innovations of Logo 50 years ago was to very carefully design appropriate error messages.

cycomachead commented 7 years ago

My personal theory, is that error messages make or break the feelings of success or frustration or progress when programming. This goes for my day job (programming) and teaching. A thoughtful message is educational, constructive and even delightful in some cases.

There are a few environments I use that give virtually no error messages. I hate doing that work. I'm pretty sure students feel the same way about their courses.

jmoenig commented 7 years ago

Sigh.

Okay, first the good news: I've just activated type assertions for inputs to all list primitives. Now you get actually meaningful error messages:

https://github.com/jmoenig/Snap--Build-Your-Own-Blocks/commit/b6159d7b19eee9a984f5e7e7dd7dceaf266f47a2

This will most likely be in the next release (4.1) coming later this summer.

I've had type assertions in the code for a long time, but commented out most of them for performance reasons. Always type-asserting inputs all the time does put somewhat of a strain to performance, especially in computation-heavy projects. I get that teachers have a hard time explaining JavaScript error messages (which Snap! does catch, btw, otherwise there wouldn't be any red borders around scripts) to students, hence I changed my mind in this particular case.

brianharvey commented 7 years ago

Jens, you shouldn't have to slow down performance to solve this problem. Don't do the type checking until you catch an error. (This is the same insight that will let us do hybrid scope! And do a good job on all the other error messages, too.)

brianharvey commented 7 years ago

@cycomachead I agree with your comment above, about 98%. The other 2% is that I have seen high school students, including very good programmers, get an error message and react by deleting their entire program and starting over. That's what gives some designers the impulse to avoid error messages altogether. But Ken is right that the solution to this problem is to have really good error messages that make it clear how to fix the problem.

cycomachead commented 7 years ago

Yeah, but I’ve seen students throw code away because there are no error messages and it doesn’t work. I mean I’ve done that too.

And re detecting errors after they happen: it’s not trivial because we need to encode type info into slots and blocks but it’s totally doable. There’s a PR that I have called “generic error messages” that does this.

This is why I want to discuss error handling in Bordeaux! It’s critically important, IMO, that we do have good error messages.

-- Michael Ball From my iPhone michaelball.co

On Jul 8, 2017, at 12:08 PM, Brian Harvey notifications@github.com wrote:

@cycomachead I agree with your comment above, about 98%. The other 2% is that I have seen high school students, including very good programmers, get an error message and react by deleting their entire program and starting over. That's what gives some designers the impulse to avoid error messages altogether. But Ken is right that the solution to this problem is to have really good error messages that make it clear how to fix the problem.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

cycomachead commented 7 years ago

1744

brianharvey commented 7 years ago

Yeah, I saw your PR about slot type info, but in my naiveté I don't understand why the error handler can't just say if message = "list.at is not a function" then say "ITEM of non-list." If you can the retrieve the actual second input, even better.

And while we're at it, ITEM of an empty list should throw an error, not just return the empty string. And the message should say "You probably don't have a base case in your recursion."

jmoenig commented 7 years ago

Brian, how often do I have to remind you that JS isn't Scheme or Smalltalk? :-)

JS doesn't have self-reflection, you can't access the call-stack. Neither before nor after you catch an error.

brianharvey commented 7 years ago

I am only an egg, but what does the JS call stack have to do with anything? It's the Snap! stack that matters, and you clearly have access to that, to know which block to light up red.

cycomachead commented 7 years ago

Sure, we can hard code a list of error messages, but that doesn't seem super useful. These small JS errors happen everywhere. We do have access to the block inputs, but not necessarily what they should be - or, at least, it's encoded in a useful way. (that's why I set the type for each input type.)

cycomachead commented 7 years ago

Also, we can't really hard code a list of JS error messages, when I think about it. The error messages are dependent on the browser implementation, and in the case of Snap! we want to know the block that ended up calling the message, not necessarily the exact JS function. But again, that's essentially what my PR does.

And while we're at it, ITEM of an empty list should throw an error, not just return the empty string. And the message should say "You probably don't have a base case in your recursion."

1) We aren't in a position to make "probably" statements until we actually analyze the code executed, 2) This is a Scratch compatibility thing. And it's possible that someone is relying on that behavior. It's a hacky but effective check if you are removing all items of a list.

brianharvey commented 7 years ago

Oh, okay. I was trying to find a way to meet Jens's goal of not putting any time into errors until they happen. But once an error does happen, I think, it's fine to spend all the time we need to do the analysis of the code leading to the error.

(Question: Does catching errors in JS take time if the error doesn't happen?)

About item of empty list, ugh. I'm not sure I agree with #1; that's why I said "probably." Once we analyze the user's code we can just say "you left out the base case" without hedging. As for Scratch compatibility, it seems clear to me from other threads that we're getting close to having to have a Scratch compatibility mode for old projects. And here's a case where we're now (I bet) running extra code in ITEM to avoid an error message.

brianharvey commented 7 years ago

I tried causing a JS error that wasn't because of a bad input type. It was hard because lots of things that should cause errors instead return empty strings (e.g., HTTP:// of a bad URL) but I finally did it:

google chrome001

The trick is to drag a MOVE block onto the stage icon in the sprite corral, then switch to the stage and click it. In fact, I had trouble causing any errors at all that didn't boil down to ITEM of a non-list. There's "expecting n inputs but getting m" in CALL and friends, but that's not a JS error.

brianharvey commented 7 years ago

Ooh! Ooh!

google chrome001

runs forever. (Same with REST FOR ( ) BEATS.)

cycomachead commented 7 years ago

(Question: Does catching errors in JS take time if the error doesn't happen?)

Yes, it does. But we have to catch them anyway, so unfortunately it doesn't matter much - if we don't catch the errors, then JS will just stop executing.

But once an error does happen, I think, it's fine to spend all the time we need to do the analysis of the code leading to the error.

Sure, that's true. But we don't do any of that now, and in the case of recursion, it's not always trivial. - Though I think we can do a pretty good job for 90% of the cases, so if we're interested in really doing stuff like this, I do think it's worthwhile.

About item of empty list, ugh. I'm not sure I agree with #1; that's why I said "probably."

We still have to be careful with this though, there's plenty of room for an error like that in cases where users have never heard of recursion.

I tried causing a JS error that wasn't because of a bad input type.

Yeah, that's why a generic error handling approach is useful. I think my PR does pretty much what you're suggesting. We just can't do it be looking for specific messages / functions. When any error occurs, this will go through the block's input spec and compare the types in that spec to the types that were provided.

Anyway, yes, the vast majority of errors shown are from list functions, but it's all of the list primitives, not just item of. I also tried to make a couple improvements to non-type mismatch JS errors, though it's harder to do all of those in a generic way. One thing I did was number the input where an error occurred - which it's not always clear what's what when students are passing all variables into a function.

Otherwise, the additional advantage I see of having a more generic error format, is that it could allow us to have the option for more errors to be thrown. (You can also imagine that you'd want different error reporting semantics when you're using Snap! Full screen compared to when you're editing a project.) That's all stuff for the future, but I see having a better error reporting mechanism as a prerequisite to displaying more errors.

cycomachead commented 7 years ago

Ooh! Ooh! ... runs forever.

I think that should be a separate issue, but any truthy value, and NaN, seems have this wait forever.

brianharvey commented 7 years ago

Yeah, that's why a generic error handling approach is useful. I think my PR does pretty much what you're suggesting.

No, because in my example there's no type mismatch. 10 is a number.

cycomachead commented 7 years ago

Ok, sure, but the intent is to just use the original error message if we can't determine something more sensible.

We want is something like "Can't use MOVE on the Stage" or "No move block exists for the stage" - which is it's own case to think about with OOP, I'd imagine. There's plenty of room for similar errors, and already the same is possible with sprite local blocks.