Jira (PUP-11632) locator.extract_tree_text does returns bad string on simple function call

18 views
Skip to first unread message

Daniel Parks (Jira)

unread,
Sep 26, 2022, 6:02:02 PM9/26/22
to puppe...@googlegroups.com
Daniel Parks created an issue
 
Puppet / Bug PUP-11632
locator.extract_tree_text does returns bad string on simple function call
Issue Type: Bug Bug
Affects Versions: PUP 7.19.0
Assignee: Unassigned
Created: 2022/09/26 3:01 PM
Priority: Normal Normal
Reporter: Daniel Parks

Puppet Version: 7.20.0 (main: 948a4bec67def5244595a6f69976a9a2479f1df1)
OS Name/Version: macOS 12.6.0

ast.locator.extract_tree_text(ast) returns an incorrect string for a simple function call.

  it 'extracts a simple function call' do
    parser = Puppet::Pops::Parser::Parser.new()
    model = parser.parse_string('$param.func()').model
    expect(model.body.locator.extract_tree_text(model.body)).to eq('$param.func()')
  end

This is causing a problem for Puppet Strings. See https://github.com/puppetlabs/puppet-strings/issues/240#issuecomment-1257153890

Desired Behavior:

The above test should succeed.

Actual Behavior:

The above test (I added it to spec/unit/pops/parser/locator_spec.rb) fails with got "$param.func(".

Add Comment Add Comment
 
This message was sent by Atlassian Jira (v8.20.11#820011-sha1:0629dd8)
Atlassian logo

Daniel Parks (Jira)

unread,
Sep 26, 2022, 6:04:02 PM9/26/22
to puppe...@googlegroups.com
Daniel Parks commented on Bug PUP-11632
 
Re: locator.extract_tree_text does returns bad string on simple function call

Also — sorry for bringing it up here, but there doesn’t seem to be a better place — my old Jira acccount (danielparks) seems to be gone. Can it be restored? I had tickets under it.

Daniel Parks (Jira)

unread,
Sep 26, 2022, 7:09:01 PM9/26/22
to puppe...@googlegroups.com
Daniel Parks commented on Bug PUP-11632

Also, you may be pleased to hear that fixing this will allow me to remove Puppet String’s dependency on Puppet::Pops::Adapters::SourcePosAdapter!

Daniel Parks (Jira)

unread,
Sep 26, 2022, 8:45:01 PM9/26/22
to puppe...@googlegroups.com
Daniel Parks commented on Bug PUP-11632

Found another broken case. The default value of:

String $os = $facts['kernel'] ? {
        'Linux'  => 'linux',
        'Darwin' => 'darwin',
        default  => $facts['kernel'],
      }

The output loses the closing brace and the final comma (though that seems reasonable).

I’m adding these cases to my puppet-strings branch: https://github.com/puppetlabs/puppet-strings/compare/main...danielparks:puppet-strings:fix_default_expressions

Christopher Thorn (Jira)

unread,
Sep 27, 2022, 4:18:03 PM9/27/22
to puppe...@googlegroups.com

Daniel Parks I asked our help desk about restoring your old Jira account.

Henrik Lindberg (Jira)

unread,
Oct 2, 2022, 5:10:03 PM10/2/22
to puppe...@googlegroups.com

The most probable cause of these kinds of problems are mistakes in the grammar itself where the parsing logic assigns a location (position and length) to produced ast nodes using a call to loc.  For example like this lines 229 to 230 

bracketed_expression
  : expression LBRACK access_args endcomma RBRACK =LBRACK { result = val[0].access(val[2]); loc result, val[0], val[4] }

The rules in the grammar should allow location information to bubble upwards, and when doing so, it must return a result with a location based on the correct reduced tokens.

The grammar source I references above for expr[expr] the correct tokens (val[0] and val[4]) are used.

For the selector expression however found [here|http://example.com] it seems to be wrong as it accesses val[0] and val[1], which means that the ast node for a selector entry will stop at the =>. Here is the source:

selector_entry
  : expression FARROW expression { result = Factory.MAP(val[0], val[2]) ; loc result, val[1] }

Unfortunately location information is somewhat different throughout the grammar, for some an astnode will have the position of the operator and the operators length, and others include the location of length of the entire expression. That is naturally where the method extract_tree_text comes in as it should find the earliest source position and longest length starting from the ast node it operates on. Since the off by one error is not visible for every node, it most likely not the calculation itself that is wrong, but the information recorded by the parser. The problem could also be in the factory methods that produce the ast nodes.

Fo anyone tackling this, there is an overview of how this works in Puppet Internals - Puppet LanguageDevelopment

PS, it would have been handy if the puppet parser dump command had an option to output the location information as it would then be trivial to manually verify the location and lengths produced by the parser, now coding is unfortunately required.

Henrik Lindberg (Jira)

unread,
Oct 2, 2022, 5:18:02 PM10/2/22
to puppe...@googlegroups.com

Daniel Parks Are you saying that when using the Puppet::Pops::Adapters::SourcePosAdapter the correct result is obtained, but not when using ast.extract_tree_text ?

Daniel Parks (Jira)

unread,
Oct 2, 2022, 7:40:03 PM10/2/22
to puppe...@googlegroups.com
Daniel Parks commented on Bug PUP-11632

Thanks for the explanation, Henrik Lindberg; it makes sense. I sincerely hope somebody else gets to this first, but I may end up doing it if not. I should get over my irrational fear of debugging parsers at some point, right?

No, Puppet::Pops::Adapters::SourcePosAdapter is only slightly related and doesn’t appear to be source of the issue.

An explanation: before my PR, Puppet Strings used ::Puppet::Pops::Adapters::SourcePosAdapter.adapt(node).extract_text to get the text of an AST node, which internally just calls node.locator.extract_text(node.offset, node.length).

The problem the PR addressed was that when you fed an expression like 1 + 1 to object.locator.extract_text it returned '+' which was definitely not what we wanted. If this bug came into play it wasn’t at all obvious because we were only getting a fragment of the expression.

I changed Puppet Strings to use node.locator.extract_tree_text(node), which actually handles expressions. That’s where this issue popped up.

A pleasant side effect of all this is that the next release of Puppet Strings won’t use Puppet::Pops::Adapters::SourcePosAdapter at all.

(Also, thanks for asking the help desk about my old account, Christopher Thorn!)

Josh Cooper (Jira)

unread,
Oct 4, 2022, 4:36:02 PM10/4/22
to puppe...@googlegroups.com

Josh Cooper (Jira)

unread,
Oct 4, 2022, 4:36:02 PM10/4/22
to puppe...@googlegroups.com
Reply all
Reply to author
Forward
0 new messages