unexpected "warn_ungrouped_named_tokens_in_collection" warnings in versions >2.4.0

[a-recknagel]

Hi Paul, I wanted to alert you to this issue in case you didn't hear about it yet: (python-poetry/poetry#1244, pypa/packaging#170). The poetry people are working around it by fixing pyparsing to 4.2.0 😕

[ptmcg]

Thanks - I just pushed out 2.4.2, which has all these warnings disabled by default.

[a-recknagel]

Ah, great. This can be closed then.

[fungi]

Worth noting, these returned in 3.0.0 (I assume intentionally?), and so are cropping up again where pip/packaging call into pyparser.

[ptmcg]

@fungi - thanks for reporting this, though it does sadden me.

I'm confused though - I was under the impression that packaging used a vendored 2.4.7 version of pyparsing, and so was immune to any changes in 3.0.0 until that version got vendored in (after some testing). Can you update this ticket to how these warnings are getting emitted?

[fungi]

You're right, it's not as dire as previously unless something directly depending on pyparsing gets installed. I saw it start cropping up during wheel builds, because the pypa/build tool declares an install_requires on packaging>=19.0, and packaging in turn has an install_requires on pyparsing>=2.0.2, which now brings in pyparsing 3.0.0.

[ptmcg]

And does this build tool regularly run with -Wd? Otherwise those warnings should not be enabled (or there is a bug in my code around the warning enabling/detecting bits).

[fungi]

Sorry, I didn't mean to imply it was happening by default. This is when running explicitly with PYTHONWARNINGS=error in order to catch incorrect behaviors, use of deprecated functions and other latent bugs; but since the exception is a generic one and the warning string contains a colon (the field separator for the PYTHONWARNINGS envvar), the only way to effectively filter it is to ignore all UserWarning exceptions raised within packaging.requirements, which could easily mask other conditions there.

I guess my question is, does that UserWarning actually indicate something packaging.requirements is doing incorrectly, which should be fixed?

[ptmcg]

You should be able to ignore warnings from the pyparsing module using -Wi:::pyparsing. But yes, the warning is there to tell you that the grammar you've created may be creating ambiguous or overlapping results names.

[fungi]

Thanks for the clarification, I'll see if the pypa/packaging maintainers have any interest in help correcting their use of pyparsing, or just want to wait on the PEP 508 parser replacement work instead.

[ptmcg]

Can you post the actual warning and with line number/traceback so I can see where in the code this is getting flagged?

[fungi]

Sure! Here's a minimal reproducer with full TB (essentially the same as pypa/packaging#170 but now only occurs when warnings are enabled):

$ python3.10 --version

Python 3.10.0

$ python3.10 -m venv foo

$ foo/bin/pip install packaging

Collecting packaging
  Downloading packaging-21.0-py3-none-any.whl (40 kB)
Collecting pyparsing>=2.0.2
  Downloading pyparsing-3.0.1-py3-none-any.whl (96 kB)
Installing collected packages: pyparsing, packaging
Successfully installed packaging-21.0 pyparsing-3.0.1

$ foo/bin/pip list

Package    Version
---------- -------
packaging  21.0
pip        21.2.3
pyparsing  3.0.1
setuptools 57.4.0

$ PYTHONWARNINGS=error foo/bin/python -c 'import packaging.requirements'

Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/home/fungi/foo/lib/python3.10/site-packages/packaging/requirements.py", line 66, in <module>
    VERSION_SPEC = originalTextFor(_VERSION_SPEC)("specifier")
  File "/home/fungi/foo/lib/python3.10/site-packages/pyparsing/core.py", line 1631, in __call__
    return self._setResultsName(name)
  File "/home/fungi/foo/lib/python3.10/site-packages/pyparsing/core.py", line 3671, in _setResultsName
    warnings.warn(
UserWarning: warn_ungrouped_named_tokens_in_collection: setting results name 'specifier' on And expression collides with '_original_end' on contained expression

[ptmcg]

I looked further at the pyparsing code and at your grammar, and this turns out to be a false alarm in pyparsing. I've tightened up the warning detection code, and this will be fixed in 3.0.5.

[fungi]

Thanks so much!

[ptmcg]

3.0.5 released with this fix

[fungi]

Thanks again!

[ptmcg]

I believe this fix may have broken this code in requirements.py:

MARKER_EXPR.setParseAction(
    lambda s, l, t: Marker(s[t._original_start : t._original_end])
)

To fix the warning, I renamed the internally defined results names _original_start and _original_end. Since MARKER_EXPR is and originalTextFor expression, this slicing out of s should already be done inside the pyparsing-defined parse action, so should be accessible as t[0].

This code should also be unnecessary, though since it uses a numeric index to get the version, should continue to work. Though again, originalTextFor should make this code unnecessary.

VERSION_SPEC.setParseAction(lambda s, l, t: t[1])

[9999years]

@ptmcg I'm not super familiar with pyparsing, but that code doesn't seem to work. With pyparsing 3.0.5 and packaging 20.9, recreating the problematic snippet:

from packaging.markers import MARKER_EXPR, Marker
from pyparsing import ParseResults, originalTextFor


def into_marker(original: str, loc: int, results: ParseResults) -> Marker:
    print("original:", original)
    print("loc:     ", loc)
    print("results: ", repr(results))
    return Marker(results[0])


MARKER_EXPR = originalTextFor(MARKER_EXPR)("marker")
MARKER_EXPR.setParseAction(into_marker)

MARKER_EXPR.parseString('python_version >= "3.6"')

Produces the following output:

original: python_version >= "3.6"
loc:      0
results:  ParseResults([0, (<Variable('python_version')>, <Op('>=')>, <Value('3.6')>), 23], {'_NOWARN_original_start': 0, '_NOWARN_original_end': 23, 'marker': [0, (<Variable('python_version')>, <Op('>=')>, <Value('3.6')>), 23]})
Traceback (most recent call last):
  File "xxx_parse_reqs.py", line 40, in <module>
    print(MARKER_EXPR.parseString('python_version >= "3.6"'))
  File "/home/rturner/.cache/pypoetry/virtualenvs/example-package-JfO58x2Y-py3.7/lib/python3.7/site-packages/pyparsing/core.py", line 1100, in parse_string
    loc, tokens = self._parse(instring, 0)
  File "/home/rturner/.cache/pypoetry/virtualenvs/example-package-JfO58x2Y-py3.7/lib/python3.7/site-packages/pyparsing/core.py", line 827, in _parseNoCache
    tokens = fn(instring, tokens_start, ret_tokens)
  File "/home/rturner/.cache/pypoetry/virtualenvs/example-package-JfO58x2Y-py3.7/lib/python3.7/site-packages/pyparsing/core.py", line 283, in wrapper
    ret = func(*args[limit:])
  File "xxx_parse_reqs.py", line 34, in into_marker
    return Marker(results[0])
  File "/home/rturner/.cache/pypoetry/virtualenvs/example-package-JfO58x2Y-py3.7/lib/python3.7/site-packages/packaging/markers.py", line 307, in __init__
    self._markers = _coerce_parse_result(MARKER.parseString(marker))
  File "/home/rturner/.cache/pypoetry/virtualenvs/example-package-JfO58x2Y-py3.7/lib/python3.7/site-packages/pyparsing/core.py", line 1098, in parse_string
    instring = instring.expandtabs()
AttributeError: 'int' object has no attribute 'expandtabs'

It doesn't seem to change depending on the usage of the as_string argument to original_text_for, either.

This expression seems to work:

MARKER_EXPR.setParseAction(
    lambda s, l, t: Marker(s[t[0] : t[-1]])
)

But I'm not sure why, and the documentation seems to imply that it shouldn't work, so I'm not sure what's going on here.

[ptmcg]

In your code that doesn't work, this line:

MARKER_EXPR.setParseAction(into_marker)

should be:

MARKER_EXPR.addParseAction(into_marker)

originalTextFor has its own internal parse action to slice the input string using the starting and ending locations. setParseAction will overwrite that parse action; addParseAction will add your parse action to the chain of parse actions.

[9999years]

Ah, thank you! That change works -- I'm not sure why the downstream was using setParseAction instead of addParseAction.

[ptmcg]

It is very old code, and probably predates when addParseAction was added to pyparsing.

[ptmcg]

Fixed (for the last time!) in 3.0.6