Skip to content

Partial flush and index locations#326

Open
myrrhlin wants to merge 3 commits intoPerl-Critic:masterfrom
myrrhlin:flush-partial
Open

Partial flush and index locations#326
myrrhlin wants to merge 3 commits intoPerl-Critic:masterfrom
myrrhlin:flush-partial

Conversation

@myrrhlin
Copy link

@myrrhlin myrrhlin commented Mar 3, 2026

Resolves issue #325

PPI:Document->index_locations contains code to perform partial reindexing -- that is, it only recalculates locations for the first element seen to be missing one, and all subsequent elements. Existing locations at the beginning of the document are considered still good. This is a performance optimization. This would be really handy, for example, if one were to use e.g. a replace method to change part of an existing document with new source text, which can make the cached locations for all subsequent elements invalid. This behavior is undocumented in the POD.

PPI:Document->flush_locations can be used to flush all locations from the document; it leans on the private method PPI:Element->_flush_locations to do the job. However, the latter contains code to do "partial flushing" of locations -- that is, flush the cached location from the invoking instance and all subsequent elements, but leaving prior element locations intact. This would be really handy to ensure that a later Document->index_locations call only does a partial reindex. This behavior is also undocumented in the POD.

Despite the existing code, neither of these intended behaviors work, because each has a minor bug. There are (obviously) no tests of these desired behaviors.

@myrrhlin
Copy link
Author

myrrhlin commented Mar 3, 2026

(flush-partial)$ prove -r t/
t/07_token.t ..................... ok
t/08_regression.t ................ ok
t/06_round_trip.t ................ ok
t/19_selftesting.t ............... ok
t/ppi_statement_package.t ........ ok
t/ppi_statement_include.t ........ ok
t/25_increment.t ................. ok
t/21_exhaustive.t ................ ok
t/ppi_statement_sub.t ............ ok
t/04_element.t ................... ok
t/ppi_token_unknown.t ............ ok
t/ppi_token_operator.t ........... ok
t/ppi_token_attribute.t .......... ok
t/ppi_token_quotelike_words.t .... ok
t/ppi_token_number_version.t ..... ok
t/ppi_token_word.t ............... ok
t/ppi_token_prototype.t .......... ok
t/12_location.t .................. ok
t/ppi_token_heredoc.t ............ ok
t/05_lexer.t ..................... ok
t/feature_tracking.t ............. ok
t/26_bom.t ....................... ok
t/ppi_token_symbol.t ............. ok
t/signature_details.t ............ ok
t/ppi_statement_compound.t ....... ok
t/29_logical_filename.t .......... ok
t/01_compile.t ................... ok
t/ppi_token_quote_interpolate.t .. ok
t/ppi_element_flush.t ............ ok
t/18_cache.t ..................... ok
t/15_transform.t ................. ok
t/ppi_element_replace.t .......... ok
t/ppi_token_quotelike_regexp.t ... ok
t/ppi_statement_scheduled.t ...... ok
t/ppi_token__quoteengine_full.t .. ok
t/ppi_token_quote_single.t ....... ok
t/ppi_token_magic.t .............. ok
t/14_charsets.t .................. ok
t/ppi_lexer.t .................... ok
t/ppi_statement_variable.t ....... ok
t/24_v6.t ........................ ok
t/27_complete.t .................. ok
t/ppi_token_quote.t .............. ok
t/marpa.t ........................ ok
t/28_foreach_qw.t ................ ok
t/ppi_token_quote_double.t ....... ok
t/11_util.t ...................... ok
t/22_readonly.t .................. ok
t/ppi_token_structure.t .......... ok
t/ppi_node.t ..................... ok
t/ppi_token_quote_literal.t ...... ok
t/09_normal.t .................... ok
t/16_xml.t ....................... ok
t/ppi_token.t .................... ok
t/ppi_element.t .................. ok
t/17_storable.t .................. ok
t/03_document.t .................. ok
t/ppi_token_dashedword.t ......... ok
t/ppi_token_regexp.t ............. ok
t/ppi_token_pod.t ................ ok
t/signatures.t ................... ok
t/ppi_normal.t ................... ok
t/23_file.t ...................... ok
t/10_statement.t ................. ok
t/interactive.t .................. ok
t/ppi_statement.t ................ ok
t/13_data.t ...................... ok
t/ppi_token_whitespace.t ......... ok
All tests successful.
Files=68, Tests=52855, 44 wallclock secs ( 4.57 usr  0.66 sys + 39.99 cusr  2.39 csys = 47.61 CPU)
Result: PASS

myrrhlin added 3 commits March 3, 2026 04:08
these tests document desired behavior with 'partial' _flush_locations (for
when the developer knows the place where the cached locations are bad),
and index_locations (when -some- locations, presumably good, already
exist).

these behaviors are not documented in POD, only in source code.  but
they don't currently work!
When it exists, the value of $token->{_location} is an arrayref,
so line 804 elicits a fatal error:

  Not a SCALAR reference

This is currently dead code -- no existing execution path in PPI actually
triggers this block.  But we can make it work as intended.

This resolves part of issue Perl-Critic#325
As described in issue Perl-Critic#325, this method leaves alone existing locations, up
to the first element that is missing one, then it recalculates locations
for the rest of the document.  This is a performance optimization.

However, the _add_location method needs to be given the previous (last good)
location (passed as the first argument, $start) to calculate the next one.
In the partial reindexing case, where a document has its first element with
a location, then _add_location is passed undef for $start, which elicits
the warnings:

 Use of uninitialized value in numeric eq (==) at ../PPI/Document.pm line 749.
 Use of uninitialized value in addition (+) at ../PPI/Document.pm line 794.
 Use of uninitialized value in addition (+) at ../PPI/Document.pm line 723.

..and the calculated locations are all wrong.

This commit fixes the partial indexing.

A side effect of this solution is that all the recalculated element
locations retain the original filename, even if they didn't originate in
that file.
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.

1 participant