search

jhump / protoreflect

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

Regression upgrading from v1.14.1 to v1.15.4: new mustBeSource constraint/check #591

Closed stapelberg closed 9 months ago

stapelberg commented 9 months ago

We have a usage of protoreflect where the user calls ParseFiles(), but the sources are only available for some of the files. For the remaining files, only the descriptor is available.

This worked with protoreflect v1.14.1, but in v1.15.4, this use-case is running into the new mustBeSource check: https://github.com/jhump/protoreflect/blob/bf8a7d8361451273ce8797d2fe89509b3560c0f6/desc/protoparse/parser.go#L558-L570

Commenting out line 559-561 makes the program work again, but I was wondering if this additional check was introduced intentionally?

Thanks

jhump commented 9 months ago

@stapelberg, why does your program provide that filename to the compiler? If you already have a descriptor, why bother compiling it?

I can relax this, to fix the regression. But this check was added because, TBH, I thought it was expected behavior, not realizing the previous version was lenient about this.

stapelberg commented 9 months ago

@stapelberg, why does your program provide that filename to the compiler? If you already have a descriptor, why bother compiling it?

I’m not sure, I haven’t looked into the affected program at this level of detail.

I can try to find out, but it’ll probably be a while.

jhump commented 9 months ago

@stapelberg, I will go ahead and remove the constraint since it was not present in the earlier version. Sorry about the breakage, and thanks for filing these issues.

jhump commented 9 months ago

@stapelberg, I've now merged fixes for the three regressions you identified (this one included). Two are unreleased. Before I create another patch release, maybe you could try to update to latest HEAD and see if you find any additional issues (which I could also include in the next patch release)?

stapelberg commented 9 months ago

@stapelberg, I've now merged fixes for the three regressions you identified (this one included).

Thank you!

Two are unreleased. Before I create another patch release, maybe you could try to update to latest HEAD and see if you find any additional issues (which I could also include in the next patch release)?

Copying here what I emailed you, for posterity:

As you asked, I have picked up the latest revision from the “main” branch, and that does indeed fix all currently known issues. I’m running another global test run now and will let you know should any additional issues arise, or whether we found any issues after running unit and integration tests…