|
|
Descriptiongo/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 #
MessagesTotal messages: 13
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go
Sign in to reply to this message.
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
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, gri@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, gri@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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)
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, gri@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
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.
Sign in to reply to this message.
LGTM thanks!
Sign in to reply to this message.
*** 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>
Sign in to reply to this message.
|