code review 124940043: cmd/go, go/build: implement import comment checking

cmd/go, go/build: implement import comment checking

See golang.org/s/go14customimport for design.

Added case to deps_test to allow go/build to import regexp.
Not a new dependency, because go/build already imports go/doc
which imports regexp.

Fixes issue 7453.

[rsc, 2014-08-08 19:06:48]

Hello r (cc: golang-codereviews@googlegroups.com),

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

[r, 2014-08-08 19:33:56]

surely doc.go needs to be updated.

[r, 2014-08-08 19:39:40]

https://codereview.appspot.com/124940043/diff/40001/src/pkg/go/build/build.go
File src/pkg/go/build/build.go (right):

https://codereview.appspot.com/124940043/diff/40001/src/pkg/go/build/build.go...
src/pkg/go/build/build.go:793: importPattern  = `\A` + spaceOrComment +
`*package[ \t\r\v\f]+[\p{L}0-9_]+[ \t\r\v\f]*//[ \t\r\v\f]*import[
\t\r\v\f]*([^\n]*?)[ \t\r\v\f]*\n`
really \r? it shouldn't appear except before \n, which is explicit in the
pattern.
also drop \f and \v, which are not "white space" in go.

this is inscrutable but i don't believe it accepts

package foo /* import "asdf" */

but i think it should.

i'd avoid regexps and just write some code here. it'll be easier to understand.

[rsc, 2014-08-08 20:43:59]

On 2014/08/08 19:39:40, r wrote:
> https://codereview.appspot.com/124940043/diff/40001/src/pkg/go/build/build.go
> File src/pkg/go/build/build.go (right):
> 
>
https://codereview.appspot.com/124940043/diff/40001/src/pkg/go/build/build.go...
> src/pkg/go/build/build.go:793: importPattern  = `\A` + spaceOrComment +
> `*package[ \t\r\v\f]+[\p{L}0-9_]+[ \t\r\v\f]*//[ \t\r\v\f]*import[
> \t\r\v\f]*([^\n]*?)[ \t\r\v\f]*\n`
> really \r? it shouldn't appear except before \n, which is explicit in the
> pattern.
> also drop \f and \v, which are not "white space" in go.
> 
> this is inscrutable but i don't believe it accepts
> 
> package foo /* import "asdf" */
> 
> but i think it should.
> 
> i'd avoid regexps and just write some code here. it'll be easier to
understand.

I started with real code and it wasn't easier to understand. Once I drop \r\v\f
it will be that much clearer.
I'm happy to write real code if you'd prefer.

On the topic of the comment form, the doc (golang.org/s/go14customimport) says:

"If a package statement in a package has an end-of-line comment of the form `//
import "path"`, then the go command will refuse to install the code under any
import path except the given one."

so I don't believe /* import "asdf" */ counts.

[r, 2014-08-08 20:47:41]

It seems wrong to me that

package x // import "a"

works but

package x /* import "a" */

does not. Unlike the //go: stuff, this is intended to be human-friendly and
accepting only one format of comments is likely to be surprising since there
will be no message saying "that thing you thought was an import qualifier does
nothing".

[rsc, 2014-08-08 20:52:36]

On Fri, Aug 8, 2014 at 4:47 PM, <r@golang.org> wrote:

> It seems wrong to me that
>
> package x // import "a"
>
> works but
>
> package x /* import "a" */
>

Where is the line?

package x /*
import "a"
*/

?

I implemented the definition in the doc. If you write a new definition I'll
put it in the doc and implement it.

Russ

[r, 2014-08-08 20:59:46]

The XXX (what's it called? an import qualifier) is specified with a
comment that is the first non-space item on the line after the package
name. The comment can be either in // form or in /* */ form with no
newlines within.

package x // import "a"
package x /* import "a" */

[r, 2014-08-08 21:02:30]

Moreover, if there is a comment but it's not in this form, there
should probably be a complaint from the go tool.

My fear is that someone will write a comment they think defines the
import but that it actually doesn't and forks become possible,
silently.

-rob

[rsc, 2014-08-12 17:43:14]

There's a tension between correcting mistakes and disallowing all code that
has a comment there now.
Right now as long as we can find 'import' then we require the rest to have
the correct form.
But things like

package p // i wish this was q

are ignored.

Russ

[rsc, 2014-08-12 18:14:02]

PTAL

I have updated golang.org/s/go14customimport to give the new rule, and I
have updated the code to implement it, without a regular expression.

If a comment is found and that comment begins with the word 'import', then
the rest of the comment must take the expected form. This is tested
in testdata/importcom/src/bad/bad.go. If a comment is found that does not
begin with 'import', that comment is ignored. This is tested in
testdata/importcom/src/works/x/x1.go.

[r, 2014-08-12 18:18:39]

SGTM. The import keyword is a required part of the specification.

-rob


On Tue, Aug 12, 2014 at 10:43 AM, Russ Cox <rsc@golang.org> wrote:
> There's a tension between correcting mistakes and disallowing all code that
> has a comment there now.
> Right now as long as we can find 'import' then we require the rest to have
> the correct form.
> But things like
>
> package p // i wish this was q
>
> are ignored.
>
> Russ

[josharian, 2014-08-12 18:20:56]

https://codereview.appspot.com/124940043/diff/70014/src/cmd/go/pkg.go
File src/cmd/go/pkg.go (right):

https://codereview.appspot.com/124940043/diff/70014/src/cmd/go/pkg.go#newcode425
src/cmd/go/pkg.go:425: // expandNoGo expands build.NoGoError's message to say
whether there
expandNoGo--here and below--is probably meant for a different CL, one addressing
https://groups.google.com/forum/#!topic/golang-dev/jyQq8ZYmZaI.

[r, 2014-08-12 18:24:51]

LGTM

https://codereview.appspot.com/124940043/diff/70014/src/cmd/go/pkg.go
File src/cmd/go/pkg.go (right):

https://codereview.appspot.com/124940043/diff/70014/src/cmd/go/pkg.go#newcode270
src/cmd/go/pkg.go:270: err = fmt.Errorf("%s contains package %q", bp.Dir,
bp.ImportComment)
maybe
package in directory %q has package path %q

[rsc, 2014-08-12 21:40:21]

https://codereview.appspot.com/124940043/diff/70014/src/cmd/go/pkg.go
File src/cmd/go/pkg.go (right):

https://codereview.appspot.com/124940043/diff/70014/src/cmd/go/pkg.go#newcode270
src/cmd/go/pkg.go:270: err = fmt.Errorf("%s contains package %q", bp.Dir,
bp.ImportComment)
Submitted as is, but I'm happy to try to iterate on this separately.
I assume the main objection is package %q vs package path %q.
(Can't use %q for directories because of Windows.)

Right now it prints something like

github.com/rsc/pdf: /Users/rsc/g/src/github.com/rsc/pdf contains package
"rsc.io/pdf"

If it said:

github.com/rsc/pdf: package in directory /Users/rsc/g/src/github.com/rsc/pdf has
package path "rsc.io/pdf"

then I'm a little worried about not getting across that it's the source code
that's the problem. Maybe:

github.com/rsc/pdf: code in directory /Users/rsc/g/src/github.com/rsc/pdf
expects import "rsc.io/pdf"

?

[rsc, 2014-08-12 21:41:07]

*** Submitted as https://code.google.com/p/go/source/detail?r=d4469d39943e ***

cmd/go, go/build: implement import comment checking

See golang.org/s/go14customimport for design.

Added case to deps_test to allow go/build to import regexp.
Not a new dependency, because go/build already imports go/doc
which imports regexp.

Fixes issue 7453.

LGTM=r
R=r, josharian
CC=golang-codereviews
https://codereview.appspot.com/124940043

[r, 2014-08-12 21:43:46]

On 2014/08/12 21:40:21, rsc wrote:
> https://codereview.appspot.com/124940043/diff/70014/src/cmd/go/pkg.go
> File src/cmd/go/pkg.go (right):
> 
>
https://codereview.appspot.com/124940043/diff/70014/src/cmd/go/pkg.go#newcode270
> src/cmd/go/pkg.go:270: err = fmt.Errorf("%s contains package %q", bp.Dir,
> bp.ImportComment)
> Submitted as is, but I'm happy to try to iterate on this separately.
> I assume the main objection is package %q vs package path %q.
> (Can't use %q for directories because of Windows.)
> 
> Right now it prints something like
> 
> github.com/rsc/pdf: /Users/rsc/g/src/github.com/rsc/pdf contains package
> "rsc.io/pdf"
> 
> If it said:
> 
> github.com/rsc/pdf: package in directory /Users/rsc/g/src/github.com/rsc/pdf
has
> package path "rsc.io/pdf"
> 
> then I'm a little worried about not getting across that it's the source code
> that's the problem. Maybe:
> 
> github.com/rsc/pdf: code in directory /Users/rsc/g/src/github.com/rsc/pdf
> expects import "rsc.io/pdf"
> 
> ?

I like your final wording.