Fixed regression with variant expressions involving missing terms

[spookylukey]

I'm not sure what exact code re-organisation caused this, but it is a regression as far as I can tell.

Interestingly, I caught this when re-basing my compiler branch onto master (with the re-worked resolver implementation). In the compiler branch I added a test purely to handle a corner case in the compiler implementation - the resolver already passed that test and didn't need to be changed. With the changed resolver implementation, that test caught a new corner case. This is an illustration of how the two implementations can help each other (tests added only for one end up catching bugs in the other), and an argument in favour of keeping the two implementations in one code base, so they can share a test suite (@stasm we talked about this).

[Pike]

Yeah, this is a regression, sorry.

Looking back into the pre-pre-eval code

python-fluent/fluent.runtime/fluent/runtime/resolver.py

Lines 348 to 361 in 59292d6

	@handle.register(VariantExpression)
	def handle_variant_expression(expression, env):
	message = lookup_reference(expression.ref, env)
	if isinstance(message, FluentNone):
	return message
	# TODO What to do if message is not a VariantList?
	# Need test at least.
	assert isinstance(message.value, VariantList)
	variant_name = expression.key.name
	return select_from_variant_list(message.value,
	env,
	variant_name)

there was a explicit not-found handler, and an assert for VariantList.

In my patch, there was a period where I was heavily confused between FluentNone and FluentNoneResolver, and I probably removed that and didn't add it back.

What do you think about

if isinstance(message, FluentNoneResolver):
  return message(env)

# ....
assert isinstance(message, ...

[spookylukey]

Thanks for the check @Pike - I finally realized that we do need to handle the case of neither VariantList nor FluentNoneResolver - as per the additional test.

[spookylukey]

@Pike - did you get chance to look at this? My latest addition addresses your last suggestion I think. Cheers!

[Pike]

Sorry for the lag, the spec was on PTO. And I needed to talk to the spec about the error here.

So, a term variant reference on a term w/out variants isn't actually an error in general. At least not in the same sense that other runtime errors are. In particular, we should allow for language fallback on runtime errors, but not in this case.

A warning-level thing is in order, though, and chatting with stas, we had the idea to add reporter APIs for errors and warnings to the bundle or the context, which could then act accordingly. But that's probably way out of scope.

What do you think about adding the message to logging.info instead? It's a bit overboarding right now, given that we're remove Term Variants in Fluent 0.9, but we'll have the same scenario with parameterized terms in 0.8+, where you can do {-term(1,2,not-having:"this")}. We'll add checks for that to c-l, too, but doesn't hurt to have runtime reporting?

The idea is that applications could observe the logger, and for example fail the test suite if such a term reference is executed.

FYI, you can see the filed and fixed bugs we have on those checks on https://bugzilla.mozilla.org/showdependencytree.cgi?id=1280893&maxdepth=1&hide_resolved=0

[spookylukey]

I agree that runtime warning/error reporting is helpful, as well as pre-deployment/release checks like compare-locales - runtime reporting can give you an idea of the size of the problem for specific strings under actual usage conditions, so you can prioritize fixes that you decided to ignore at release time.

Two questions:

1. 'info' or 'warning'?

logging.info sounds like something that would never get onto anyone's radar (for example, in production apps I almost always log at warning and above).

On the other hand, I don't like to report warnings unless those warnings both can and should be fixed (otherwise people drown in warnings and ignore them all). I'm guessing in this case the FTL should be fixed, right? If a message says -term[accusative] or -term(case='accusative') but the term doesn't have any variants or concept of 'case', that sounds like a mistake.

I imagine there could be unusual cases where normally the translator would specify these variants, so for consistency always does so when using terms in messages, but certain terms are exceptions and therefore never decline according to case (for example) and therefore don't specify variants. This could be fixed by having a dummy select expression i.e. instead of

-my-term = My Term

you'd have to write this to silence the warning:

-my-term =  { $case ->
           *[all]    My Term
  }

Does that sound reasonable for exceptional cases?

2. How to report the issue?

In other words, whose job is it to use logging? I had been assuming, given that FluentBundle.format returns an error list, it doesn't also do logging, but leaves that to the calling code. (That's what I'm doing in django-ftl, currently it logs all errors produced by format). It would be more consistent to report warnings in a list somehow, like with errors, rather than log them.

I presume we don't want to gain a warnings return value as well as errors. One possible option would be to add the warnings to the errors return value, but to use a different exception base class (e.g. FluentWarning) so that warnings can be easily filtered out or handled differently.

Putting error/warning reporting into FluentBundle itself significantly changes the dependencies of how messages are produced. For example, in my compiler implementation, until now the functions produced all have the signature def my_message(args, errors) (with locale available as a global, but you could add that as a parameter). If reporting of warnings went via FluentBundle, these functions would need to take a bundle parameter as well as (or instead of) errors. This wouldn't necessarily be a huge problem, but it is a significant change, could potentially limit implementation options and optimizations, and could perhaps be more problematic for other implementations if they needed to follow suit.

Maybe for now using logging.info or logging.warning would be OK, and longer term think about some better options?

[stasm]

(I was away last week and I'm catching up with the discussions here.)

Thanks for the good comment, @spookylukey. I have a few thoughts on how to handle errors/warnings for missing variables in terms. However, I'd like to prefix them by saying that this issue seems to be specifically about VariantExpressions and VariantLists, both of which will be removed by Syntax 0.9 in favor of parameterized terms. In the light of this, I think it's OK to keep this PR and the logic of reporting the error simple and strict. My comments below are about parametrized terms.

On the other hand, I don't like to report warnings unless those warnings both can and should be fixed (otherwise people drown in warnings and ignore them all). I'm guessing in this case the FTL should be fixed, right? If a message says -term[accusative] or -term(case='accusative') but the term doesn't have any variants or concept of 'case', that sounds like a mistake.

There are valid scenarios where the term shouldn't be fixed. The use-case for terms is to factor out common translations out of messages so that they are consistent across all of the UI. As a secondary goal, terms can be swapped in and out without having to touch other messages. For instance, the -brand-name term in Firefox can be supplied from some place else, effectively changing the branding of the whole app. This can be very powerful for launching new products on a shared foundation of translations, or even starting new forks. Depending on the language, the other values of the term might not need the same logic, and thus, should be able to safely ignore parameters passed into the reference to the term.

This could be fixed by having a dummy select expression i.e. instead of

-my-term = My Term

you'd have to write this to silence the warning:

-my-term =  { $case ->
          *[all]    My Term
 }

Yes, that could be a work-around. Or even the following if there are more variables to ignore:

-my-term =  { "always" ->
     [ignore] {$case} {$capitalization} {$some-other-variable}
    *[always]    My Term
}

However, I think this is against the simple things should be simple principle. At a higher level it would also hurt the localization. The alternative terms (in forks of the app, for example) would need to keep track of all possible variables names in the upstream term, and make sure they implement the upstream term's interface, so to speak.

In other words, whose job is it to use logging? I had been assuming, given that FluentBundle.format returns an error list, it doesn't also do logging, but leaves that to the calling code.

Good point. Again, this highlights the lack of the reference resolver. Thanks for having these conversations with us :) @Pike suggested to me yesterday that perhaps what we really need is a reference test suite for the resolver rather than the actual implementation. I think both could be useful, but I like the idea of focusing on the test suite. It would specify which errors and warnings should be reported, but doesn't have to specify how they are reported. This could be left for the implementation to decide, matching the idioms of the programming language. In functional languages, the errors can be returned in a Result type, in OO languages they can be logged into a method, in C they could set the value of errno etc. I'll be happy deferring the decision about what's best for Python to you and @Pike.

I presume we don't want to gain a warnings return value as well as errors. One possible option would be to add the warnings to the errors return value, but to use a different exception base class (e.g. FluentWarning) so that warnings can be easily filtered out or handled differently.

I like this solution a lot, especially given that we already have the code which collects and returns the errors array in place. In the future, if you decide that another pattern of reporting errors would be more suitable for python-fluent, we can change it for both errors and warnings.

[spookylukey]

@Pike - are you happy with the idea of adding a FluentWarning instance to the errors list a solution for now?

[Pike]

Yeah regarding FluentWarning. I'm not sure what the base class of FluentWarning should be. RuntimeWarning sounds like it, but the way they're used in warnings doesn't ring a bell right away. In particular if warnings.warn does the right thing, if we'd decide to call it.

And sorry for the continuous radio silence here, I paged python.runtime out of my brain for a while to focus on other projects here at work.

[spookylukey]

I agree RuntimeWarning should be a good base class (it inherits from Exception ultimately). I think we can just ignore the warnings mechanism from our code - we can just use it as a base class, allowing other code to filter it easily.

No worries about the silence. I have no pressing need for fast progress with fluent.runtime - I'm now happily using my branches in production, that's fine for now.