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

Jump to bottom

cmd/cover: //line directives frequently handled incorrectly #41222

Open
pjweinb opened this issue Sep 4, 2020 · 3 comments
Open

cmd/cover: //line directives frequently handled incorrectly #41222

pjweinb opened this issue Sep 4, 2020 · 3 comments
Labels
help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@pjweinb
Copy link

pjweinb commented Sep 4, 2020

(This combines #35781, #33690, #25280, and golang/vscode-go#603)

Source files that contain //line directives confuse the coverage tool, prducing coverage output that is unhelpful or confusing to users.

For instance, a short main.go (import path foo.gak) with first lines

//
package main

produces a coverage file:

mode: set
foo.gak/main.go:6.10,8.26 2 1
foo.gak/main.go:12.2,12.19 1 1
foo.gak/main.go:8.26,11.3 2 1
foo.gak/main.go:12.19,13.21 1 0

If the first line is changed to

//line con.go:8

the coverage file changes to:

mode: set
foo.gak/main.go:12.0,14.0 2 1
foo.gak/main.go:18.0,18.0 1 1
foo.gak/main.go:14.0,17.0 2 1
foo.gak/main.go:18.0,19.0 1 0

This file does not refer to con.go, and the ranges are wrong (or, at best, imprecise) for that file, and completely wrong for main.go.

A minimal improvement would be for the coverage tool to change user's //line directives to bare // comments, so that the coverage data corresponds to the file presented to the tool. [Getting results that point to files mentioned in //line directives would require changing the format of the tables in the generated code, at least. Even then the results might be peculiar, for instance if the generated file were run through go fmt.]
@andybons
Copy link
Member

andybons commented Sep 8, 2020

@golang/tools-team

@andybons andybons added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 8, 2020
@andybons andybons added this to the Unplanned milestone Sep 8, 2020
@ianlancetaylor ianlancetaylor modified the milestones: Unplanned, Backlog Oct 14, 2020
@brianpursley
Copy link

brianpursley commented Nov 2, 2020 •
edited

I was looking into this a little bit and here is some more background information...

According to the Go Programming Language Reference, line directives can be of //line or /*line ...*/ format:

In order to be recognized as a line directive, the comment must start with //line or /*line followed by a space, and must contain at least one colon. The //line form must start at the beginning of a line. A line directive specifies the source position for the character immediately following the comment as having come from the specified file, line and column: For a //line comment, this is the first character of the next line, and for a /*line comment this is the character position immediately following the closing */.

The //line format wouldn't be too hard to replace because it has to be at the start of the line, but /*line ...*/ is more complicated because it can be anywhere on a line. However unlikely, it could be inside a string, so it is not just a simple replace... replacing it if it is inside a string, would change the program behavior.

I suppose one approach would be to just fix it for //line type directives (by replacing those lines with //) and leave /*line ...*/ as-is, and it would be a known limitation of cover.

WDYT?

@gopherbot
Copy link

Change https://go.dev/cl/447957 mentions this issue: src/goCover: ignore bogus-looking line/column data

gopherbot pushed a commit to golang/vscode-go that referenced this issue Nov 7, 2022
@hyangah
src/goCover: ignore bogus-looking line/column data
f67d75f 
Go's line directive can cause the coverage data to include bogus
line/column info.

The extension naively assumed the coverage data position info is
always valid and line/column numbers are 1-based sane numbers. Of course,
that's not always true any more.

The example in #2453 generated the following coverage data.

  mode: set
  blah/blah.go:3.16,5.2 1 1
  blah/blah.go:7.16,9.2 1 1
  blah/blah.go:13.16,98989.0 2 1
  blah/blah.go:98992.0,98993.0 2 1
  blah/blah.go:98989.0,98991.0 1 0

The column values were 0. The extension blindly converted them to 0-based
positions by subtracting 1. This illegal range eventually made the vscode to
throw an exception eventually.

I think the go test -cover shouldn't use the bogus line/col info in
coverage profile, but that is another issue.

It does not make sense to map such bogus numbers, so, this CL filters them
out.

For golang/go#41222
Fixes #2453

Change-Id: I31a28fecae0a01406e0dc32c48dca1fb4f7b6f0c
Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/447957
Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com>
TryBot-Result: kokoro <noreply+kokoro@google.com>
Reviewed-by: Peter Weinberger <pjw@google.com>
Reviewed-by: Than McIntosh <thanm@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
5 participants
@andybons @ianlancetaylor @brianpursley @pjweinb @gopherbot