jhump / protoreflect

Reflection (Rich Descriptors) for Go Protocol Buffers
Apache License 2.0
1.35k stars 172 forks source link

Fix panic when inferring imports #575

Closed jhump closed 1 year ago

jhump commented 1 year ago

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 fixupFilenames in an existing test. However, nothing was actually testing the parser with InferImportPaths set to true, so this wasn't caught during the "great overhaul" of v1.15.0.

The panic was caused by the use of InferImportPaths: true along 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's SourceLocations, if available).

This adds a simple repro test, which now passes. But this furthermore fixes the InferImportPaths functionality. 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 the InferImportPaths functionality.