Fix panic that can occur when interpreting options in lenient mode #331
Conversation
…lso improve error reporting in lenient mode
options/options.go
Outdated
| reporter internal.ErrorHandler | ||
| lenient bool | ||
|
|
||
| // lenienceEnabled is set to true when errors reported to reporter | ||
| // should be lenient | ||
| lenienceEnabled bool | ||
| lenientErrReported bool |
There was a problem hiding this comment.
I would have liked to have formatted this with less churn in the diff. But then I got govet lint warnings based on the struct size being inflated due to bool fields in the middle of the struct. So I moved them all to the end to get rid of that lint issue.
| pathBuffer []int32 | ||
|
|
||
| reporter internal.ErrorHandler | ||
| lenient bool |
There was a problem hiding this comment.
I'm happy to rename some of these fields if you think that would help readability. The existing lenient flag means the interpreter will tolerate certain kinds of errors. The new flag below is set to true around the parts of the code that can generate those kinds of errors. (So errors can still occur in lenient mode -- it's just certain kind of errors that lenient mode tolerates.)
| interp.enableLenience(true) | ||
| err := interp.interpretFieldPseudoOptions(fqn, fld, opts) | ||
| interp.enableLenience(false) |
There was a problem hiding this comment.
This is how we indicate the operations in which errors are tolerated.
options/options.go
Outdated
| if err != nil && !interp.lenient { | ||
| if index, err := internal.FindOption(interp.file, interp.reporter, scope, uo, "json_name"); err != nil { | ||
| return err | ||
| } | ||
| if index >= 0 { | ||
| } else if index >= 0 { |
There was a problem hiding this comment.
This simple reformatting is for symmetry/consistency with the block below.
| @@ -389,7 +406,7 @@ func (interp *interpreter) interpretFieldPseudoOptions(fqn string, fld *descript | |||
| } | |||
|
|
|||
| // and process default pseudo-option | |||
| if index, err := interp.processDefaultOption(scope, fqn, fld, uo); err != nil && !interp.lenient { | |||
| if index, err := interp.processDefaultOption(scope, fqn, fld, uo); err != nil { | |||
There was a problem hiding this comment.
We no longer check interp.lenient here (or above) because when lenience is enabled, the returned err will now always be nil.
| if interp.lenientErrReported { | ||
| remain = append(remain, uo) | ||
| continue | ||
| } |
There was a problem hiding this comment.
When lenience is enabled, the above error is always nil, but we still need to know if any error was reported, so we know whether we need to keep this option in the uninterpreted set.
| if srcInfo == nil { | ||
| // can happen if we are lenient when interpreting -- this node | ||
| // could not be interpreted and thus has no source info; skip | ||
| return true | ||
| } |
There was a problem hiding this comment.
THIS is the actual fix for the panic. In the cases where srcInfo was nil here, it would trigger a nil-dereference panic on the next line, to look at the Path field. Oops.
| @@ -244,6 +254,25 @@ func TestOptionsInUnlinkedFiles(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func TestOptionsInUnlinkedFileInvalid(t *testing.T) { | |||
There was a problem hiding this comment.
This is a repro test case. It differs from the ones above because it actually is expected to return an error, even in lenient mode (since the error is not related to an unresolved option or type error). This would previously trigger the panic. And then, after the panic was fixed, it would return the wrong error without the other changes in this branch.
| var ewp ErrorWithPos | ||
| if errors.As(err, &ewp) { | ||
| // replace existing position with given one | ||
| return &errorWithSpan{SourceSpan: span, underlying: ewp.Unwrap()} | ||
| } | ||
| return &errorWithSpan{SourceSpan: span, underlying: err} |
There was a problem hiding this comment.
I made some minor changes/improvements throughout this package, for better consistency.
The panic fix is tiny. But this PR is not because other changes/improvements were called for: most of the code changes are to improve error handling when in lenient mode. After I fixed the panic, the test case was failing in a different way, due to an issue with how errors (even in lenient mode) were still being passed to an error reporter and causing the stage to fail.
This issue was introduced in #279. That PR moved things around, pushing the responsibility of calling
interp.reporter.HandleErrordown, so that interpreting a single option could potentially report multiple errors (instead of failing fast and only reporting the first).But when in lenient mode, we don't actually want to send those errors to the reporter: sending the error to the reporter meant the error would get memoized and then returned in subsequent handle calls, which could cause the process to fail when it should be lenient and also can cause it to report the wrong error in lenient mode. So now we demarcate the parts of the process where errors are tolerated (i.e. where lenience mode is actually activated) with a new field on the interpreter that is examined when errors are reported.
This will resolve jhump/protoreflect#620.