+1, with one comment below. On Jul 2, 2009, at 11:33 AM, Brice Figureau wrote:
> > Careful inspection of the parser code show that when we > associate a source line number for an AST node, we use the > current line number of the currently lexed token. > In many case, this is correct, but there are some cases where > this is incorrect. > Unfortunately due to how LALR parser works the ast node creation > of a statement can appear _after_ we lexed another token after > the current statement: > > 1. $foo = 1 > 2. > 3. class test > > When the parser asks for the class token, it can reduce the > assignement statement into the AST VarDef node, because no other > grammar rule match. Unfortunately we already lexed the class token > so we affect to the VarDef node the line number 3 instead of 1. > > This is not a real issue for error reporting, but becomes a real > concern when we associate documentation comments to AST node for > puppetdoc. > > The solution is to enhance the tokens lexed and returned to the parser > to carry their declaration line number. > Thus a token value becomes a TokenValue, an object embedding the > lexed value and the line number of the lexed token. > > Next, each time we create an AST node, we use the line number of > the correct token (ie the foo line number in the previous example). > > Signed-off-by: Brice Figureau <[email protected]> > --- > lib/puppet/parser/grammar.ra | 117 ++++++----- > lib/puppet/parser/lexer.rb | 49 ++++- > lib/puppet/parser/parser.rb | 389 +++++++++++++++++++ > +--------------- > lib/puppet/parser/parser_support.rb | 7 +- > spec/unit/parser/lexer.rb | 34 +++- > 5 files changed, 350 insertions(+), 246 deletions(-) > > diff --git a/lib/puppet/parser/grammar.ra b/lib/puppet/parser/ > grammar.ra > index d7828d7..4eab6ab 100644 > --- a/lib/puppet/parser/grammar.ra > +++ b/lib/puppet/parser/grammar.ra > @@ -75,26 +75,30 @@ statement: resource > fstatement: NAME LPAREN funcvalues RPAREN { > args = aryfy(val[2]) > result = ast AST::Function, > - :name => val[0], > + :name => val[0].value, > + :line => val[0].line, I don't think it's worth doing here, but at some point it's probably worth just passing the token to the AST initialization method. It could also include the file name, and it would probably simplify things a bit. I'm not positive that will work for every AST class, but I expect some future refactoring of the AST will make this worthwhile. > > :arguments => args, > :ftype => :statement > } > | NAME LPAREN funcvalues COMMA RPAREN { > args = aryfy(val[2]) > result = ast AST::Function, > - :name => val[0], > + :name => val[0].value, > + :line => val[0].line, > :arguments => args, > :ftype => :statement > } | NAME LPAREN RPAREN { > result = ast AST::Function, > - :name => val[0], > + :name => val[0].value, > + :line => val[0].line, > :arguments => AST::ASTArray.new({}), > :ftype => :statement > } > | NAME funcvalues { > args = aryfy(val[1]) > result = ast AST::Function, > - :name => val[0], > + :name => val[0].value, > + :line => val[0].line, > :arguments => args, > :ftype => :statement > } > @@ -126,7 +130,7 @@ namestring: name > | selector > | quotedtext > | CLASSNAME { > - result = ast AST::Name, :value => val[0] > + result = ast AST::Name, :value => val[0].value > } > > resource: classname LBRACE resourceinstances endsemi RBRACE { > @@ -267,15 +271,16 @@ collstatement: collexpr > result.parens = true > } > > -colljoin: AND | OR > +colljoin: AND { result=val[0].value } > + | OR { result=val[0].value } > > collexpr: colllval ISEQUAL simplervalue { > - result = ast AST::CollExpr, :test1 => val[0], :oper => > val[1], :test2 => val[2] > + result = ast AST::CollExpr, :test1 => val[0], :oper => > val[1].value, :test2 => val[2] > #result = ast AST::CollExpr > #result.push *val > } > | colllval NOTEQUAL simplervalue { > - result = ast AST::CollExpr, :test1 => val[0], :oper => > val[1], :test2 => val[2] > + result = ast AST::CollExpr, :test1 => val[0], :oper => > val[1].value, :test2 => val[2] > #result = ast AST::CollExpr > #result.push *val > } > @@ -305,11 +310,11 @@ undef: UNDEF { > } > > name: NAME { > - result = ast AST::Name, :value => val[0] > + result = ast AST::Name, :value => val[0].value, :line => > val[0].line > } > > type: CLASSREF { > - result = ast AST::Type, :value => val[0] > + result = ast AST::Type, :value => val[0].value, :line => > val[0].line > } > > resourcename: quotedtext > @@ -320,17 +325,17 @@ resourcename: quotedtext > | array > > assignment: VARIABLE EQUALS expression { > - if val[0] =~ /::/ > + if val[0].value =~ /::/ > raise Puppet::ParseError, "Cannot assign to variables in > other namespaces" > end > # this is distinct from referencing a variable > - variable = ast AST::Name, :value => val[0] > - result = ast AST::VarDef, :name => variable, :value => val[2] > + variable = ast AST::Name, :value => val[0].value, :line => > val[0].line > + result = ast AST::VarDef, :name => variable, :value => > val[2], :line => val[0].line > } > > append: VARIABLE APPENDS expression { > - variable = ast AST::Name, :value => val[0] > - result = ast AST::VarDef, :name => variable, :value => > val[2], :append => true > + variable = ast AST::Name, :value => val[0].value, :line => > val[0].line > + result = ast AST::VarDef, :name => variable, :value => > val[2], :append => true, :line => val[0].line > } > > params: # nothing > @@ -348,11 +353,11 @@ params: # nothing > } > > param: NAME FARROW rvalue { > - result = ast AST::ResourceParam, :param => val[0], :value => > val[2] > + result = ast AST::ResourceParam, :param => val[0].value, :line > => val[0].line, :value => val[2] > } > > addparam: NAME PARROW rvalue { > - result = ast AST::ResourceParam, :param => val[0], :value => > val[2], > + result = ast AST::ResourceParam, :param => val[0].value, :line > => val[0].line, :value => val[2], > :add => true > } > > @@ -404,29 +409,29 @@ rvalue: quotedtext > funcrvalue: NAME LPAREN funcvalues RPAREN { > args = aryfy(val[2]) > result = ast AST::Function, > - :name => val[0], > + :name => val[0].value, :line => val[0].line, > :arguments => args, > :ftype => :rvalue > } | NAME LPAREN RPAREN { > result = ast AST::Function, > - :name => val[0], > + :name => val[0].value, :line => val[0].line, > :arguments => AST::ASTArray.new({}), > :ftype => :rvalue > } > > quotedtext: DQTEXT { > - result = ast AST::String, :value => val[0] > + result = ast AST::String, :value => val[0].value, :line => > val[0].line > } | SQTEXT { > - result = ast AST::FlatString, :value => val[0] > + result = ast AST::FlatString, :value => val[0].value, :line => > val[0].line > } > > boolean: BOOLEAN { > - result = ast AST::Boolean, :value => val[0] > + result = ast AST::Boolean, :value => val[0].value, :line => > val[0].line > } > > resourceref: NAME LBRACK rvalues RBRACK { > Puppet.warning addcontext("Deprecation notice: Resource > references should now be capitalized") > - result = ast AST::ResourceReference, :type => val[0], :title => > val[2] > + result = ast AST::ResourceReference, :type => > val[0].value, :line => val[0].line, :title => val[2] > } | classref LBRACK rvalues RBRACK { > result = ast AST::ResourceReference, :type => val[0], :title => > val[2] > } > @@ -482,52 +487,52 @@ else: # nothing > > expression: rvalue > | expression PLUS expression { > - result = ast AST::ArithmeticOperator, :operator => > val[1], :lval => val[0], :rval => val[2] > + result = ast AST::ArithmeticOperator, :operator => > val[1].value, :lval => val[0], :rval => val[2] > } > | expression MINUS expression { > - result = ast AST::ArithmeticOperator, :operator => > val[1], :lval => val[0], :rval => val[2] > + result = ast AST::ArithmeticOperator, :operator => > val[1].value, :lval => val[0], :rval => val[2] > } > | expression DIV expression { > - result = ast AST::ArithmeticOperator, :operator => > val[1], :lval => val[0], :rval => val[2] > + result = ast AST::ArithmeticOperator, :operator => > val[1].value, :lval => val[0], :rval => val[2] > } > | expression TIMES expression { > - result = ast AST::ArithmeticOperator, :operator => > val[1], :lval => val[0], :rval => val[2] > + result = ast AST::ArithmeticOperator, :operator => > val[1].value, :lval => val[0], :rval => val[2] > } > | expression LSHIFT expression { > - result = ast AST::ArithmeticOperator, :operator => > val[1], :lval => val[0], :rval => val[2] > + result = ast AST::ArithmeticOperator, :operator => > val[1].value, :lval => val[0], :rval => val[2] > } > | expression RSHIFT expression { > - result = ast AST::ArithmeticOperator, :operator => > val[1], :lval => val[0], :rval => val[2] > + result = ast AST::ArithmeticOperator, :operator => > val[1].value, :lval => val[0], :rval => val[2] > } > | MINUS expression =UMINUS { > result = ast AST::Minus, :value => val[1] > } > | expression NOTEQUAL expression { > - result = ast AST::ComparisonOperator, :operator => > val[1], :lval => val[0], :rval => val[2] > + result = ast AST::ComparisonOperator, :operator => > val[1].value, :lval => val[0], :rval => val[2] > } > | expression ISEQUAL expression { > - result = ast AST::ComparisonOperator, :operator => > val[1], :lval => val[0], :rval => val[2] > + result = ast AST::ComparisonOperator, :operator => > val[1].value, :lval => val[0], :rval => val[2] > } > | expression GREATERTHAN expression { > - result = ast AST::ComparisonOperator, :operator => > val[1], :lval => val[0], :rval => val[2] > + result = ast AST::ComparisonOperator, :operator => > val[1].value, :lval => val[0], :rval => val[2] > } > | expression GREATEREQUAL expression { > - result = ast AST::ComparisonOperator, :operator => > val[1], :lval => val[0], :rval => val[2] > + result = ast AST::ComparisonOperator, :operator => > val[1].value, :lval => val[0], :rval => val[2] > } > | expression LESSTHAN expression { > - result = ast AST::ComparisonOperator, :operator => > val[1], :lval => val[0], :rval => val[2] > + result = ast AST::ComparisonOperator, :operator => > val[1].value, :lval => val[0], :rval => val[2] > } > | expression LESSEQUAL expression { > - result = ast AST::ComparisonOperator, :operator => > val[1], :lval => val[0], :rval => val[2] > + result = ast AST::ComparisonOperator, :operator => > val[1].value, :lval => val[0], :rval => val[2] > } > | NOT expression { > result = ast AST::Not, :value => val[1] > } > | expression AND expression { > - result = ast AST::BooleanOperator, :operator => val[1], :lval > => val[0], :rval => val[2] > + result = ast AST::BooleanOperator, :operator => > val[1].value, :lval => val[0], :rval => val[2] > } > | expression OR expression { > - result = ast AST::BooleanOperator, :operator => val[1], :lval > => val[0], :rval => val[2] > + result = ast AST::BooleanOperator, :operator => > val[1].value, :lval => val[0], :rval => val[2] > } > | LPAREN expression RPAREN { > result = val[1] > @@ -605,7 +610,7 @@ selectlhand: name > | boolean > | undef > | DEFAULT { > - result = ast AST::Default, :value => val[0] > + result = ast AST::Default, :value => val[0].value, :line => > val[0].line > } > > # These are only used for importing, and we don't interpolate there. > @@ -626,14 +631,14 @@ import: IMPORT qtexts { > #definition: DEFINE NAME argumentlist parent LBRACE statements > RBRACE { > definition: DEFINE classname argumentlist LBRACE statements RBRACE { > @lexer.commentpop > - newdefine classname(val[1]), :arguments => val[2], :code => > val[4] > + newdefine classname(val[1]), :arguments => val[2], :code => > val[4], :line => val[0].line > @lexer.indefine = false > result = nil > > #} | DEFINE NAME argumentlist parent LBRACE RBRACE { > } | DEFINE classname argumentlist LBRACE RBRACE { > @lexer.commentpop > - newdefine classname(val[1]), :arguments => val[2] > + newdefine classname(val[1]), :arguments => val[2], :line => > val[0].line > @lexer.indefine = false > result = nil > } > @@ -643,30 +648,30 @@ hostclass: CLASS classname classparent LBRACE > statements RBRACE { > @lexer.commentpop > # Our class gets defined in the parent namespace, not our own. > @lexer.namepop > - newclass classname(val[1]), :code => val[4], :parent => val[2] > + newclass classname(val[1]), :code => val[4], :parent => > val[2], :line => val[0].line > result = nil > } | CLASS classname classparent LBRACE RBRACE { > @lexer.commentpop > # Our class gets defined in the parent namespace, not our own. > @lexer.namepop > - newclass classname(val[1]), :parent => val[2] > + newclass classname(val[1]), :parent => val[2], :line => > val[0].line > result = nil > } > > nodedef: NODE hostnames nodeparent LBRACE statements RBRACE { > @lexer.commentpop > - newnode val[1], :parent => val[2], :code => val[4] > + newnode val[1], :parent => val[2], :code => val[4], :line => > val[0].line > result = nil > } | NODE hostnames nodeparent LBRACE RBRACE { > @lexer.commentpop > - newnode val[1], :parent => val[2] > + newnode val[1], :parent => val[2], :line => val[0].line > result = nil > } > > -classref: CLASSREF > +classref: CLASSREF { result = val[0].value } > > -classname: NAME > - | CLASSNAME > +classname: NAME { result = val[0].value } > + | CLASSNAME { result = val[0].value } > > # Multiple hostnames, as used for node names. These are all literal > # strings, not AST objects. > @@ -677,10 +682,10 @@ hostnames: hostname > result << val[2] > } > > -hostname: NAME > - | SQTEXT > - | DQTEXT > - | DEFAULT > +hostname: NAME { result = val[0].value } > + | SQTEXT { result = val[0].value } > + | DQTEXT { result = val[0].value } > + | DEFAULT { result = val[0].value } > > nil: { > result = nil > @@ -708,15 +713,15 @@ arguments: argument > > argument: NAME EQUALS rvalue { > Puppet.warning addcontext("Deprecation notice: must now include > '$' in prototype") > - result = [val[0], val[2]] > + result = [val[0].value, val[2]] > } > | NAME { > Puppet.warning addcontext("Deprecation notice: must now include > '$' in prototype") > - result = [val[0]] > + result = [val[0].value] > } | VARIABLE EQUALS rvalue { > - result = [val[0], val[2]] > + result = [val[0].value, val[2]] > } | VARIABLE { > - result = [val[0]] > + result = [val[0].value] > } > > nodeparent: nil > @@ -732,7 +737,7 @@ classparent: nil > classnameordefault: classname | DEFAULT > > variable: VARIABLE { > - result = ast AST::Variable, :value => val[0] > + result = ast AST::Variable, :value => val[0].value, :line => > val[0].line > } > > array: LBRACK rvalues RBRACK { > diff --git a/lib/puppet/parser/lexer.rb b/lib/puppet/parser/lexer.rb > index 6884e68..28c8aca 100644 > --- a/lib/puppet/parser/lexer.rb > +++ b/lib/puppet/parser/lexer.rb > @@ -98,6 +98,37 @@ class Puppet::Parser::Lexer > end > end > > + class TokenValue > + attr_reader :token, :value, :line > + > + def initialize(token, value, line) > + @token = token > + @value = value > + @line = line > + end > + > + def [](value) > + case value > + when :value > + self.value > + when :line > + self.line > + end > + end > + > + def name > + @token.name > + end > + > + def to_s > + @token.name > + end > + > + def inspect > + "#<TOKEN:#{na...@#{line} value=\"#...@value}\">" > + end > + end > + > TOKENS = TokenList.new > TOKENS.add_tokens( > '[' => :LBRACK, > @@ -353,7 +384,7 @@ class Puppet::Parser::Lexer > > return if token.skip > > - return token, value > + return TokenValue.new(token, value, @line) > end > > # Go up one in the namespace. > @@ -415,24 +446,26 @@ class Puppet::Parser::Lexer > @last_return = false > end > > - final_token, value = munge_token(matched_token, value) > + token_value = munge_token(matched_token, value) > > - unless final_token > + unless token_value > skip() > next > end > > - if match = @@pairs[value] and final_token.name ! > = :DQUOTE and final_token.name != :SQUOTE > + value = token_value.value > + > + if match = @@pairs[value] and token_value.name ! > = :DQUOTE and token_value.name != :SQUOTE > @expected << match > - elsif exp = @expected[-1] and exp == value and > final_token.name != :DQUOTE and final_token.name != :SQUOTE > + elsif exp = @expected[-1] and exp == value and > token_value.name != :DQUOTE and token_value.name != :SQUOTE > @expected.pop > end > > - if final_token.name == :LBRACE > + if token_value.name == :LBRACE > commentpush > end > > - yield [final_token.name, value] > + yield [token_value.name, token_value] > > if @previous_token > namestack(value) if @previous_token.name == :CLASS > @@ -447,7 +480,7 @@ class Puppet::Parser::Lexer > @indefine = value > end > end > - @previous_token = final_token > + @previous_token = token_value > skip() > end > @scanner = nil > diff --git a/lib/puppet/parser/parser_support.rb b/lib/puppet/parser/ > parser_support.rb > index 1d81e60..2fb0755 100644 > --- a/lib/puppet/parser/parser_support.rb > +++ b/lib/puppet/parser/parser_support.rb > @@ -324,6 +324,7 @@ class Puppet::Parser::Parser > args[:code] = code if code > args[:parentclass] = parent if parent > args[:doc] = doc > + args[:line] = options[:line] > > @astset.classes[name] = ast AST::HostClass, args > end > @@ -350,7 +351,8 @@ class Puppet::Parser::Parser > :code => options[:code], > :parser => self, > :classname => name, > - :doc => options[:doc] > + :doc => options[:doc], > + :line => options[:line] > } > > [:code, :arguments].each do |param| > @@ -374,7 +376,8 @@ class Puppet::Parser::Parser > args = { > :name => name, > :parser => self, > - :doc => doc > + :doc => doc, > + :line => options[:line] > } > if options[:code] > args[:code] = options[:code] > diff --git a/spec/unit/parser/lexer.rb b/spec/unit/parser/lexer.rb > index 2952b73..26d0487 100755 > --- a/spec/unit/parser/lexer.rb > +++ b/spec/unit/parser/lexer.rb > @@ -4,6 +4,20 @@ require File.dirname(__FILE__) + '/../../spec_helper' > > require 'puppet/parser/lexer' > > +# This is a special matcher to match easily lexer output > +Spec::Matchers.create :be_like do |ary| > + match do |result| > + r = true > + result.zip(ary) do |a,b| > + unless a[0] == b[0] and ((a[1].is_a? > (Puppet::Parser::Lexer::TokenValue) and a[1].value == b[1]) or a[1] > == b[1]) > + r = false > + break > + end > + end > + r > + end > +end > + > describe Puppet::Parser::Lexer do > describe "when reading strings" do > before { @lexer = Puppet::Parser::Lexer.new } > @@ -474,7 +488,7 @@ describe Puppet::Parser::Lexer, "when lexing > comments" do > > it "should skip whitespace before lexing the next token after a > non-token" do > @lexer.string = "/* 1\n\n */ \ntest" > - @lexer.fullscan.include?([:NAME, "test"]).should be_true > + @lexer.fullscan.should be_like([[:NAME, "test"], > [false,false]]) > end > end > > @@ -491,7 +505,7 @@ describe "Puppet::Parser::Lexer in the old > tests" do > } > strings.each { |str,ary| > @lexer.string = str > - @lexer.fullscan().should == ary > + @lexer.fullscan().should be_like(ary) > } > end > > @@ -515,7 +529,7 @@ describe "Puppet::Parser::Lexer in the old > tests" do > } > strings.each { |str,array| > @lexer.string = str > - @lexer.fullscan().should == array > + @lexer.fullscan().should be_like(array) > } > end > > @@ -535,7 +549,7 @@ describe "Puppet::Parser::Lexer in the old > tests" do > > it "should correctly identify keywords" do > @lexer.string = "case" > - @lexer.fullscan.should == [[:CASE, "case"], [false, false]] > + @lexer.fullscan.should be_like([[:CASE, "case"], [false, > false]]) > end > > it "should correctly match strings" do > @@ -545,11 +559,11 @@ describe "Puppet::Parser::Lexer in the old > tests" do > > names.each { |t| > @lexer.string = t > - @lexer.fullscan.should == [[:NAME,t],[false,false]] > + @lexer.fullscan.should be_like([[:NAME,t],[false,false]]) > } > types.each { |t| > @lexer.string = t > - @lexer.fullscan.should == [[:CLASSREF,t],[false,false]] > + @lexer.fullscan.should be_like([[:CLASSREF,t], > [false,false]]) > } > end > > @@ -558,7 +572,7 @@ describe "Puppet::Parser::Lexer in the old > tests" do > > string.each { |t| > @lexer.string = t > - @lexer.fullscan.should == [[:NAME,t],[false,false]] > + @lexer.fullscan.should be_like([[:NAME,t],[false,false]]) > } > end > > @@ -575,7 +589,7 @@ describe "Puppet::Parser::Lexer in the old > tests" do > > @lexer.string = string > > - @lexer.fullscan.should == [[:AT, "@"], [:NAME, "type"], > [:LBRACE, "{"], [false,false]] > + @lexer.fullscan.should be_like([[:AT, "@"], [:NAME, > "type"], [:LBRACE, "{"], [false,false]]) > end > > it "should correctly deal with namespaces" do > @@ -618,7 +632,7 @@ describe "Puppet::Parser::Lexer in the old > tests" do > > @lexer.scan do |t, s| > t.should == :VARIABLE > - string.sub(/^\$/, '').should == s > + string.sub(/^\$/, '').should == s[:value] > break > end > end > @@ -630,7 +644,7 @@ describe "Puppet::Parser::Lexer in the old > tests" do > > string.each do |foo| > @lexer.string = foo > - @lexer.fullscan[0].should == [:CLASSREF, foo] > + @lexer.fullscan.should be_like([[:CLASSREF, foo], > [false,false]]) > end > end > > -- > 1.6.0.2 > > > > -- The most likely way for the world to be destroyed, most experts agree, is by accident. That's where we come in; we're computer professionals. We cause accidents. --Nathaniel Borenstein --------------------------------------------------------------------- Luke Kanies | http://reductivelabs.com | http://madstop.com --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Puppet Developers" group. To post to this group, send email to [email protected] To unsubscribe from this group, send email to [email protected] For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en -~----------~----~----~----~------~----~------~--~---
