protoc: fix source code info location for missing label #6436
Conversation
| @@ -2808,6 +2808,34 @@ TEST_F(SourceInfoTest, Fields) { | |||
| EXPECT_TRUE(HasSpan(file_.message_type(0), "name")); | |||
| } | |||
|
|
|||
| TEST_F(SourceInfoTest, Fields) { | |||
There was a problem hiding this comment.
I think the test is failing because this test case has the same name as the previous case.
There was a problem hiding this comment.
Yep. I was having problems getting make check to run locally. I just got it running locally and verified that the test passes. (I also intentionally commented out a line below, hoping it would make the test fail, to ensure that these assertions would catch any location for the label that should not be present.)
Anyhow, it should be good now.
There was a problem hiding this comment.
BTW, where did you see test output with that error? The only things I see in GitHub are the CLA check and a "mergeable" check which was looking at PR labels that I can't set. Or did a third check appear on a prior commit which will eventually show up for the last commit I pushed?
There was a problem hiding this comment.
I think the test results must have disappeared after you pushed a new commit (since they would be invalidated in that case). I just added the tag to kick off a new test run though, so the results should start reappearing.
jhump
force-pushed
the
jh/fix-source-code-info-location-for-missing-label
branch
from
036a8bb
to
76f81ce
Compare
|
@jhump Thanks for fixing this! |
Fixes #6378.
This was always emitting a location for the label, even if no label was present. This resulted in a malformed span -- since the label did not actually exist, the associated span had an end that was before the start.
This patch makes it so that the location is simply omitted if there is no location in source (e.g. optional fields in a file with proto3 syntax).