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

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

Open
Frodo45127 opened this issue Nov 12, 2021 · 5 comments
Labels
bug good first issue Want to help? Those are great bugs to start with! help wanted We need help making decisions or writing PRs for this.

Comments

@Frodo45127
Copy link

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

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

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

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 }
  1. 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
Copy link
Member

eemeli commented Nov 17, 2021

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 gregtatum added bug help wanted We need help making decisions or writing PRs for this. good first issue Want to help? Those are great bugs to start with! labels Nov 7, 2022
@gregtatum
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug good first issue Want to help? Those are great bugs to start with! help wanted We need help making decisions or writing PRs for this.
Projects
None yet
Development

No branches or pull requests

5 participants