markerlib allows cases it shouldn't

Create issue
Issue #325 new
Vinay Sajip created an issue

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

{{{ " ==" "'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 == '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'


    '2.7 == python_version'

    as these are equivalent, but ==

    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 == 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] =
    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. Vinay Sajip reporter

    I see that, but could it just be an oversight in the spec? For example, distutils2 tests disallow the cases I mentioned.

    While I don't have very strong feelings about it, it would be nice to disallow useless cases which are readily identifiable, because it would cause less surprise.

  5. 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.

  6. 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).

  7. 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.

  8. Log in to comment