Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

invoke ignores errors #1790

Open
ToonTalk opened this issue Jul 5, 2017 · 22 comments
Open

invoke ignores errors #1790

ToonTalk opened this issue Jul 5, 2017 · 22 comments

Comments

@ToonTalk
Copy link

ToonTalk commented Jul 5, 2017

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
Copy link
Owner

jmoenig commented Jul 6, 2017

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
Copy link
Author

ToonTalk commented Jul 7, 2017

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
Copy link
Contributor

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
Copy link
Author

ToonTalk commented Jul 8, 2017

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

@cycomachead
Copy link
Collaborator

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
Copy link
Owner

jmoenig commented Jul 8, 2017

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:

b6159d7

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
Copy link
Collaborator

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
Copy link
Collaborator

@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
Copy link
Collaborator

cycomachead commented Jul 8, 2017 via email

@cycomachead
Copy link
Collaborator

#1744

@brianharvey
Copy link
Collaborator

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
Copy link
Owner

jmoenig commented Jul 8, 2017

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

brianharvey commented Jul 8, 2017

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
Copy link
Collaborator

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
Copy link
Collaborator

Ooh! Ooh!
google chrome001
runs forever. (Same with REST FOR ( ) BEATS.)

@cycomachead
Copy link
Collaborator

(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
Copy link
Collaborator

Ooh! Ooh! ... runs forever.

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

@brianharvey
Copy link
Collaborator

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
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants