Handle #line directives in get_source_expressions() (#3106)

[youdie006]

Fixes #3106.

Root cause

A #line directive resets the R parser's line numbering. Because of that, getParseData() can report the LINE_DIRECTIVE row with parent == 0, so top_level_expressions() (which is which(pc$parent <= 0L)) counts it as a top-level expression. But xmlparsedata does not always surface the directive as a top-level /exprlist/* node. The two lists then have different lengths, and maybe_append_expression_xml() zips them positionally:

for (i in seq_along(expressions)) {
  expressions[[i]]$xml_parsed_content <- expression_xmls[[i]]  # subscript out of bounds

Repro from the issue:

writeLines(con = tmp <- tempfile(), c("{", '#line 1 "foo.R"', "1+1", "}"))
lintr::get_source_expressions(tmp)
#> Error in expression_xmls[[i]] : subscript out of bounds

I confirmed the desync directly: for that input top_level_expressions() returns 2 rows (expr + LINE_DIRECTIVE) while /exprlist/* returns 1 (expr).

Fix

LINE_DIRECTIVE is a parser directive, not an expression, so it should never be paired with an XML expression node. I exclude it on both sides so the two lists stay aligned in every configuration:

• top_level_expressions(): which(pc$parent <= 0L & pc$token != "LINE_DIRECTIVE")
• the XML selection: /exprlist/*[not(self::LINE_DIRECTIVE)]

Excluding it on only one side would re-introduce a desync in the plain top-level #line case (where xmlparsedata does list the directive), so both are needed. I verified alignment (and that linting still works and reports physical line numbers) across: a #line inside a block, a #line at top level, multiple #line directives, plain comments, and no directive.

This follows your suggestion in the issue to "add it to the set of things that we fix up coming from the R parser." Happy to adjust if you'd prefer neutralizing the directive earlier in the pipeline instead.

Test

Added a regression test in test-get_source_expressions.R. Red-green verified with devtools::test(filter = "get_source_expressions"): before the change the new test errors with subscript out of bounds; after, the whole file passes. lintr::lint() on the changed files is clean; NEWS.md updated.

---

Disclosure: I used AI assistance (Claude) while preparing this change. I root-caused the desync, ran the tests (red-green), and take responsibility for the contribution.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (c3c9805) to head (c3a3dec).

Additional details and impacted files

@@            Coverage Diff            @@
##              main     #3109   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          129       129           
  Lines         7468      7468           
=========================================
  Hits          7468      7468           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[MichaelChirico]

thanks! I don't think this is the right direction to go. I think we should basically try and overcome the reported R bug and treat LINE_DIRECTIVE like a COMMENT whenever we see it. That's ~basically what the parser itself wants to do -- note that getParseData doesn't attempt to interpret the line/file info at all, for example.

One option is to try and get equivalent behavior to my proposed R patch ex-post in get_source_expressions by fixing up parent as-if it were a COMMENT.

But I think probably the best approach for now is wait-and-see -- I'd like to have R core weigh in on the bug first before fixing a path forward for lintr since this case is a little bit ambiguous.

[youdie006]

Thanks for the review and the context, that's helpful. Treating LINE_DIRECTIVE as a COMMENT (fixing up its parent post-hoc so neither top_level_expressions() nor xmlparsedata surfaces it as a top-level expression) is a cleaner and more general framing than excluding it in two places, especially since getParseData() already declines to interpret the line/file info. I'm happy to rework the patch that way.

Agreed on wait-and-see until R core weighs in, given the ambiguity. I'll leave this open and can either update it to the COMMENT-reparent approach or close it in favor of your patch, whichever you prefer once there's a direction.