@fluent/syntax: Variant values with [] don't roundtrip through the parser/serializer

[Pike]

The following Fluent snippet doesn't round-trip right:

shared-photos =
    { $userName } { $photoCount ->
        [one][x]added a new photo { $item ->
                [bucket] to the bucket
               *[folder] to the folder
            }
       *[other] added { $photoCount } new photos
    }

The result is

shared-photos =
    { $userName } { $photoCount ->
        [one]
            [x]added a new photo { $item ->
                [bucket] to the bucket
               *[folder] to the folder
            }
       *[other] added { $photoCount } new photos
    }

Filing here, but it affects all of js, python, and kotlin.

[Pike]

This code here

fluent.js/fluent-syntax/src/serializer.ts

Lines 146 to 148 in f41d910

	const startOnNewLine =
	pattern.elements.some(isSelectExpr) ||
	pattern.elements.some(includesNewLine);

doesn't take into account that block_text starts with indented_char,

https://github.com/projectfluent/fluent/blob/7282b865958ea0602ddc6b3d583ab7477abb49e7/spec/fluent.ebnf#L110

aka, - "[" - "*" - ".".

Easy to fix. @stasm, I'm not sure where to test this, though. Should we go as far as have tests in the spec to ensure those are all parse errors?
And then, yes, serializer tests, too.

[Michael-F-Bryan]

As far as I can tell, the issue is that [one][x] is serialized to [one]\n[x] (i.e the literal [x] starts on a newline, maybe with some indents)... Is it guaranteed that a serializer should preserve all whitespace? I thought the whitespace before a variant's first pattern element wasn't significant.

When I was implementing projectfluent/fluent-rs#184 I noticed that test inputs were constructed so things round-trip exactly, but just thought that was to make testing easier and not because it's required by the spec.

EDIT: Never mind. I updated my code to re-parse the serialized text and make sure it's identical to the original, and we run into a parse error. Seems like putting the variant's text element on a new line isn't valid syntax (or at least, as implemented by the Rust parser).

[Pike]

Usually white-space around syntax elements is not significant, but in this case, newlines are. That's in the grammar snippet I linked to.

Generally speaking, handling white-space with proper round-tripping is a pro for tooling applications. It's easier that way to not have different tools battle over how to serialize strings. There are tricks to lessen the impact (compare ASTs when deciding to write messages, for example), but generally, full round-trip is good for tooling.

Not that I know of an implementations that does that.