Document: don't allow characters with unicode property Bidi_Class in source files

[danarmak]

Update documentation with changes made in #10017. Previously discussed on scala-contributors.

This is my first PR and there are some things I'm not sure about:

• The EBNF uses several CharNoXxxOrYyy constructions that are not explicitly defined. Since I couldn't update the definition I had to add ...OrZzz to their names. Zzz is here "BidiFormatting" which makes the names annoyingly long, and you can't keep adding exceptions this way. Is that alright? I considered shorter names but just "bidi" is misleading.
• I linked to the Unicode standard definition of these codepoints. Is that a good thing? Should I additionally or instead explain why these particular characters are forbidden, linking to the CVE or some other discussion? (As for the Unicode link, I could make it even more explicit by explaining that these are the codepoints whose Bidi_Class has 'explicit strength' but most readers won't benefit from that.)
• I noted that Scala 12 forbids these characters anywhere in the source file (e.g. comments, identifiers) - is that useful? It's an implementation restriction. Should I mark the PR [nomerge] and edit out that part in the 2.13 version?

[SethTisue]

Welcome!

Is that alright? I considered shorter names but just "bidi" is misleading.

What you've done seems to me like the good choice under the circumstances. References to the long names are few. If we need to grow the names again in the future we could always reconsider at that point.

I linked to the Unicode standard definition of these codepoints. Is that a good thing?

Yes

Should I additionally or instead explain why these particular characters are forbidden

I'd say the PR would be mergeable either way. If you do decide to include a link and/or explanatory wording, it can be brief, as the topic is well covered elsewhere. (But note that from the spec, we don't link to particular scala/scala or scala/bug issues or PRs.)

I noted that Scala 12 forbids these characters anywhere in the source file (e.g. comments, identifiers) - is that useful? It's an implementation restriction. Should I mark the PR [nomerge] and edit out that part in the 2.13 version?

Though we do accept PRs against the 2.12 spec, we don't really care about the 2.12 spec anymore. The 2.13 spec is the one we actually care about. And the 2.13 spec should not include historical information about earlier versions; that would be considered clutter.

If you care to take the time to adjust both the 2.12 and 2.13 versions separately, fine, but we would also be perfectly content if you simply targeted the 2.13.x branch only.

[danarmak]

I adjusted the PR for 2.13. Please adjust the milestone tag, etc.

[lrytz]

That's not correct, the characters are not allowed at all in source files. In string / character literals, the characters can be included as unicode escapes, but not literally.

In 2.12 (just for the record), the characters are also not allowed at all. The difference is that in unicode escapes (of these forbidden characters) are allowed everywhere (not only in character / string literals) in 2.12.

[danarmak]

I got this wrong; the commit to 2.13 was different from the one to 2.12 and I thought 2.13 was more permissive, but it's the other way around. Fixing.

[danarmak]

I've changed this. It now seems a bit odd that this is in a list prefaced by saying "To construct tokens...". Do you think it should be moved to the first line of the section ("Scala source code consists of Unicode text" [and these characters are allowed])?

[lrytz]

I agree, I think the spec only needs to be changed in the first section. Something like this:

The nine Bidirectional explicit formatting characters \u202a - \u202e and \u2066 - \u2069 (inclusive) are forbidden from appearing in source files. Note that they can be represented using unicode escapes in string and character literals.

Paragraphs or sentences starting with "note" in the spec are clarifications that don't introduce new information. I think that's all that's needed here, no changes are needed in character / string literal sections.

[danarmak]

Then do you think the other file (13-syntax-summary.md) doesn't need to be changed at all, and adding this note in this file is enough?

[lrytz]

For me that would be enough, but maybe others would prefer also a not in the syntax summary file? Things are not very precise already, for example stringElement ::= charNoDoubleQuoteOrNewline | escapeSeq but charNoDoubleQuoteOrNewline is not actually defined in the spec as far as I can tell...

[danarmak]

Yes, I noted the implicitly-defined xxxOrYyy constructions in the syntax summary (there are several of them) in a previous comment.

FWIW I would prefer a note in the syntax summary; that's where I looked first (and I was surprised that the summary is not an exact subset of the full grammar's productions).

[danarmak]

I pushed a new commit that makes only that change to both files and does not change any grammar productions. What do you think?

[lrytz]

To me that looks good, thank you!

[som-snytt]

Thanks, I agree it didn't feel right as a production issue.

I recall that the deceptively simple "Scala source code consists of Unicode text." was subject to long discussions.

BTW, I just noticed that JLS uses AbutNotB style, but always with a definition. Not a great example but UnqualifiedMethodIdentifier := Identifier but not yield. There they didn't actually call it IdentifierNotYield.

[danarmak]

I recall that the deceptively simple "Scala source code consists of Unicode text." was subject to long discussions.

Are there any other Unicode codepoints that are not allowed in Scala source code? Or did the discussions agree on allowing all of Unicode, and then this bidi issue came up?

[som-snytt]

That change wasn't so long ago (mid-pandemic): b124a54

I only remember that sjrd stepped in with clarifying wording.

Yes, all of Unicode, but then this security issue came up, as the spirits do on Halloween. 🎃

[lrytz]

thanks you @danarmak