Skip to content

Fix inconsistency in whether locations are opaque in sexps#1547

Merged
WardBrian merged 3 commits intomasterfrom
fix-opaque-location-sexp
Sep 11, 2025
Merged

Fix inconsistency in whether locations are opaque in sexps#1547
WardBrian merged 3 commits intomasterfrom
fix-opaque-location-sexp

Conversation

@WardBrian
Copy link
Member

This was mostly a simple case of some missing parenthesis changing the meaning of annotations, or annotations being misnamed.

Submission Checklist

  • Run unit tests
  • Documentation
    • If a user-facing facing change was made, the documentation PR is here:
    • OR, no user-facing changes were made

Release notes

Copyright and Licensing

By submitting this pull request, the copyright holder is agreeing to
license the submitted work under the BSD 3-clause license (https://opensource.org/licenses/BSD-3-Clause)

@WardBrian WardBrian requested a review from nhuurre September 11, 2025 13:47
@codecov
Copy link

codecov bot commented Sep 11, 2025

Codecov Report

❌ Patch coverage is 66.66667% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.14%. Comparing base (dee6786) to head (861e74e).
⚠️ Report is 8 commits behind head on master.

Files with missing lines Patch % Lines
src/frontend/Ast.ml 66.66% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1547      +/-   ##
==========================================
+ Coverage   90.09%   90.14%   +0.05%     
==========================================
  Files          64       64              
  Lines        9861     9855       -6     
==========================================
  Hits         8884     8884              
+ Misses        977      971       -6     
Files with missing lines Coverage Δ
src/middle/Program.ml 88.88% <ø> (-0.45%) ⬇️
src/frontend/Ast.ml 76.67% <66.66%> (+2.07%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment on lines 117 to +119
| LTuplePack of
{ lvals: 'l lvalue_pack list
; loc: Location_span.t [@sexp.opaque] [@compare.ignore] }
; loc: (Location_span.t[@sexp.opaque] [@compare.ignore]) }
Copy link
Collaborator

Choose a reason for hiding this comment

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

I didn't find a related test. What does this do exactly?

Copy link
Member Author

Choose a reason for hiding this comment

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

I just added an extra Parse_test -- removing the parenthesis will change the output of this new test to include the location, rather than omit <opaque> as elsewhere.

I couldn't find much about the fact that these annotations for deriving are so sensitive, but it seems like they're ignored if they're not precisely as expected, meaning it's easy to miss

@WardBrian WardBrian merged commit c664a04 into master Sep 11, 2025
3 checks passed
@WardBrian WardBrian deleted the fix-opaque-location-sexp branch September 11, 2025 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants