Skip to content

Conversation

@uzairnawaz
Copy link

Resolves line information issues described by #11203

For functions that don't include explicit return statements in the Go source code, the ssa package inserts return instructions without line information attached, leading to poor error messages.
Ex: -: return with unexpected locks held (locks: &({param:f}.mu) exclusively)

Quick fix is to instead use the line number information of the parent function instead.

@ayushr2
Copy link
Collaborator

ayushr2 commented Dec 9, 2025

Can you squash your commits, we don't have the ability to squash and merge yet.

@uzairnawaz uzairnawaz force-pushed the return-parent-line-numbers branch from 6e7b714 to dc3d29a Compare December 9, 2025 17:47
@uzairnawaz uzairnawaz force-pushed the return-parent-line-numbers branch from dc3d29a to 9a04865 Compare December 9, 2025 17:49
copybara-service bot pushed a commit that referenced this pull request Dec 9, 2025
…struction has no line numbers attached

Resolves line information issues described by #11203

For functions that don't include explicit return statements in the Go source code, the ssa package inserts return instructions without line information attached, leading to poor error messages.
Ex: -: return with unexpected locks held (locks: &({param:f}.mu) exclusively)

Quick fix is to instead use the line number information of the parent function instead.

FUTURE_COPYBARA_INTEGRATE_REVIEW=#12347 from uzairnawaz:return-parent-line-numbers 9a04865
PiperOrigin-RevId: 842286019
@ayushr2
Copy link
Collaborator

ayushr2 commented Dec 10, 2025

@uzairnawaz
Copy link
Author

uzairnawaz commented Dec 11, 2025

It appears that errors that contain no line info are currently getting discarded.
Ex (on the master branch):

$ go build tools/checklocks/cmd/checklocks
$ ./checklocks tools/checklocks/tests/methods.go
-: lock a.mu (&({param:a}.mu)) not held exclusively (locks: no locks held)

But, running the tests with bazel:

$ bazel test //tools/checklocks/test:test_nogo

shows that they all pass. This same bazel test fails on this branch with the same error as above (lock not held exclusively). My guess is that attaching the line numbers causes us to now see all the errors that were previously being discarded.

I can go through and annotate other occurences of this to prevent failures. Do you think this should be a separate PR or include it here?

@ayushr2
Copy link
Collaborator

ayushr2 commented Dec 12, 2025

Thanks for the investigation.

Do you think this should be a separate PR or include it here?

It should be a separate PR.

copybara-service bot pushed a commit that referenced this pull request Dec 14, 2025
The changes in #12347 led to additional errors being reported in existing code. This PR addresses those issues with additional annotations.

FUTURE_COPYBARA_INTEGRATE_REVIEW=#12379 from uzairnawaz:fix-new-checklock-errors 74be08b
PiperOrigin-RevId: 844243954
copybara-service bot pushed a commit that referenced this pull request Dec 16, 2025
The changes in #12347 led to additional errors being reported in existing code. This PR addresses those issues with additional annotations.

FUTURE_COPYBARA_INTEGRATE_REVIEW=#12379 from uzairnawaz:fix-new-checklock-errors 9871b5a
PiperOrigin-RevId: 844243954
copybara-service bot pushed a commit that referenced this pull request Dec 16, 2025
The changes in #12347 led to additional errors being reported in existing code. This PR addresses those issues with additional annotations.

FUTURE_COPYBARA_INTEGRATE_REVIEW=#12379 from uzairnawaz:fix-new-checklock-errors 9871b5a
PiperOrigin-RevId: 844243954
copybara-service bot pushed a commit that referenced this pull request Dec 16, 2025
The changes in #12347 led to additional errors being reported in existing code. This PR addresses those issues with additional annotations.

FUTURE_COPYBARA_INTEGRATE_REVIEW=#12379 from uzairnawaz:fix-new-checklock-errors 9871b5a
PiperOrigin-RevId: 844243954
Copy link

@kakkoyun kakkoyun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

I attempted to fix the same issue here #11741

But this PR is way more elegant to address it.
I'll rebase my PR and check if it is still needed.

kakkoyun added a commit to kakkoyun/checklocks that referenced this pull request Dec 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants