|
|
|
go/parser: stop ParseFile after ten errors.
There wil be a panic if more than ten errors are encountered. ParseFile
will recover and return the ErrorList.
Fixes issue 3943.
Patch Set 1 #Patch Set 2 : diff -r a44171137af8 https://code.google.com/p/go #Patch Set 3 : diff -r a44171137af8 https://code.google.com/p/go #
Total comments: 6
Patch Set 4 : diff -r a44171137af8 https://code.google.com/p/go #
Total comments: 5
Patch Set 5 : diff -r a44171137af8 https://code.google.com/p/go #
Total comments: 14
Patch Set 6 : diff -r 8c44c45a208e https://code.google.com/p/go #
Total messages: 13
|
matloob
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go
|
12 years, 11 months ago (2013年02月10日 04:12:42 UTC) #1 | ||||||||||||||||||||||||||||||||
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go
https://codereview.appspot.com/7307085/diff/4001/src/pkg/go/parser/interface.go File src/pkg/go/parser/interface.go (right): https://codereview.appspot.com/7307085/diff/4001/src/pkg/go/parser/interface.... src/pkg/go/parser/interface.go:61: SpuriousErrors // report all (not just the first) errors per line Please add an AllErrors flag as in the original CL. Some tests rely on getting all errors, and w/o a flag the original behavior cannot be obtained anymore. I also suggest you run sh run.bash in src to verify all tests still pass... https://codereview.appspot.com/7307085/diff/4001/src/pkg/go/parser/interface.... src/pkg/go/parser/interface.go:90: defer handlePanic(&err) use a closure here instead, then you don't need to pass in &err https://codereview.appspot.com/7307085/diff/4001/src/pkg/go/parser/parser.go File src/pkg/go/parser/parser.go (right): https://codereview.appspot.com/7307085/diff/4001/src/pkg/go/parser/parser.go#... src/pkg/go/parser/parser.go:344: if p.errors.Len() > 10 { this needs to be guarded by a flag
Hello golang-dev@googlegroups.com, gri@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
https://codereview.appspot.com/7307085/diff/4001/src/pkg/go/parser/interface.go File src/pkg/go/parser/interface.go (right): https://codereview.appspot.com/7307085/diff/4001/src/pkg/go/parser/interface.... src/pkg/go/parser/interface.go:61: SpuriousErrors // report all (not just the first) errors per line Done. The tests still pass. Should any of them fail? On 2013年02月10日 06:25:04, gri wrote: > Please add an AllErrors flag as in the original CL. Some tests rely on getting > all errors, and w/o a flag the original behavior cannot be obtained anymore. > > I also suggest you run sh run.bash in src to verify all tests still pass... https://codereview.appspot.com/7307085/diff/4001/src/pkg/go/parser/interface.... src/pkg/go/parser/interface.go:90: defer handlePanic(&err) On 2013年02月10日 06:25:04, gri wrote: > use a closure here instead, then you don't need to pass in &err Done. https://codereview.appspot.com/7307085/diff/4001/src/pkg/go/parser/parser.go File src/pkg/go/parser/parser.go (right): https://codereview.appspot.com/7307085/diff/4001/src/pkg/go/parser/parser.go#... src/pkg/go/parser/parser.go:344: if p.errors.Len() > 10 { On 2013年02月10日 06:25:04, gri wrote: > this needs to be guarded by a flag Done.
https://codereview.appspot.com/7307085/diff/7003/src/pkg/go/parser/interface.go File src/pkg/go/parser/interface.go (right): https://codereview.appspot.com/7307085/diff/7003/src/pkg/go/parser/interface.... src/pkg/go/parser/interface.go:85: defer func() { move this past the readSource code - the defer is not needed until later https://codereview.appspot.com/7307085/diff/7003/src/pkg/go/parser/interface.... src/pkg/go/parser/interface.go:87: err = e.(scanner.ErrorList) // re-panics if it's not an ErrorList This is not quite correct: Now, if we have a bailout, the result is the ErrorList content, unsorted, and without multiples removed. We cannot easily remove multiples at this point because we might return less then 10 errors even if there are more (because we count multiples). But we also lose the sorting. I propose: 1) Instead of panicking with the error list, introduce a special type 'bailout': // A bailout panic is raised to indicate early termination. type bailout struct{} Then, exit early with panic(bailout{}). That also makes is more clearly in the code what the panic is for. 2) Here, if we have a panic and we don't have a bailout, re-panic. This is a real error in the parser and should never happen. 3) In all other case, do what ParseFile is doing now at the end. That is, use the deferred function to assign the error result in all cases (but the very first, readSource, which is not using the error list). I am thinking that in case of the limit to 10 errors, we probably don't want to remove the SpuriousErrors (no RemoveMultiples). In the other case, leave it as is for the same behavior, i.e., the SpuriousErrors flag only makes sense if AllErrors is set. https://codereview.appspot.com/7307085/diff/7003/src/pkg/go/parser/parser.go File src/pkg/go/parser/parser.go (right): https://codereview.appspot.com/7307085/diff/7003/src/pkg/go/parser/parser.go#... src/pkg/go/parser/parser.go:344: if p.mode&AllErrors == 0 && p.errors.Len() > 10 { s/>/>=/ otherwise you'll return 11 errors.
Hello golang-dev@googlegroups.com, gri@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
https://codereview.appspot.com/7307085/diff/7003/src/pkg/go/parser/interface.go File src/pkg/go/parser/interface.go (right): https://codereview.appspot.com/7307085/diff/7003/src/pkg/go/parser/interface.... src/pkg/go/parser/interface.go:85: defer func() { On 2013年02月11日 22:42:57, gri wrote: > move this past the readSource code - the defer is not needed until later Done. https://codereview.appspot.com/7307085/diff/7003/src/pkg/go/parser/interface.... src/pkg/go/parser/interface.go:87: err = e.(scanner.ErrorList) // re-panics if it's not an ErrorList Does this mean that a user who was supplying SpuriousErrors before now needs to supply SpuriousErrors|AllErrors to get the same effect? On 2013年02月11日 22:42:57, gri wrote: > This is not quite correct: Now, if we have a bailout, the result is the > ErrorList content, unsorted, and without multiples removed. We cannot easily > remove multiples at this point because we might return less then 10 errors even > if there are more (because we count multiples). > > But we also lose the sorting. > > I propose: > > 1) Instead of panicking with the error list, introduce a special type 'bailout': > > // A bailout panic is raised to indicate early termination. > type bailout struct{} > > Then, exit early with panic(bailout{}). That also makes is more clearly in the > code what the panic is for. > > 2) Here, if we have a panic and we don't have a bailout, re-panic. This is a > real error in the parser and should never happen. > > 3) In all other case, do what ParseFile is doing now at the end. That is, use > the deferred function to assign the error result in all cases (but the very > first, readSource, which is not using the error list). > > I am thinking that in case of the limit to 10 errors, we probably don't want to > remove the SpuriousErrors (no RemoveMultiples). In the other case, leave it as > is for the same behavior, i.e., the SpuriousErrors flag only makes sense if > AllErrors is set.
I think this is essentially what we want but in retrospect I think maybe we can simplify the flag handling and code a bit by re-interpreting the SpuriousErrors flag. https://codereview.appspot.com/7307085/diff/10001/src/pkg/go/parser/interface.go File src/pkg/go/parser/interface.go (right): https://codereview.appspot.com/7307085/diff/10001/src/pkg/go/parser/interface... src/pkg/go/parser/interface.go:60: SpuriousErrors // report all (not just the first) errors per line when AllErrors set see the comment below for an explanation, but I suggest to change this to: SpuriousErrors // same as AllErrors, for backward-compatibility AllErrors = SpuriousErrors // report all (not just the first 10) errors per file https://codereview.appspot.com/7307085/diff/10001/src/pkg/go/parser/interface... src/pkg/go/parser/interface.go:91: remove this empty line https://codereview.appspot.com/7307085/diff/10001/src/pkg/go/parser/interface... src/pkg/go/parser/interface.go:97: // sort errors hm, i'm not sure this is quite correct You are right that AllErrors interferes in unhappy ways with SpuriousErrors. Unfortunately we cannot change the API (say, remove SpuriousErrors). Basically, in case of a bailout we want the errors sorted. We could do RemoveMultiples, but then we may end up with even less then 10 reported errors, which seems a bit odd. However, we do know that a bailout can only happen if AllErrors is not set. That all said, I've changed my mind a bit: I think the AllErrors flag as proposed so far makes things more complicated. Basically we want as default behavior max. 10 errors reported with the flags people have been setting so far. How about this: We "repurpose" the SpuriousErrors flag to also mean AllErrors (we still have AllErrors because it's the right name, but it will have the same value as SpuriousErrors so far). It's never set, so the default will be max. 10 errors (even if they were "spurious" by the old definition). If it is set, all errors (incl. spurious errors are shown). That doesn't remove anything from the API, we get the new behavior (max 10 errors by default), and when AllErrors (or SpuriousErrors) is set, we get all errors. We lose the RemoveMultiples() call, but it might be fine. If it's a problem we can revisit. So this code can be simplified to: if e := recover(); ... { ... } // set result values if f == nil { ... (see my comment below) } p.errors.Sort() err = p.errors.Err() https://codereview.appspot.com/7307085/diff/10001/src/pkg/go/parser/interface... src/pkg/go/parser/interface.go:110: if f == nil { This check now also needs to be done in the deferred function. Otherwise, if there's a bailout, f will be nil and the resulting f is nil, contrary to what the API description says. https://codereview.appspot.com/7307085/diff/10001/src/pkg/go/parser/interface... src/pkg/go/parser/interface.go:120: return f, err just: return no need to provide values; they will be set in the deferred function https://codereview.appspot.com/7307085/diff/10001/src/pkg/go/parser/parser.go File src/pkg/go/parser/parser.go (right): https://codereview.appspot.com/7307085/diff/10001/src/pkg/go/parser/parser.go... src/pkg/go/parser/parser.go:347: if p.mode&AllErrors == 0 && p.errors.Len() > 10 { s/>/>=/ You want to bail out if this is called and we have 10 errors already (i.e., this is the 11th time, error is called). https://codereview.appspot.com/7307085/diff/10001/src/pkg/go/parser/parser.go... src/pkg/go/parser/parser.go:347: if p.mode&AllErrors == 0 && p.errors.Len() > 10 { s/AllErrors/SpuriousErrors/ (see my larger comment in ParseFile)
Hello golang-dev@googlegroups.com, gri@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
https://codereview.appspot.com/7307085/diff/10001/src/pkg/go/parser/interface.go File src/pkg/go/parser/interface.go (right): https://codereview.appspot.com/7307085/diff/10001/src/pkg/go/parser/interface... src/pkg/go/parser/interface.go:60: SpuriousErrors // report all (not just the first) errors per line when AllErrors set On 2013年02月14日 06:55:56, gri wrote: > see the comment below for an explanation, but I suggest to change this to: > > SpuriousErrors // same as AllErrors, for > backward-compatibility > AllErrors = SpuriousErrors // report all (not just the first 10) errors per file Done. https://codereview.appspot.com/7307085/diff/10001/src/pkg/go/parser/interface... src/pkg/go/parser/interface.go:91: On 2013年02月14日 06:55:56, gri wrote: > remove this empty line Done. https://codereview.appspot.com/7307085/diff/10001/src/pkg/go/parser/interface... src/pkg/go/parser/interface.go:97: // sort errors On 2013年02月14日 06:55:56, gri wrote: > hm, i'm not sure this is quite correct > > You are right that AllErrors interferes in unhappy ways with SpuriousErrors. > Unfortunately we cannot change the API (say, remove SpuriousErrors). Basically, > in case of a bailout we want the errors sorted. We could do RemoveMultiples, but > then we may end up with even less then 10 reported errors, which seems a bit > odd. However, we do know that a bailout can only happen if AllErrors is not set. > > That all said, I've changed my mind a bit: I think the AllErrors flag as > proposed so far makes things more complicated. Basically we want as default > behavior max. 10 errors reported with the flags people have been setting so far. > > How about this: We "repurpose" the SpuriousErrors flag to also mean AllErrors > (we still have AllErrors because it's the right name, but it will have the same > value as SpuriousErrors so far). It's never set, so the default will be max. 10 > errors (even if they were "spurious" by the old definition). If it is set, all > errors (incl. spurious errors are shown). That doesn't remove anything from the > API, we get the new behavior (max 10 errors by default), and when AllErrors (or > SpuriousErrors) is set, we get all errors. We lose the RemoveMultiples() call, > but it might be fine. If it's a problem we can revisit. > > So this code can be simplified to: > > if e := recover(); ... { > ... > } > > // set result values > if f == nil { > ... (see my comment below) > } > > p.errors.Sort() > err = p.errors.Err() Done. https://codereview.appspot.com/7307085/diff/10001/src/pkg/go/parser/interface... src/pkg/go/parser/interface.go:110: if f == nil { On 2013年02月14日 06:55:56, gri wrote: > This check now also needs to be done in the deferred function. Otherwise, if > there's a bailout, f will be nil and the resulting f is nil, contrary to what > the API description says. Done. https://codereview.appspot.com/7307085/diff/10001/src/pkg/go/parser/interface... src/pkg/go/parser/interface.go:120: return f, err On 2013年02月14日 06:55:56, gri wrote: > just: > > return > > no need to provide values; they will be set in the deferred function Done. https://codereview.appspot.com/7307085/diff/10001/src/pkg/go/parser/parser.go File src/pkg/go/parser/parser.go (right): https://codereview.appspot.com/7307085/diff/10001/src/pkg/go/parser/parser.go... src/pkg/go/parser/parser.go:347: if p.mode&AllErrors == 0 && p.errors.Len() > 10 { On 2013年02月14日 06:55:56, gri wrote: > s/AllErrors/SpuriousErrors/ > > (see my larger comment in ParseFile) Done. https://codereview.appspot.com/7307085/diff/10001/src/pkg/go/parser/parser.go... src/pkg/go/parser/parser.go:347: if p.mode&AllErrors == 0 && p.errors.Len() > 10 { On 2013年02月14日 06:55:56, gri wrote: > s/>/>=/ > > You want to bail out if this is called and we have 10 errors already (i.e., this > is the 11th time, error is called). Done.
LGTM thanks!
*** Submitted as https://code.google.com/p/go/source/detail?r=e5cc27ee25e3 *** go/parser: stop ParseFile after ten errors. There wil be a panic if more than ten errors are encountered. ParseFile will recover and return the ErrorList. Fixes issue 3943. R=golang-dev, gri CC=golang-dev https://codereview.appspot.com/7307085 Committer: Robert Griesemer <gri@golang.org>