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

fmt: scan mishandles width for %d #9444

Closed
robpike opened this issue Dec 26, 2014 · 3 comments
Closed

fmt: scan mishandles width for %d #9444

robpike opened this issue Dec 26, 2014 · 3 comments
Assignees
Milestone

Comments

@robpike
Copy link
Contributor

@robpike robpike commented Dec 26, 2014

http://play.golang.org/p/WlMpKiMGIv fails but if the format for the integer is %d rather than %5d, it works fine.

Snippet here reproduced here for clarity:

package main

import "fmt"

func main() {
format := "%6s%5d"
// "ssssssddddd"
line := "some 3"

var str string
var numb int64

n, err := fmt.Sscanf(line, format, &str, &numb)

fmt.Println(n, err)
fmt.Println(str, numb)  

}

The use of widths vs. space-separated tokens confuses. It's possible this is working as intended but if so it needs to be explained. I believe it's a bug.

@robpike robpike self-assigned this Dec 26, 2014
@robpike robpike changed the title fmt: scan mishandles with for %d fmt: scan mishandles width for %d Jan 6, 2015
@odeke-em
Copy link
Member

@odeke-em odeke-em commented Feb 25, 2015

Hello @robpike, I wonder if you are actively working on this issue.
I got some time this evening and did a trace through the code and figured that the EOF trip is toggled because there is a one off or greater increment of the s.count variable in fmt/scan.go in function: ReadRune() which fails at condition:

func (s *ss) ReadRune() (r rune, size int, err error) {
...
    if s.atEOF || s.nlIsEnd && s.prevRune == '\n' || s.count >= s.argLimit {
           err = io.EOF
          return
    }
...
}

In the source code, I narrowed it down to:

diff --git a/src/fmt/scan.go b/src/fmt/scan.go
index d7befea..dc989f2 100644
--- a/src/fmt/scan.go
+++ b/src/fmt/scan.go
@@ -200,7 +200,7 @@ func (s *ss) ReadRune() (r rune, size int, err error) {

    r, size, err = s.rr.ReadRune()
    if err == nil {
-       s.count++
+       // s.count++ <- Troublesome spot here
        s.prevRune = r
    } else if err == io.EOF {
        s.atEOF = true

The s.count increment made before selecting s.prevRune seems unnecessary since we are just peeking and checking for EOF (at least I think we shouldn't make the increment).

Therefore, by the point the EOF test is made, the value of s.count is always 9 and so is s.argLimit (given your example) hence the error. Modifying the widths always maintained the condition s.count >= s.argLimit.

To give more flesh to this hypothesis,
if you add at least two spaces greater than the space left over from the actual space chars in the line being parsed, the bug is accounted for as seen with http://play.golang.org/p/5BKRizLGyD

To be honest, this isn't close to a good analysis: I just figured I could take a shot at it with some free time. I could totally be wrong but for now this cuts the deal and also works with a few other tests.
Any comments, hints or wisdom would be greatly appreciated.

PS: If you'd like to see my trace through to see how I narrowed it down, please let me know. I didn't include the diff because it would be too much noise.

Thank you.

@odeke-em
Copy link
Member

@odeke-em odeke-em commented Mar 21, 2015

My change breaks a couple of fmt/scan tests:

  • in particular it chokes on recognizing integer values when a string format specifier is passed in e.g it fails with

    • ("%2s%3s", "22333") and ("%4v%s", "12abcd")

    vs

    • ("%2d%3d", "22333").

Still working at understanding the real cause of the bug.

@rsc rsc added this to the Go1.5 milestone Apr 10, 2015
@gopherbot
Copy link

@gopherbot gopherbot commented Jun 13, 2015

CL https://golang.org/cl/10997 mentions this issue.

@robpike robpike closed this in a1fe3b5 Jun 13, 2015
@golang golang locked and limited conversation to collaborators Jun 25, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.