Report errors from Functions and Intl objects #398

stasm · 2019-07-19T13:48:49Z

I had a look at our error reporting and I realized that there really isn't anything that would make it hard to report errors thrown by Functions and from memoizeIntlObject. Yay.

Fix #106. Fix #107.

stasm

It's a simple change in code, and a bigger one in tests. I wanted to be thorough with exactly what errors are being collected.

stasm · 2019-07-19T13:52:14Z

fluent/src/builtins.js

  if (arg instanceof FluentNumber) {
    return new FluentNumber(arg.valueOf(), merge(arg.opts, opts));
  }
-  return new FluentNone("NUMBER()");


This was introduced in April in 24355cf to provide better fallback values. Today we can simply throw :)

Pike

Nice, but I've got one issue here, and that's the double reported errors.

I think we should ensure that a single error has a single message. We do have solid use-cases for multiple errors in one message, and by generating different messages for one problem, we're making people hunt dragons, and they don't need to.

I wonder if there's any non-error scenario in which an argument can be FluentNone? Maybe we can catch that early before calling into the functions, and deduplicate the work of handling it?

But I think we should handle that instead of generating a second false error message.

Pike · 2019-07-19T14:42:33Z

fluent/src/builtins.js

@@ -27,22 +27,18 @@ function values(opts) {

 export
 function NUMBER([arg], opts) {
-  if (arg instanceof FluentNone) {
-    return arg;
-  }


I think we should keep this ...

Pike · 2019-07-19T14:42:46Z

fluent/src/builtins.js

 }

 export
 function DATETIME([arg], opts) {
-  if (arg instanceof FluentNone) {
-    return arg;
-  }


... and this ...

Pike · 2019-07-19T14:43:11Z

fluent/test/functions_builtin_test.js

+      assert.ok(errors[0] instanceof ReferenceError);
+      assert.strictEqual(errors[0].message, "Unknown variable: $arg");
+      assert.ok(errors[1] instanceof TypeError);
+      assert.strictEqual(errors[1].message, "Invalid argument type to NUMBER");


... to avoid double reporting a single error.

stasm · 2019-07-19T15:18:04Z

Thanks, @Pike.

I think we should ensure that a single error has a single message.

I think I'd like to disagree. It's a slippery slope. There might be scenarios where it does make sense to have more than one message per error. We don't know yet, and I wouldn't make exclude it from the design. I wouldn't make it an assumption of the general design.

In my mind, reporting these errors is a feature. They describe what happened. There was a missing variable reference, and then you tried to pass it to a function.

It's an interesting bit of design because this technically isn't the resolver. It's how we implement the built-in functions, or any function for that matter. And I'm not sure if I like the idea of having to check every argument to a function in case it's FluentNone. Conceptually, throwing is a simpler, more atomic approach. At least in JS.

Would you be willing to revisit this when we discuss the spec? We might want to consider how this impacts other implementations, too. Also, let's keep in mind that we're talking about an edge-case of an edge-case :)

Pike · 2019-07-22T08:02:13Z

In a way, you're making an argument for stack traces.

I think we created quite a problem if a stack trace is something a downstream consumer like a Firefox dev or a localizer need to figure out how to fix their Fluent string.

I also think that mixing errors and stack traces is making things harder to understand. Every human consumer of Fluent errors needs to understand the fluent.js code structure to be able to tell apart errors and their stacks.

stasm · 2019-07-22T10:28:43Z

Yes, in a way they're stack traces, but flat. Neither solution is satisfying to me, really. One produces too many errors in some (very rare) cases; the other requires boilerplate in function implementations, and might not even hold for custom functions, depending on their implementations.

If I wanted to implement a custom function SUM(one, two), how should the code look like? Would I need to check each arg for FluentNone and, if true, return one of them or a new FluentNone? Then check if both args are FluentNumbers and compute the sum. And then throw for all other cases?

In the future, we might add the possibility of using variables as keyword arguments, too (projectfluent/fluent#230), which would mean that function need to check them against FluentNone as well.

What bothers me about this approach is that it's entirely up to the function implementation to adhere to it. Or not. I'd rather find a bullet-proof solution inside the resolver that guarantees the desired behavior. Building on top of the stack track analogy, perhaps we should in fact create a new trace in each FunctionReference? We could use a new Scope with an empty errors array for this purpose, and then figure out how to put those errors back in the original top-level errors. That's of course an idea for a follow-up.

For right now, my instinct is to go with the approach which employs less magic. By that, I mean fewer conditions and checks, and instead relying on the code that's already there. Yes, it means that in some (again, very rare) cases two errors will be produced instead of one, but crucially, the important one (about the missing variable) will still be there.

If you strongly feel that's not the right call, I'll add those ifs back.

Pike · 2019-07-22T13:55:42Z

Luke and I had a lengthy conversation around functions and errors in projectfluent/python-fluent#92.

I think we need to establish that Fluent functions are kernel code. They might be user land, but they're still kernel. We can't take away that Fluent functions need to validate their arguments. Number and date formatters are fuzzed in the implementations we refer to, so that's less of a risk. But that's an exception.

To take this out of abstract constructs, in this concrete case we're talking about:

Unknown variable: $arg
Invalid argument type to NUMBER

and I find that confusing. I can reverse engineer myself out of that, but it's still confusing to me.

I'd rather not have that problem right now.

Pike

Thanks, I like how this turned out in the end.

I filed #402 to see if we can make our error messages be more constructive in what is the right thing to do.

stasm commented Jul 19, 2019

View reviewed changes

stasm requested a review from Pike July 19, 2019 13:53

stasm added 2 commits July 19, 2019 15:55

Report errors from Functions

48a11f8

Report errors from instantiating Intl objects

c9b38d3

stasm force-pushed the report-errors-from-functions branch from a1160fc to c9b38d3 Compare July 19, 2019 13:55

Pike suggested changes Jul 19, 2019

View reviewed changes

stasm added 2 commits July 22, 2019 16:37

Check if arguments to builtins are FluentNone

055b222

Use NUMBER() as the placeable fallback

33f6cb2

stasm force-pushed the report-errors-from-functions branch from c7050a4 to 33f6cb2 Compare July 22, 2019 15:03

stasm requested a review from Pike July 22, 2019 15:06

Pike mentioned this pull request Jul 23, 2019

Review errors messages to ensure they're clear on what's right #402

Open

Pike approved these changes Jul 23, 2019

View reviewed changes

stasm merged commit c40eb5e into projectfluent:release-fluent-zero-thirteen Jul 23, 2019

stasm deleted the report-errors-from-functions branch July 23, 2019 07:38

This was referenced Jul 23, 2019

Handle errors thrown by Intl formatters #106

Closed

Handle errors thrown by Call Expressions #107

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Report errors from Functions and Intl objects #398

Report errors from Functions and Intl objects #398

stasm commented Jul 19, 2019

stasm left a comment

stasm Jul 19, 2019

Pike left a comment

Pike Jul 19, 2019

Pike Jul 19, 2019

Pike Jul 19, 2019

stasm commented Jul 19, 2019

Pike commented Jul 22, 2019

stasm commented Jul 22, 2019

Pike commented Jul 22, 2019

Pike left a comment

Report errors from Functions and Intl objects #398

Report errors from Functions and Intl objects #398

Conversation

stasm commented Jul 19, 2019

stasm left a comment

Choose a reason for hiding this comment

stasm Jul 19, 2019

Choose a reason for hiding this comment

Pike left a comment

Choose a reason for hiding this comment

Pike Jul 19, 2019

Choose a reason for hiding this comment

Pike Jul 19, 2019

Choose a reason for hiding this comment

Pike Jul 19, 2019

Choose a reason for hiding this comment

stasm commented Jul 19, 2019

Pike commented Jul 22, 2019

stasm commented Jul 22, 2019

Pike commented Jul 22, 2019

Pike left a comment

Choose a reason for hiding this comment