code review 6500075: cmd/gc: give better error for UTF-8-encoded BOM

cmd/gc: give better error for UTF-8-encoded BOM

Fixes issue 4040.

[rsc, 2012-09-04 19:26:59]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go/

[bradfitz, 2012-09-04 19:29:26]

LGTM

On Tue, Sep 4, 2012 at 12:26 PM, <rsc@golang.org> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> cmd/gc: give better error for UTF-8-encoded BOM
>
> Fixes issue 4040.
>
> Please review this at
http://codereview.appspot.com/**6500075/<http://codereview.appspot.com/6500075/>
>
> Affected files:
>   M src/cmd/gc/lex.c
>   A test/syntax/bom.go
>
>
> Index: src/cmd/gc/lex.c
> ==============================**==============================**=======
> --- a/src/cmd/gc/lex.c
> +++ b/src/cmd/gc/lex.c
> @@ -1571,6 +1571,7 @@
>         char str[UTFmax+1];
>         Rune rune;
>
> +top:
>         c = getc();
>         if(c < Runeself)
>                 return c;
> @@ -1591,7 +1592,13 @@
>                 for(c=0; c<i; c++)
>                         print("%s%.2x", c > 0 ? " " : "",
> *(uchar*)(str+c));
>                 print("\n");
> +               errorexit();
>         }
> +       if(rune == 0xfeff) {
> +               yyerror("Unicode byte-order mark not allowed in Go
> source");
> +               goto top;
> +       }
> +
>         return rune;
>  }
>
> Index: test/syntax/bom.go
> ==============================**==============================**=======
> new file mode 100644
> --- /dev/null
> +++ b/test/syntax/bom.go
> @@ -0,0 +1,8 @@
> +// errorcheck
> +
> +// Copyright 2012 The Go Authors.  All rights reserved.
> +// Use of this source code is governed by a BSD-style
> +// license that can be found in the LICENSE file.
> +
> +package p // ERROR "Unicode byte-order mark not allowed"
> +
>
>
>

[dsymonds, 2012-09-05 04:24:57]

http://codereview.appspot.com/6500075/diff/2002/src/cmd/gc/lex.c
File src/cmd/gc/lex.c (right):

http://codereview.appspot.com/6500075/diff/2002/src/cmd/gc/lex.c#newcode1597
src/cmd/gc/lex.c:1597: if(rune == 0xfeff) {
I thought it's only a BOM if it's at the start of the input. I don't see the
problem with a string literal starting with a BOM, for instance.

[r, 2012-09-05 14:58:35]

i'm still thinking about what's right and don't want to make a
decision yet, but I believe David is correct. My current working
hypothesis is:

1. BOM should be legal and discarded if it is the first code point in the file.
2. BOM should be legal and preserved inside strings both raw and double-quoted.
3. BOM should be illegal everywhere else.

It's possible that point 1 should read "preserved".

This seems to meet what the Unicode consortium asks, codifying idiocy
with the goal of making progress.

-rob

[rsc, 2012-09-06 00:45:58]

On Wed, Sep 5, 2012 at 10:58 AM, Rob Pike <r@golang.org> wrote:
> i'm still thinking about what's right and don't want to make a
> decision yet, but I believe David is correct. My current working
> hypothesis is:
>
> 1. BOM should be legal and discarded if it is the first code point in the
file.
> 2. BOM should be legal and preserved inside strings both raw and
double-quoted.
> 3. BOM should be illegal everywhere else.
>
> It's possible that point 1 should read "preserved".

I assume you're talking about gofmt / gofix. I don't believe it should
be preserved by those tools. We don't preserve Windows line endings
either: we assume that there is a single unique encoding of a
particular AST.

I do think that if we want to start allowing beginning of file BOMs in
source code, we need to mention it in the spec. It could be added onto
this paragraph:

    Implementation restriction: For compatibility with other tools, a
compiler may
    disallow the NUL character (U+0000) in the source text. A compiler may also
    ignore a leading Unicode byte-order mark (U+FEFF) in a source file.

Russ

[adg, 2012-09-06 01:15:33]

On 6 September 2012 10:45, Russ Cox <rsc@golang.org> wrote:
> On Wed, Sep 5, 2012 at 10:58 AM, Rob Pike <r@golang.org> wrote:
>> i'm still thinking about what's right and don't want to make a
>> decision yet, but I believe David is correct. My current working
>> hypothesis is:
>>
>> 1. BOM should be legal and discarded if it is the first code point in the
file.
>> 2. BOM should be legal and preserved inside strings both raw and
double-quoted.
>> 3. BOM should be illegal everywhere else.
>>
>> It's possible that point 1 should read "preserved".
>
> I assume you're talking about gofmt / gofix. I don't believe it should
> be preserved by those tools. We don't preserve Windows line endings
> either: we assume that there is a single unique encoding of a
> particular AST.

+1 to this. It would be a great benefit for gofmt to strip the BOM.

Andrew

[r, 2012-09-06 01:28:34]

Sounds like my proposal has resonance.

-rob

[r, 2012-09-06 03:38:58]

I had dinner with Ken and he talked me into a slightly different
proposal. Rules are changed to discard BOM everywhere outside string
literals. The argument is that if you're going to toss them at the
beginning, might as well toss them elsewhere too, so concatenated
files don't crop up later. (You won't concatenate mid-string literal.)

We now have two simple rules, both implemented in the lexer:

1. BOM should be legal and preserved inside strings both raw and double-quoted.
2. BOM should be discarded everywhere else.

We believe that these rules mean we won't have to have this design
discussion again.

-rob

[rsc, 2012-09-06 03:49:50]

If you do the spec change I'll fix the compiler.

[dsymonds, 2012-09-06 04:17:59]

On Thu, Sep 6, 2012 at 1:38 PM, Rob Pike <r@golang.org> wrote:

> 1. BOM should be legal and preserved inside strings both raw and
double-quoted.

What about a BOM character literal?

[r, 2012-09-06 06:28:13]

Rune literal you mean. Yes, that too.

I'll do the spec tomorrow.

-rob

[rsc, 2012-09-10 16:08:53]

*** Abandoned ***