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.

Sign up for GitHub

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

encoding/csv: Read returns partial record on error #59991

Closed
IvoGoman opened this issue May 4, 2023 · 4 comments
Closed

encoding/csv: Read returns partial record on error #59991

IvoGoman opened this issue May 4, 2023 · 4 comments
Labels
Documentation help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Backlog

Comments

@IvoGoman
Copy link
Contributor

IvoGoman commented May 4, 2023

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

$ go version go1.20.1 darwin/arm64

Does this issue reproduce with the latest release?

Yes it does

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

go env Output
$ GO111MODULE=""
GOARCH="arm64"
GOBIN=""
GOCACHE="/Users//Library/Caches/go-build"
GOENV="/Users//Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="arm64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users//go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users//go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/opt/homebrew/Cellar/go/1.20.1/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/opt/homebrew/Cellar/go/1.20.1/libexec/pkg/tool/darwin_arm64"
GOVCS=""
GOVERSION="go1.20.1"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users//go.mod"
GOWORK=""
CGO_CFLAGS="-O2 -g"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-O2 -g"
CGO_FFLAGS="-O2 -g"
CGO_LDFLAGS="-O2 -g"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/29/tcsmnyzs7px55jxt3grz88c80000gn/T/go-build3619271105=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I have read an invalid csv with csv.Reader

invalidCSV := "one,\"two\"\"" # extra quote which leads to parse error
csvReader := csv.NewReader(strings.NewReader(invalidCSV))
res, err := csvReader.Read()

See: https://go.dev/play/p/p66Sy_Z4494

What did you expect to see?

I have expected to receive a nil record and a non-nil error indicating the problem with this line of csv.

What did you see instead?

The method returns a partially parsed record and the appropriate error ErrQuote.
The method's documentation states this should never happen:

go/src/encoding/csv/reader.go

Lines 188 to 191 in fc106b0

// If the record has an unexpected number of fields,
// Read returns the record along with the error ErrFieldCount.
// Except for that case, Read always returns either a non-nil
// record or a non-nil error, but not both.

From what I can tell the behavior has changed 6 years ago with this change: 2181653
@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 4, 2023
@seankhliao seankhliao added this to the Backlog milestone May 4, 2023
@seankhliao
Copy link
Member

cc @dsnet @bradfitz @rsc as secondary owners

@dsnet
Copy link
Member

dsnet commented May 4, 2023

While the convention is that a non-nil error implies a nil return value, this is not required. Callers that care about errors should be checking the error before using the value.

I don't see how we can change this without breaking existing usages that are depending on this semantic.

@ianlancetaylor
Copy link
Contributor

We shouldn't change the behavior but we should certainly update the docs.

@ianlancetaylor ianlancetaylor added Documentation help wanted NeedsFix The path to resolution is known, but the work has not been done. labels May 5, 2023
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 5, 2023
@IvoGoman IvoGoman mentioned this issue May 9, 2023
Closed
@gopherbot
Copy link

Change https://go.dev/cl/494055 mentions this issue: encoding/csv: update doc comment of Read method

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Documentation help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Backlog
Development

Successfully merging a pull request may close this issue.

encoding/csv: update doc comment of Read method IvoGoman/go
5 participants
@ianlancetaylor @dsnet @IvoGoman @gopherbot @seankhliao