Implement Syntax 0.8, part3

[stasm]

Depends on #309.

[stasm]

@zbraniecki This PR is about two simple changes but it looks complex because it depends on #309, which is also included here for now. Once #309 lands, this will be easy to review. Thanks!

[stasm]

This is now ready for the review.

[zbraniecki]

Code-wide this looks good.

I must say that reading it again made me realize that I think I struggle with why are we storing the unescaped version of the string in the AST at all, it seems to me that it should be resolvers job, and the parse/ast/serialize cycle should treat escaped string literals as such.

Have we considered storing only what we now have in raw field? If we did and decided somehow against it, maybe we should at least reverse the fields and store what's in raw as the actual value, and then what's currently in value as an implementation-specific optimization as encoded_value.
In particular, Rust parser may want to skip the encoded_value in AST to keep it allocation free.

[stasm]

Thanks for the review.

I must say that reading it again made me realize that I think I struggle with why are we storing the unescaped version of the string in the AST at all, it seems to me that it should be resolvers job, and the parse/ast/serialize cycle should treat escaped string literals as such.

Why do you think it should be the resolver's job? In the runtime implementation in fluent.js we unescape during parse-time. From what I've seen, it's a common practice in many languages. Do you think we should move this logic into the resolver?

This approach has the added benefit of standardizing how the escaping mechanism works exactly on the parser level, rather than leaving it up to the (currently unspecified) runtime.

The parser-serializer roundtrip continues to use the raw escaped value.

Have we considered storing only what we now have in raw field? If we did and decided somehow against it, maybe we should at least reverse the fields and store what's in raw as the actual value, and then what's currently in value as an implementation-specific optimization as encoded_value.

projectfluent/fluent#195 has the discussion and the details on this design. if you have ideas on how to improve it, please open a new issue. I'd like to avoid going back at this point in order to streamline the effort of implementing Syntax 0.8. A new issue can be discussed and then implemented independently of this PR.

In particular, Rust parser may want to skip the encoded_value in AST to keep it allocation free.

Just skip value, then?

[zbraniecki]

In the runtime implementation in fluent.js we unescape during parse-time.

That's reasonable. We parse exclusively to display there, and we build whatever works best for display. AST is meant to store semantic representation of the source.

Do you think we should move this logic into the resolver?

No.

The parser-serializer roundtrip continues to use the raw escaped value.

Which makes me think that maybe this should be the value field.

if you have ideas on how to improve it, please open a new issue.

Will do! I don't think it should block :)