-
Notifications
You must be signed in to change notification settings - Fork 18k
text/scanner: Float tokens allow trailing 'e' or 'E' and sign (but Go literals don't) #26374
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
Comments
@ianlancetaylor What investigation does this need? Is it something I could tackle? |
The "investigation" that it needs is exactly the answer to your question in the last paragraph: do people like @griesemer agree that this is a bug? @griesemer is on vacation this month and will no doubt take a look when he returns. |
This does look like a bug to me; and there doesn't seem to be a test expecting these erroneous constants either. If there was any intent behind this at some point, I don't remember. It seems ok to fix this; I'd be surprised if existing code relies on this, especially because the respective constant literals wouldn't make it through the strconv routines anyway. The fix might be as simple as: diff --git a/src/text/scanner/scanner.go b/src/text/scanner/scanner.go
index 4e76664dc0..893a4edbaf 100644
--- a/src/text/scanner/scanner.go
+++ b/src/text/scanner/scanner.go
@@ -384,6 +384,9 @@ func (s *Scanner) scanExponent(ch rune) rune {
if ch == '-' || ch == '+' {
ch = s.next()
}
+ if !isDecimal(ch) {
+ s.error("illegal exponent")
+ }
ch = s.scanMantissa(ch)
}
return ch (plus test cases, etc.) |
Thanks, @griesemer. I'll see about writing up a CL with tests when I get back from vacation in a week or so. |
Change https://golang.org/cl/129095 mentions this issue: |
@griesemer I added the fix you suggested and also tests to match: https://go-review.googlesource.com/c/go/+/129095 One of the things that was a bit unclear to me was how much data should be consumed when an error occurs. For example, parsing |
@benhoyt The scanner should consume characters as long as they are possibly leading to a valid token, and stop as soon as an error is detected. (I'm not saying this is what happens consistently, but at least that's the idea.) For |
The documentation for text/scanner says that it "recognizes all literals as defined by the Go language specification". However, it seems to break that and be a little more liberal in that it accepts floating-point literals with a trailing
e
orE
and no digits. It also allows a trailing+
or-
sign after thee
/E
. Neither of these are valid Go floating point literals (nor as input to ParseFloat, which seems to match the Go literal spec here).Here is a Playground link to a small program that reproduces the issue: https://play.golang.org/p/leS1zsI4Rfz
In short, scanning the string
"1.5e 1.5E 1e 1E 1e+ 1e- 1.0 1 1.5z"
gives the following sequence of tokens (third value on each line is whether it's valid according to Go/ParseFloat or not):However, what I'd expect according to the text/scanner docs is for the trailing
e
's and signs to be not included in the float, but parsed as subsequent tokens, like so:Here's the relevant scanner.go source code. scanExponent uses scanMantissa, which allows 0 or more digits instead of 1 or more digits (for this case).
I ran into this issue when using text/scanner and then passing the TokenText() return value for Float tokens to ParseFloat, and getting an unexpected error on strings like
1.5e
.Go version and OS/arch:
go1.10 darwin/amd64
. Though from looking at the code it's not arch- or OS-specific. Yes, the issue reproduces with the latest release.If folks (@griesemer?) agree that this is a bug, I'd be interested in contributing a fix. However, if it's decided that users may be depending on this behavior (seem unlikely as this apparently hasn't been reported yet -- I searched for issues) and it's too late to change it, then we'll want a documentation fix.
The text was updated successfully, but these errors were encountered: