projectfluent / fluent-rs

Rust implementation of Project Fluent
https://projectfluent.org
Apache License 2.0
1.04k stars 95 forks source link

'internal error: entered unreachable code' when calling FluentBundle::format_pattern() #242

Open Frodo45127 opened 2 years ago

Frodo45127 commented 2 years ago

Fluent version: 0.16 FluentBundle version: 0.15.2 OS: Windows Rust version: 1.56/1.56.1

When calling FluentBundle::format_pattern() with a specific pattern, the lib triggers an "unreachable code" panic on fluent-bundle-0.15.2\src\resolver\expression.rs:63:36 and crashes. Managed to trigger it on windows, haven't tested it on other platforms.

Somewhat minimal reproducible example: fluent_test.zip

zbraniecki commented 2 years ago

Can you provide a single message (or a couple if they reference each other) that reproduces it? It should be easy to isolate it from any resource.

It should be possible for you to paste a minimal FTL resource and a format call (id + args) that trigger that.

As per the macro - it should be unreachable so we're definitely talking about a bug in our logic! Thank you for reporting it :)

im-mortal commented 2 years ago

The isolated minimal example ru.ftl is within the archive uploaded by Frodo45127. The offending single message is optimize_packfile_are_you_sure, and any variables it references are defined within the same file.

zbraniecki commented 2 years ago

Ah, I see. I was able to reproduce it, thank you!

The problem is two fold:

1) We do have an issue of knowing how to handle when an error is in a select expression. This can be fixed with:

--- a/fluent-bundle/src/resolver/expression.rs
+++ b/fluent-bundle/src/resolver/expression.rs
@@ -60,7 +60,7 @@ impl<'p> WriteValue for ast::Expression<&'p str> {
     {
         match self {
             Self::Inline(exp) => exp.write_error(w),
-            Self::Select { .. } => unreachable!(),
+            Self::Select { selector, .. } => selector.write_error(w),
         }
     }
 }

but I'm a bit uncertain how do we want to fallback if the error happens in selector expression. We never encountered that.

The reason we never encountered that is that theoretically selector expression should be infallible - all broken states should be caught by the parser and the AST should only be able to fail in inline... except here.

The reason we want to fail here at all is because we reached a limit of placeable, and we reached it exactly in a select expression.

If you apply fix (1), and print errors you'll see:

[
    ResolverError(
        TooManyPlaceables,
    ),
]

We can bump this number to const MAX_PLACEABLES: u8 = 200; and together those two solve the problem (you have other errors but they're not related).

So, the question is:

1) How should we fallback print error in select expression. The way I did it was to print the selector, which I'm not sure if is the correct way:

message = Hello { $foo ->
  [one] Foo
 *[bar] Bar
}

results in:

Hello { $foo }

2) Do we want to bump the max placeables? I'd like to do it across implementations (JS, Python, Rust). Maybe we should make it configurable?

@eemeli @stasm @Pike - thoughts?

eemeli commented 2 years ago

Reading through the original discussion adding this (projectfluent/fluent.js#439), the 100 value appears pretty much arbitrary, so bumping it up to a larger number would be just as valid, but also just as likely to repeat this error later on.

So having hit this issue once, making the value configurable sounds like the right move, so a user can fix the situation if they encounter it. Presumably this would be an option on FluentBundle?

gregtatum commented 1 year ago

Marking as good first bug for the first part of the bug:

We do have an issue of knowing how to handle when an error is in a select expression. This can be fixed with:

Making the MAX_PLACEABLES is not a good first bug, and is a second piece of work that is larger in scope.