Fix panic when inferring imports #575
Conversation
| // TODO: if this re-writes one of the names in filenames, lookups below will break | ||
| results = fixupFilenames(results) |
There was a problem hiding this comment.
Without addressing this TODO, there were other latent bugs lurking in here. Addressing the TODO was required to get new test passing.
| } | ||
| return results, rep.Error() | ||
| } | ||
|
|
||
| func parseToProtoRecursive(res protocompile.Resolver, filename string, rep *reporter.Handler, srcPosAddr *SourcePos, results map[string]parser.Result) { | ||
| func parseToProtoRecursive(res protocompile.Resolver, filename string, rep *reporter.Handler, srcPosAddr *SourcePos, results map[string]parser.Result) error { |
There was a problem hiding this comment.
Error handling is now more explicit, instead of relying on rep.ReporterError(). It is possible that a reporter chose to not return an error, so there are cases where this could return nil, and the logic here could try to proceed, even when it really shouldn't.
| imp, ok := decl.(*ast2.ImportNode) | ||
| if !ok { | ||
| continue | ||
| if astRoot != nil { |
There was a problem hiding this comment.
This was the crux of fixing the panic: we may have a parseResult, but not an astRoot, like if the file came from a descriptor, not source.
| delete(candidatesAvailable, best) | ||
|
|
||
| // If other candidates are actually references to the same file, remove them. |
There was a problem hiding this comment.
This new logic, to remove redundant files -- the case when the same file may be inadvertently referenced via multiple relative paths -- was necessary to get the new test to pass and also makes this functionality much more useful.
Without it, this logic might correctly "fix up" filenames, but there's still be extra files in the operation which then cause issues since they define the same elements. So the linker would fail with errors like foo.proto: symbol already defined in a/b/c/foo.proto. This is now fixed.
| @@ -422,3 +422,106 @@ message Foo { | |||
| comment := fds[0].GetMessageTypes()[0].GetFields()[0].GetSourceInfo().GetLeadingComments() | |||
| testutil.Eq(t, " leading comments\n", comment) | |||
| } | |||
|
|
|||
| func TestParseInferImportPaths_SimpleNoOp(t *testing.T) { | |||
There was a problem hiding this comment.
This simple case reproduces the panic.
| testutil.Eq(t, 1, len(fds)) | ||
| } | ||
|
|
||
| func TestParseInferImportPaths_FixesNestedPaths(t *testing.T) { |
There was a problem hiding this comment.
This more involved test actually verifies that InferImportPaths is working and doing what it's supposed to do.
This resolves the panic identified in #572.
This has been broken ever since 1.15.0-rc1, sadly. The only coverage in here was a call to
fixupFilenamesin an existing test. However, nothing was actually testing the parser withInferImportPathsset to true, so this wasn't caught during the "great overhaul" of v1.15.0.The panic was caused by the use of
InferImportPaths: truealong with an imported file whose AST was not available -- such as a descriptor for a well-known/standard import. The code will now fallback to examining the file descriptor when no AST is available (and try to get source position information from the descriptor'sSourceLocations, if available).This adds a simple repro test, which now passes. But this furthermore fixes the
InferImportPathsfunctionality. There was previously no test case, and adding a test case identified some other issues (that likely have been present from the beginning 😞). So not only does this fix the panic but it also provides considerable improvement to theInferImportPathsfunctionality.