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

x/tools/analysis/passes/printf: regression in format type checks #28858

Closed
kevinburke opened this issue Nov 18, 2018 · 9 comments
Closed

x/tools/analysis/passes/printf: regression in format type checks #28858

kevinburke opened this issue Nov 18, 2018 · 9 comments
Labels
FrozenDueToAge
Milestone
Go1.12

Comments

@kevinburke
Copy link
Contributor

kevinburke commented Nov 18, 2018 •
edited

The following code is incorrect - I meant to write len(s) not s.

package main

import (
	"testing"
)

func TestStringFormat(t *testing.T) {
	s := make([]string, 0)
	if len(s) != 1 {
		// "s" below is a mistake; I meant to type "len(s)"
		//
		// I expect to get this error:
		// ./lib_test.go:26: Errorf format %d has arg s of wrong type []string
		t.Errorf("wrong length: want %d got %d", 1, s)
	}
}

In Go 1.10, running go test returns a vet error:

./lib_test.go:26: Errorf format %d has arg s of wrong type []string

However, in Go 1.11 and above, go test does not return an error - it instead returns:

lib_test.go:26: wrong length: want 1 got []

I added a repository to reproduce - you can see in the Travis CI build which fails on Go 1.10 but passes on Go 1.11 and tip. https://travis-ci.org/kevinburke/test-fmt-repro/builds/456666561
@gopherbot gopherbot added this to the Unreleased milestone Nov 18, 2018
@kevinburke
Copy link
Contributor Author

I tried to search whether this change was deliberate but couldn't find anything. It's possible it was deliberate.

@ALTree
Copy link
Member

ALTree commented Nov 18, 2018

It appears that this was introduced in 98409a4 (cmd/vet: better align print warnings with fmt).

@mvdan
Copy link
Member

mvdan commented Nov 19, 2018

This does look like a regression introduced by that commit of mine a while ago. Will have a look.

@mvdan mvdan assigned mvdan and unassigned alandonovan Nov 19, 2018
@mvdan
Copy link
Member

mvdan commented Nov 19, 2018

Thanks for the report, @kevinburke. This turned out to uncover another batch of bugs in vet's printf checker, where it was fairly inconsistent with what fmt does. See https://go-review.googlesource.com/c/tools/+/149979.

In its defense, fmt is poorly documented when it comes to pointers. I've raised #28865 for that.

@alandonovan
Copy link
Contributor

Thanks for the quick fix, Daniel.

@kevinburke
Copy link
Contributor Author

OK - should we mark this as a release blocker?

@mvdan
Copy link
Member

mvdan commented Nov 19, 2018 •
edited

Certainly seems like something we can fix in 1.12 to me, but it's not a regression in tip, so I wouldn't call it a blocker. Will leave the decision to Alan.

@alandonovan
Copy link
Contributor

I agree with Daniel: not a regression, not a release blocker. Shouldn't be hard to fix though.

@mvdan mvdan modified the milestones: Unreleased, Go1.12 Nov 19, 2018
@gopherbot
Copy link

Change https://golang.org/cl/152277 mentions this issue: cmd/vendor: update to golang.org/x/tools@e5f3ab76

@golang golang locked and limited conversation to collaborators Dec 3, 2019
@rsc rsc unassigned mvdan Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1.12
Development

No branches or pull requests
5 participants
@kevinburke @mvdan @ALTree @alandonovan @gopherbot