Skip to content

Loading…

Match#method_missing is unsafe #41

Merged
merged 14 commits into from

2 participants

@blambeau
Collaborator

The added test case shows that Kernel::p is actually called in place of Match#method_missing when 'p' is used as a label or rule name.

As any gem might contribute private methods to Object, using Citrus labels (or even rule names, actually) in semantic productions without doing so with Match#[] or Match#captures is unsafe.

Moreover, the addition of any method/helper to the Match class may potentially break an existing grammar, so it breaks Citrus's public API and should probably increment the MAJOR version number.

I imagine two possible ways of fixing this problem:

1) Dynamically generate methods to the matches (from captures) so that labels and rule names are correctly resolved by ruby itself. The main advantage is that it won't lead to break Citrus. There will be an extra cost at Match#process_events! time.
2) Remove method_missing completely and clearly document that the safe way to go is to use self[:rule_name] in semantic production rules.

Any comment on this? If you agree that this is a problem (?) and make a choice for a particular solution, I can complete the pull request with a fix.

@mjackson
Owner

@blambeau Let's go ahead and merge this, though I think that lib/citrus/core_ext.rb should require 'citrusat the top. This lets us remove therequire 'citrus'intest/helper.rb` and simplifies usage for end users as well.

@mjackson
Owner

@blambeau Thank you for bringing this to my attention! :) I would be ok with removing method_missing support completely and incrementing the major version number. Let's do this in two separate commits: the first that makes the fix and updates all tests/documentation to reflect the change and another to increment the version number so we can cut a new release.

@blambeau
Collaborator

Ok, I'll do it when I have some OSS time for Citrus (hopefully in a few days).

I would be in favor of also releasing 2.4.2 with pending commits and ruby warnings about method_missing that will be removed as well as Object#grammar.

@blambeau blambeau added a commit that referenced this pull request
@blambeau blambeau Remove Match#method_missing, add Match#capture(name)
Match#method_missing is unsafe as illustrated in Github issue #41.
In particular, it makes composing a grammar with aribitrary gems
unsafe when the latter make core extensions, leads to unexpected
results with labels matching Kernel methods (e.g. `p`), and
prevents the Match class from getting new methods in a backward
compatible way. This commit therefore removes it.

A convenient method is provided to get a single capture by label
name. Calling string operators must now be done via Match#to_str.
4cf4839
blambeau added some commits
@blambeau blambeau Show why Match#method_missing is unsafe with a test.
The added test case shows that Kernel::p is actually called in place
of Match#method_missing when 'p' is used as a label or rule name.

As any gem might contribute private methods to Object, using Citrus
labels (or even rule names, actually) in semantic productions without
doing so with Match#captures is unsafe.

Moreover, the addition of any method/helper to the Match class may
potentially break an existing grammar, so it breaks Citrus's public
API and should probably increment the MAJOR version number.
0738d9e
@blambeau blambeau Remove Match#method_missing, add Match#capture(name)
Match#method_missing is unsafe as illustrated in Github issue #41.
In particular, it makes composing a grammar with aribitrary gems
unsafe when the latter make core extensions, leads to unexpected
results with labels matching Kernel methods (e.g. `p`), and
prevents the Match class from getting new methods in a backward
compatible way. This commit therefore removes it.

A convenient method is provided to get a single capture by label
name. Calling string operators must now be done via Match#to_str.
1a194c0
@blambeau blambeau Fix the Match#method_missing related test. 8ce434f
@blambeau blambeau Update README to reflect Match#method_missing removal. 935aa14
@blambeau blambeau Enhance Match#captures and test both #capture and #captures b77f558
@blambeau blambeau referenced this pull request
Closed

Review for releasing 2.5.0 #45

@blambeau
Collaborator

@mjijackson So, here is a proposal for removing Match#method_missing. The CHANGES file has been updated to document the proposed 2.5.x -> 3.0 migration plan. Tell me what you think of it.

@mjackson
Owner

This looks great. The explanation in the CHANGES file is succinct and well-written. Thank you for remembering to update the bundled grammars as well.

@mjackson
Owner

Ok, I've merged this work and cut the 3.0 release!

Please note: I re-arranged some of the commits (but kept your authorship) to keep 2.5.x inline with master because 2.5.x shouldn't have any commits that aren't also in master (yet), so you will probably need to git reset --hard origin/master after fetching updates.

@mjackson mjackson closed this
@blambeau
Collaborator

@mjijackson Hmmm. Looks like we had a release problem here. Supposed changes for 3.0 have not been included in the release. Match#method_missing is still there in master (and in released gem).

@mjackson
Owner

!!! :/ Oh man, I forgot to pull in the no-method-missing branch before cutting the release. I've merged that branch and pushed f33ec13.

@blambeau Can you please verify that commit is good and I'll cut version 3.0.1 as a fix?

Thanks!

@mjackson mjackson reopened this
@mjackson mjackson merged commit 8360cc4 into mjackson:master
@blambeau
Collaborator

Looks good this time. You can cut 3.0.1 for me! Thanks @mjijackson.

@mjackson
Owner

@blambeau Done! Thanks again for all your work on this, and sorry for botching the release so badly. I'll keep working on my git-fu to prevent future incidents!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Dec 9, 2013
  1. Merge pull request #42 from tbuehlmann/master

    committed
    Removed Ruby warnings
Commits on Mar 6, 2014
  1. @blambeau
Commits on Mar 10, 2014
  1. @blambeau

    Require citrus in core_ext.rb

    blambeau committed
  2. @blambeau
  3. Merge pull request #44 from blambeau/master

    committed
    Move core extensions to lib/core_ext.rb
Commits on Mar 13, 2014
  1. @blambeau

    Updated CHANGES

    blambeau committed
  2. @blambeau

    Releasing 2.5.0

    blambeau committed
  3. @blambeau

    Updated CHANGES

    blambeau committed
Commits on Mar 14, 2014
  1. @blambeau

    Show why Match#method_missing is unsafe with a test.

    blambeau committed
    The added test case shows that Kernel::p is actually called in place
    of Match#method_missing when 'p' is used as a label or rule name.
    
    As any gem might contribute private methods to Object, using Citrus
    labels (or even rule names, actually) in semantic productions without
    doing so with Match#captures is unsafe.
    
    Moreover, the addition of any method/helper to the Match class may
    potentially break an existing grammar, so it breaks Citrus's public
    API and should probably increment the MAJOR version number.
  2. @blambeau

    Remove Match#method_missing, add Match#capture(name)

    blambeau committed
    Match#method_missing is unsafe as illustrated in Github issue #41.
    In particular, it makes composing a grammar with aribitrary gems
    unsafe when the latter make core extensions, leads to unexpected
    results with labels matching Kernel methods (e.g. `p`), and
    prevents the Match class from getting new methods in a backward
    compatible way. This commit therefore removes it.
    
    A convenient method is provided to get a single capture by label
    name. Calling string operators must now be done via Match#to_str.
  3. @blambeau
  4. @blambeau
  5. @blambeau
  6. @blambeau

    Rephrase CHANGES.

    blambeau committed
Showing with 162 additions and 74 deletions.
  1. +49 −0 CHANGES
  2. +16 −14 README.md
  3. +7 −31 lib/citrus.rb
  4. +19 −0 lib/citrus/core_ext.rb
  5. +31 −15 lib/citrus/file.rb
  6. +8 −8 lib/citrus/grammars/calc.citrus
  7. +1 −1 lib/citrus/version.rb
  8. +2 −2 test/extension_test.rb
  9. +7 −0 test/grammar_test.rb
  10. +1 −1 test/helper.rb
  11. +21 −2 test/match_test.rb
View
49 CHANGES
@@ -1,5 +1,52 @@
= HEAD
+ * Moved Object#grammar to citrus/core_ext.rb. Citrus no longer installs core
+ extensions by default. Use "require 'citrus/core_ext.rb'" instead of
+ "require 'citrus'" to keep the previous behavior.
+
+ * Removed Match#method_missing, added #capture(name) and #captures(name)
+
+ Match#method_missing is unsafe as illustrated in Github issue #41. In
+ particular, it makes composing a grammar with aribitrary gems unsafe (e.g.
+ when the latter make core extensions), leads to unexpected results with
+ labels match existing Kernel methods (e.g. `p`), and prevents Match from
+ getting new methods in a backward compatible way. This commit therefore
+ removes it.
+
+ In Citrus 2.x, method_missing allowed rule productions to denote captured
+ matches by label name:
+
+ rule pair
+ (foo ':' bar) {
+ [foo.value, bar.value]
+ }
+ end
+
+ Also, it allowed invoking String operators on the Match's text:
+
+ rule int
+ [0-9]+ { to_i }
+ end
+
+ Those two scenarios no longer work out of the box in Citrus 3.0. You must
+ use capture(label) for the former, and to_str for the latter:
+
+ rule pair
+ (foo ':' bar) {
+ [capture(:foo).value, capture(:bar).value]
+ }
+ end
+
+ rule int
+ [0-9]+ { to_str.to_i }
+ end
+
+ Match#captures now accepts an optional label name as first argument and
+ returns the corresponding array of matches for that label (useful in case
+ the label belongs to a repetition).
+
+= 2.5.0 / 2014-03-13
+
* Inputs may be generated from many different sources, including Pathname and
IO objects (thanks blambeau).
@@ -9,6 +56,8 @@
* Citrus.load no longer raises Citrus::LoadError for files that can't be found
or are not readable. Users must rescue Errno::ENOENT instead, for example.
+ * Removed a few ruby warnings (thanks tbuehlmann)
+
= 2.4.1 / 2011-11-04
* Fixed a bug that prevented rule names from starting with "super".
View
30 README.md
@@ -232,8 +232,8 @@ Additionally, extensions may be specified inline using curly braces. When using
this method, the code inside the curly braces may be invoked by calling the
`value` method on the match object.
- [0-9] { to_i } # match any digit and return its integer value when
- # calling the #value method on the match object
+ [0-9] { to_str.to_i } # match any digit and return its integer value when
+ # calling the #value method on the match object
Note that when using the inline block method you may also specify arguments in
between vertical bars immediately following the opening curly brace, just like
@@ -358,13 +358,13 @@ blocks. Let's extend the `Addition` grammar using this technique.
grammar Addition
rule additive
(number plus term:(additive | number)) {
- number.value + term.value
+ capture(:number).value + capture(:term).value
}
end
rule number
([0-9]+ space) {
- to_i
+ to_str.to_i
}
end
@@ -383,17 +383,19 @@ execute by calling `value` on match objects that result from those rules. It's
easiest to explain what is going on here by starting with the lowest level
block, which is defined within `number`.
-Inside this block we see a call to another method, namely `to_i`. When called in
-the context of a match object, methods that are not defined may be called on a
-match's internal string object via `method_missing`. Thus, the call to `to_i`
-should return the integer value of the match.
+Inside this block we see a call to another method, namely `to_str`. When called
+in the context of a match object, this method returns the match's internal
+string object. Thus, the call to `to_str.to_i` should return the integer value
+of the match.
Similarly, matches created by `additive` will also have a `value` method. Notice
the use of the `term` label within the rule definition. This label allows the
match that is created by the choice between `additive` and `number` to be
-retrieved using the `term` method. The value of an additive match is determined
+retrieved using `capture(:term)`. The value of an additive match is determined
to be the values of its `number` and `term` matches added together using Ruby's
-addition operator.
+addition operator. Note that the plural form `captures(:term)` can be used to
+get an array of matches for a given label (e.g. when the label belongs to a
+repetition).
Since `additive` is the first rule defined in the grammar, any match that
results from parsing a string with this grammar will have a `value` method that
@@ -441,11 +443,11 @@ look like this:
rule additive
(number plus term:(additive | number)) {
def lhs
- number.value
+ capture(:number).value
end
def rhs
- term.value
+ capture(:term).value
end
def value
@@ -475,11 +477,11 @@ define the following module.
module Additive
def lhs
- number.value
+ capture(:number).value
end
def rhs
- term.value
+ capture(:term).value
end
def value
View
38 lib/citrus.rb
@@ -1326,9 +1326,14 @@ def string
# Returns a hash of capture names to arrays of matches with that name,
# in the order they appeared in the input.
- def captures
+ def captures(name = nil)
process_events! unless @captures
- @captures
+ name ? @captures[name] : @captures
+ end
+
+ # Convenient method for captures[name].first
+ def capture(name)
+ captures[name].first
end
# Returns an array of all immediate submatches of this match.
@@ -1342,16 +1347,6 @@ def first
matches.first
end
- # Allows methods of this match's string to be called directly and provides
- # a convenient interface for retrieving the first match with a given name.
- def method_missing(sym, *args, &block)
- if string.respond_to?(sym)
- string.__send__(sym, *args, &block)
- else
- captures[sym].first
- end
- end
-
alias_method :to_s, :string
# This alias allows strings to be compared to the string value of Match
@@ -1552,22 +1547,3 @@ def captures_hash
end
end
end
-
-class Object
- # A sugar method for creating Citrus grammars from any namespace.
- #
- # grammar :Calc do
- # end
- #
- # module MyModule
- # grammar :Calc do
- # end
- # end
- #
- def grammar(name, &block)
- namespace = respond_to?(:const_set) ? self : Object
- namespace.const_set(name, Citrus::Grammar.new(&block))
- rescue NameError
- raise ArgumentError, "Invalid grammar name: #{name}"
- end
-end
View
19 lib/citrus/core_ext.rb
@@ -0,0 +1,19 @@
+require 'citrus'
+class Object
+ # A sugar method for creating Citrus grammars from any namespace.
+ #
+ # grammar :Calc do
+ # end
+ #
+ # module MyModule
+ # grammar :Calc do
+ # end
+ # end
+ #
+ def grammar(name, &block)
+ namespace = respond_to?(:const_set) ? self : Object
+ namespace.const_set(name, Citrus::Grammar.new(&block))
+ rescue NameError
+ raise ArgumentError, "Invalid grammar name: #{name}"
+ end
+end
View
46 lib/citrus/file.rb
@@ -6,6 +6,10 @@ module Citrus
# Some helper methods for rules that alias +module_name+ and don't want to
# use +Kernel#eval+ to retrieve Module objects.
module ModuleNameHelpers #:nodoc:
+ def module_name
+ capture(:module_name)
+ end
+
def module_segments
@module_segments ||= module_name.value.split('::')
end
@@ -58,6 +62,7 @@ def value
captures[:include].each {|inc| grammar.include(inc.value) }
captures[:rule].each {|r| grammar.rule(r.rule_name.value, r.value) }
+ root = capture(:root)
grammar.root(root.value) if root
grammar
@@ -66,10 +71,17 @@ def value
end
rule :rule do
- all(:rule_keyword, :rule_name, zero_or_one(:expression), :end_keyword) {
- # An empty rule definition matches the empty string.
- expression ? expression.value : Rule.for('')
- }
+ mod all(:rule_keyword, :rule_name, zero_or_one(:expression), :end_keyword) do
+ def rule_name
+ capture(:rule_name)
+ end
+
+ def value
+ # An empty rule definition matches the empty string.
+ expr = capture(:expression)
+ expr ? expr.value : Rule.for('')
+ end
+ end
end
rule :expression do
@@ -88,7 +100,8 @@ def value
rule :labelled do
all(zero_or_one(:label), :extended) {
- rule = extended.value
+ label = capture(:label)
+ rule = capture(:extended).value
rule.label = label.value if label
rule
}
@@ -96,7 +109,8 @@ def value
rule :extended do
all(:prefix, zero_or_one(:extension)) {
- rule = prefix.value
+ extension = capture(:extension)
+ rule = capture(:prefix).value
rule.extension = extension.value if extension
rule
}
@@ -104,7 +118,8 @@ def value
rule :prefix do
all(zero_or_one(:predicate), :suffix) {
- rule = suffix.value
+ predicate = capture(:predicate)
+ rule = capture(:suffix).value
rule = predicate.value(rule) if predicate
rule
}
@@ -112,7 +127,8 @@ def value
rule :suffix do
all(:primary, zero_or_one(:repeat)) {
- rule = primary.value
+ repeat = capture(:repeat)
+ rule = capture(:primary).value
rule = repeat.value(rule) if repeat
rule
}
@@ -124,7 +140,7 @@ def value
rule :grouping do
all(['(', zero_or_one(:space)], :expression, [')', zero_or_one(:space)]) {
- expression.value
+ capture(:expression).value
}
end
@@ -132,7 +148,7 @@ def value
rule :require do
all(:require_keyword, :quoted_string) {
- quoted_string.value
+ capture(:quoted_string).value
}
end
@@ -148,7 +164,7 @@ def value
rule :root do
all(:root_keyword, :rule_name) {
- rule_name.value
+ capture(:rule_name).value
}
end
@@ -172,7 +188,7 @@ def value
rule :alias do
all(notp(:end_keyword), :rule_name) {
- Alias.new(rule_name.value)
+ Alias.new(capture(:rule_name).value)
}
end
@@ -232,7 +248,7 @@ def flags
rule :label do
all(/[a-zA-Z0-9_]+/, :space, ':', :space) {
- first.to_sym
+ first.to_str.to_sym
}
end
@@ -312,8 +328,8 @@ def value
rule :star do
all(/[0-9]*/, '*', /[0-9]*/, :space) { |rule|
- min = captures[1] == '' ? 0 : captures[1].to_i
- max = captures[3] == '' ? Infinity : captures[3].to_i
+ min = captures[1] == '' ? 0 : captures[1].to_str.to_i
+ max = captures[3] == '' ? Infinity : captures[3].to_str.to_i
Repeat.new(rule, min, max)
}
end
View
16 lib/citrus/grammars/calc.citrus
@@ -12,7 +12,7 @@ grammar Calc
rule additive
(factor operator:('+' | '-') space* term) {
- factor.value.send(operator.to_s, term.value)
+ capture(:factor).value.send(capture(:operator).to_s, capture(:term).value)
}
end
@@ -22,7 +22,7 @@ grammar Calc
rule multiplicative
(prefix operator:('*' | '/' | '%') space* factor) {
- prefix.value.send(operator.to_s, factor.value)
+ capture(:prefix).value.send(capture(:operator).to_s, capture(:factor).value)
}
end
@@ -32,9 +32,9 @@ grammar Calc
rule prefixed
(operator:('-' | '+' | '~') space* prefix) {
- s = operator.to_s
+ s = capture(:operator).to_s
s += '@' unless s == '~' # Unary + and - require an @.
- prefix.value.send(s)
+ capture(:prefix).value.send(s)
}
end
@@ -44,7 +44,7 @@ grammar Calc
rule exponential
(primary operator:'**' space* prefix) {
- primary.value.send(operator.to_s, prefix.value)
+ capture(:primary).value.send(capture(:operator).to_s, capture(:prefix).value)
}
end
@@ -54,7 +54,7 @@ grammar Calc
rule group
(lparen term rparen) {
- term.value
+ capture(:term).value
}
end
@@ -65,11 +65,11 @@ grammar Calc
end
rule float
- (digits '.' digits space*) { to_f }
+ (digits '.' digits space*) { to_str.to_f }
end
rule integer
- (digits space*) { to_i }
+ (digits space*) { to_str.to_i }
end
rule digits
View
2 lib/citrus/version.rb
@@ -1,6 +1,6 @@
module Citrus
# The current version of Citrus as [major, minor, patch].
- VERSION = [2, 4, 1]
+ VERSION = [2, 5, 0]
# Returns the current version of Citrus as a string.
def self.version
View
4 test/extension_test.rb
@@ -9,12 +9,12 @@ def a_test
module NumericModule
def add_one
- to_i + 1
+ to_str.to_i + 1
end
end
NumericProcBare = Proc.new {
- to_i + 1
+ to_str.to_i + 1
}
def test_match_module
View
7 test/grammar_test.rb
@@ -149,6 +149,13 @@ def test_parse_file
end
end
+ def test_labeled_production
+ grammar = Grammar.new {
+ rule(:abc) { label('abc', :p){ capture(:p) } }
+ }
+ assert_equal('abc', grammar.parse('abc').value)
+ end
+
def test_global_grammar
assert_raise ArgumentError do
grammar(:abc)
View
2 test/helper.rb
@@ -2,7 +2,7 @@
$LOAD_PATH.unshift(lib) unless $LOAD_PATH.include?(lib)
require 'test/unit'
-require 'citrus'
+require 'citrus/core_ext'
class Test::Unit::TestCase
include Citrus
View
23 test/match_test.rb
@@ -94,13 +94,13 @@ def test_matches
grammar :Addition do
rule :additive do
all(:number, :plus, label(any(:additive, :number), 'term')) {
- number.value + term.value
+ capture(:number).value + capture(:term).value
}
end
rule :number do
all(/[0-9]+/, :space) {
- strip.to_i
+ to_str.strip.to_i
}
end
@@ -139,4 +139,23 @@ def test_matches2
assert(match.matches)
assert_equal(3, match.matches.length)
end
+
+ def test_capture
+ match = Addition.parse('1+2')
+ assert(match.capture(:number))
+ assert_equal(1, match.capture(:number).value)
+ end
+
+ def test_captures
+ match = Addition.parse('1+2')
+ assert(match.captures)
+ assert_equal(7, match.captures.size)
+ end
+
+ def test_captures_with_a_name
+ match = Addition.parse('1+2')
+ assert(match.captures(:number))
+ assert_equal(2, match.captures(:number).size)
+ assert_equal([1, 2], match.captures(:number).map(&:value))
+ end
end
Something went wrong with that request. Please try again.