Skip to content
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
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@linzhp
Copy link
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 go/parser error line mismatch cmd/cover: use different //line comment for code that does not come from source file May 7, 2018
@ianlancetaylor
Copy link
Contributor

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.

@ianlancetaylor ianlancetaylor added help wanted NeedsFix The path to resolution is known, but the work has not been done. labels May 7, 2018
@ianlancetaylor ianlancetaylor added this to the Go1.12 milestone May 7, 2018
@nodo
Copy link
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
Copy link
Contributor

@nodo Sure, thanks.

@linzhp
Copy link
Contributor Author

linzhp commented May 15, 2018

Thanks @nodo

@nodo
Copy link
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
Copy link
Contributor

@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
Copy link
Contributor

nodo commented May 31, 2018

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

@gopherbot
Copy link

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

@nodo
Copy link
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
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
Copy link

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
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>
@ianlancetaylor ianlancetaylor modified the milestones: Go1.12, Unplanned Dec 12, 2018
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

4 participants