Issue #325 new

markerlib allows cases it shouldn't

Vinay Sajip
created an issue

markerlib should raise a SyntaxError in the following cases, which are valid Python but not valid for markers:

{{{ "os.name == os.name" "'2' == '2'" "python_version == python_version" }}}

Tests should be added for such cases, as well as Python syntax errors.

Comments (9)

  1. Daniel Holth

    It would probably be possible to check the left and right hand side of the comparison operator. It would also be possible to edit Metadata 1.3 so that markers are specifically a subset of Python, and forget about the left/right restrictions.

    One thing that Metadata 1.3 specifically is intended to allow are () such as

    (python_version == '2.7' and os.name == 'linux') or (python_version == 2.6)

  2. Vinay Sajip reporter

    I'm not sure what you mean by left/right restrictions. As I see it, it should be possible to allow either

    python_version == '2.7'

    or

    '2.7 == python_version'

    as these are equivalent, but

    os.name == os.name

    and similar examples above are meaningless in the context of a marker and shouldn't be allowed. I also don't believe we should allow 2.6 as a float value (as you seem to be suggesting in your comment). If you left the quotes off 2.6 by accident - then I agree, conditional expressions are perfectly reasonable to allow as long as they don't fall foul of the "doesn't make sense" test.

  3. Daniel Holth

    A careful reading of the spec shows that os.name == os.name is legal.

    EXPR [in|==|!=|not in] EXPR
    
    where EXPR belongs to any of those:
    
    python_version = '%s.%s' % (sys.version_info[0], sys.version_info[1])
    python_full_version = sys.version.split()[0]
    os.name = os.name
    sys.platform = sys.platform
    platform.version = platform.version()
    platform.machine = platform.machine()
    platform.python_implementation = platform.python_implementation()
    a free string, like '2.4', or 'win32'
    

    Both the left and right hand side are EXPR which can be any of the choices, including a string on both sides, or a variable on both sides.

    It is possible to write useless markers. Our responsibility is only to make it possible to write useful markers.

    I forgot to include quotes in the comments. markerlib does not include Number in the set of allowed AST nodes, and raises a syntax error pointing at the column containing the number if you try to include it.

  4. Daniel Holth

    I do have strong-ish feelings about inventing a different language for environment markers because I feel it represents a distrust of Python as a programming language. PEP 345 includes the text "It makes it also easy to understand to non-pythoneers." I have removed this sentence in Metadata 1.3 because Python is easy to understand. That's supposed to be the point of Python.

    It's better for it to be Python. Non-Pythoneers will never, ever write an environment marker and are not very likely to read it. The less a Python programmer has to learn yet another language to do packaging (setup.cfg, I'm looking at you), the better.

  5. Vinay Sajip reporter

    I didn't say anything about inventing a different language. The marker DSL is already a subset of Python - full Python isn't allowed (e.g. non-string litersls are disallowed).

    My comment is about disallowing legal Python which makes no sense semantically, even though it's valid Python syntactically (just like python_version == 2.6 is, even without the quotes around the 2.6).

  6. Daniel Holth

    I meant to write python_version == '2.6' instead of python_version == 2.6.

    To implement the restriction, you could either pass objects with overridden comparison methods instead of strings as the values of python_version etc, that would guard against isinstance(other, str), or implement a visit_Compare() method in ASTWhitelist:

    >>> ast.dump(ast.parse("'2.4' == python_version", mode='eval'))
    Expression(body=Compare(left=Str(s='2.4'), ops=[Eq()], comparators=[Name(id='python_version', ctx=Load())]))
    

    visit_Compare would have to check that left and comparators[0] were one Str() and one Name() and it would probably go ahead and assert len(comparators) == 1.

    It might be important to define that an unknown variable in an environment marker is "warn and return False" instead of "crash" for when we have to add new variables.

  7. Log in to comment