New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/cover: use different //line comment for code that does not come from source file #25280

Open
linzhp opened this Issue May 7, 2018 · 10 comments

Comments

Projects
None yet
4 participants
@linzhp
Contributor

linzhp commented May 7, 2018

What version of Go are you using (go version)?

go version go1.10.1 darwin/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GORACE=""
GOTMPDIR=""
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/m6/vs051rl54k3_sgxr94ylcz5h0000gn/T/go-build238749509=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

git clone git@github.com:linzhp/go_examples.git
cd go_examples/hyphen
go tool cover -mode set -var Var_power-driven -o power-driven-gen.go power-driven.go
go run main.go

What did you expect to see?

An error pointing to the right line of power-driven-gen.go

What did you see instead?

power-driven.go:7:14: expected ';', found '-'

However, power-driven.go has less than 7 lines.

@ianlancetaylor ianlancetaylor changed the title from go/parser error line mismatch to cmd/cover: use different //line comment for code that does not come from source file May 7, 2018

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented May 7, 2018

The go/parser package is working correctly. The error occurs in new code added by the cover tool. The cover tool should add a new //line comment for that code, so that it does not get misattributed to the original file.

In this case the bug is, of course, using a -var option with a name that is not a valid Go identifier. The cover tool could perhaps check for that too.

@nodo

This comment has been minimized.

Contributor

nodo commented May 15, 2018

Hey @ianlancetaylor, I would like to start contributing to Go. I would be interested in working on this if that's OK for you. What do you think?

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented May 15, 2018

@nodo Sure, thanks.

@linzhp

This comment has been minimized.

Contributor

linzhp commented May 15, 2018

Thanks @nodo

@nodo

This comment has been minimized.

Contributor

nodo commented May 20, 2018

@ianlancetaylor I have started to look at the issue and I have a couple of doubts.

In order to check that the string passed using -var is a valid Go identifier I considered using https://golang.org/pkg/go/scanner and check if the output of the scanner is exactly one identifier. If that is the case, then the option is valid.

Another possibility would be to match the string against a regex that matches https://golang.org/ref/spec#Identifiers.

What do you think about these solutions? Are there other options?

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented May 31, 2018

@nodo A valid Go identifier is defined by the language spec at https://golang.org/ref/spec#Identifiers. It's easy enough to write a function for that, something like:

first := true
for _, c := range ident {
    if !unicode.IsLetter(c) && c != '_' && (first || !unicode.IsDigit(c)) {
        return false // invalid identifier
    }
    first = false
}
@nodo

This comment has been minimized.

Contributor

nodo commented May 31, 2018

Awesome! Thanks @ianlancetaylor , I will continue that way.

@gopherbot

This comment has been minimized.

gopherbot commented Jun 21, 2018

Change https://golang.org/cl/120316 mentions this issue: cmd/cover: check that the argument of -var is valid

@nodo

This comment has been minimized.

Contributor

nodo commented Jun 21, 2018

@ianlancetaylor I have added a first CL with the check that the argument of -var is a valid go identifier. WDYT?

gopherbot pushed a commit that referenced this issue Dec 10, 2018

cmd/cover: check that the argument of -var is valid
At the moment, the cover tool does not check that the argument of -var
is a valid identifier. Hence, it could generate a file that fails to
compile afterwards.

Updates #25280

Change-Id: I6eb1872736377680900a18a4a28ba002ab5ea8ca
Reviewed-on: https://go-review.googlesource.com/c/120316
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@gopherbot

This comment has been minimized.

gopherbot commented Dec 11, 2018

Change https://golang.org/cl/153500 mentions this issue: cmd/cover: simplify and correct isValidIdentifier

gopherbot pushed a commit that referenced this issue Dec 11, 2018

cmd/cover: simplify and correct isValidIdentifier
Per comment on CL 120316.

Updates #25280

Change-Id: I7d078de4030bd10934468e04ff696a34749bd454
Reviewed-on: https://go-review.googlesource.com/c/153500
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Rob Pike <r@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment