mitodl / mitx-grading-library

The MITx Grading Library, a python grading library for edX
https://edge.edx.org/courses/course-v1:MITx+grading-library+examples/
BSD 3-Clause "New" or "Revised" License
14 stars 9 forks source link

Suffix match order bug #132

Closed ChristopherChudzicki closed 6 years ago

ChristopherChudzicki commented 6 years ago

Note: I came across this bug while rewriting the parser/evaluator. I'm creating the issue mostly so I can reference it in a future PR. (A PR which would also fix this bug.)

from mitxgraders.helpers.calc import evaluator

variables = {}
functions = {}
suffixes_ok = {  'ab': 2 }
suffixes_error = { 'a': 10, 'ab': 2 }

expr = '5ab'

evaluator(expr, variables, functions, suffixes_ok) # works as expected
evaluator(expr, variables, functions, suffixes_error) # throws an error

The issue is that a is a front substring of ab. I think that the grammar was originally intended to handle this case because in defining suffixes, MatchFirst is used, which can be used to specify the order in which to check. (So above, MatchFirst(['ab', 'a']) would have been fine.) But we give MatchFirst a bunch of dictionary keys, which aren't ordered:

# Define our suffixes
suffix = MatchFirst(Literal(k) for k in self.suffixes.keys())
ChristopherChudzicki commented 6 years ago

Incidentally, it is also possible to do some pretty silly things with suffixes:

from mitxgraders.helpers.calc import evaluator

variables = {}
functions = {}
suffixes_ok = { '-1': 10 }

expr = '5-1'

result, _ = evaluator(expr, variables, functions, suffixes_ok)
print(result) # 10.0
jolyonb commented 6 years ago

You sure that doesn't give you 50?

I was aware the suffixes had these issues, but since we were restricting them to % and hard-coded metric suffixes, it seemed safe enough. Feel free to fix if you like. I suspect that it should probably restrict to single character suffixes that cannot include certain characters.

jolyonb commented 6 years ago

Resolved by #134