Fix: careful with linenumber removal in order to improve code coverage - #281
Merged
Conversation
Contributor
|
Your PR no longer requires formatting changes. Thank you for your contribution! |
Jutho
reviewed
Jun 15, 2026
Member
|
Other than my comment, I think this looks reasonable. I indeed removed line numbers to make the macroexpanded code more readable. |
Member
Author
|
If the line number removal is just for the macro-expanded code, it might be reasonable to just disable it altogether? That also avoids the issues with Note that I still need to check that this actually fixes the codecov issue, so this PR is not ready |
Member
Author
|
Update here: I reworked this slightly, basically just removing only the line number nodes that were inserted by us, while keeping the ones that were present in the original expression. From what I understand, this should be enough to attribute all of the code to the originating expressions, hopefully both making error messages as well as code coverage behave now. |
Codecov Report✅ All modified and coverable lines are covered by tests.
🚀 New features to boost your workflow:
|
lkdvos
enabled auto-merge (squash)
July 30, 2026 21:08
Follow-up on the previous commit, addressing the two open questions in the PR discussion. Instead of recognizing parser-generated `LineNumberNode`s by string-prefix matching against `TensorOperations/src`, snapshot the set of files referenced by the `LineNumberNode`s of the expression handed to the parser -- taken before any preprocessor runs, so those are by construction exactly the user's line numbers -- and drop everything else. This also explains why the step cannot be a stateless entry in `parser.postprocessors`: it needs state captured before the preprocessors. The path-prefix version missed two cases: - `ext/`, which lies outside `src/`, so no extension was covered; - `@planar`/`@plansor`, where TensorKit's own pre/postprocessors synthesize `LineNumberNode`s pointing into `TensorKit/src/planar/`. Verified that those are now stripped as well, so `@planar` and `@plansor` are fixed with no TensorKit-side change. Note that both halves of this are needed, i.e. the removal is not merely cosmetic: a synthesized `LineNumberNode` in block-statement position re-attributes every statement that follows it to a line in TensorOperations' own source, which steals the attribution from the user's line. Conversely, `LineNumberNode`s outside block-statement position are left alone, since they can be structurally required -- the `costcheck = :warn` path emits a `@warn` macrocall whose second argument is one. Verified end to end with `--code-coverage`: on PEPSKit's `src/algorithms/contractions/ctmrg/enlarge_corner.jl` (the file from #280), the four contraction statements of `enlarge_northwest_corner` go from no coverage information at all to a hit count, taking the file from 12/72 to 28/88 reported lines. A deliberately-never-called function stays at 0 in both cases, confirming the counts reflect real execution. Also checked on the realistic CI path, i.e. `Pkg.test(coverage = true)` against a precompiled package rather than an `include`d file. While here: - delete `removelinenumbernode`, which is unexported and now unused; - drop the redundant removal in `insertcontractiontrees!`, since its output passes through the parser's final pass anyway; - update `docs/src/man/implementation.md`, which still listed `removelinenumbernode` as an enabled default postprocessor; - tighten the tests to assert the exact set of surviving line numbers and that every one of them comes from the test file, which the previous prefix-based assertion could not detect for `ext/`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Jutho
approved these changes
Jul 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a claude-aided attempt of resolving #280.
From what I could tell, the removal of the line numbers is what causes the codecov gaps.
@Jutho I'm not entirely sure if we really need these to begin with, or if this is just a cosmetic thing to make the generated expressions easier to inspect?
In any case, I still have to more carefully investigate if this actually resolves the issue, and additionally if this automatically carries over to
@planarand@plansor.