Implement comparison (x==y and x!=y) for _sre.SRE_Pattern

[vstinner]

BPO	28727
Nosy	@vstinner, @ezio-melotti, @serhiy-storchaka
PRs	[Do Not Merge] Convert Misc/NEWS so that it is managed by towncrier #552
Files	pattern_compare.patch pattern_compare-2.patch pattern_compare-3.patch pattern_compare-4.patch pattern_compare-5.patch pattern_compare-6.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/vstinner'
closed_at = <Date 2016-11-22.14:32:35.264>
created_at = <Date 2016-11-17.16:02:48.047>
labels = ['expert-regex', 'type-feature', '3.7']
title = 'Implement comparison (x==y and x!=y) for _sre.SRE_Pattern'
updated_at = <Date 2017-03-31.16:36:24.688>
user = 'https://github.com/vstinner'

bugs.python.org fields:

activity = <Date 2017-03-31.16:36:24.688>
actor = 'dstufft'
assignee = 'vstinner'
closed = True
closed_date = <Date 2016-11-22.14:32:35.264>
closer = 'vstinner'
components = ['Regular Expressions']
creation = <Date 2016-11-17.16:02:48.047>
creator = 'vstinner'
dependencies = []
files = ['45520', '45523', '45528', '45529', '45541', '45587']
hgrepos = []
issue_num = 28727
keywords = ['patch']
message_count = 23.0
messages = ['281046', '281052', '281057', '281062', '281063', '281083', '281084', '281086', '281087', '281088', '281094', '281101', '281178', '281306', '281362', '281363', '281365', '281369', '281374', '281479', '281480', '281483', '281484']
nosy_count = 6.0
nosy_names = ['vstinner', 'ezio.melotti', 'mrabarnett', 'SilentGhost', 'python-dev', 'serhiy.storchaka']
pr_nums = ['552']
priority = 'normal'
resolution = 'fixed'
stage = 'commit review'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue28727'
versions = ['Python 3.7']

[vstinner]

Attached patch implements rich comparison for _sre.SRE_Pattern objects created by re.compile().

Comparison between patterns is used in the warnings module to not add duplicated filters, see issue bpo-18383:

New changeset f57f4e33ba5e by Martin Panter in branch '3.5':
Issue bpo-18383: Avoid adding duplicate filters when warnings is reloaded
https://hg.python.org/cpython/rev/f57f4e33ba5e

For the warnings module, it became a problem in test_warnings since the Python test runner started to clear all caches. When re.purge() is called, re.compile() creates a new object, whereas with the cache it returns the same object and so the two patterns are equal since it's the same object. => see issue bpo-28688

[SilentGhost]

Ten subtest in test_re fail with

TypeError: unhashable type: '_sre.SRE_Pattern'

[vstinner]

Ten subtest in test_re fail with: TypeError: unhashable type: '_sre.SRE_Pattern'

Oops, right. Updated patch implements also hash() on patterns.

[serhiy-storchaka]

Added comments on Rietveld.

[mrabarnett]

I hope you make it clear what you mean by 'equal', i.e. that it's comparing the pattern and the flags (AFAICT), so re.compile('(?x)a') != re.compile('(?x) a ').

[vstinner]

Patch version 3:

• pattern_hash() includes isbytes, I also shifted flags by 1 bit to not erase the isbytes bit (FYI maximum value of flags is 256)
• pattern_richcompare() avoids calling PyObject_RichCompareBool() if flags or isbytes is different
• unit test ensures that no BytesWarning warning is raised
• checks hash() in unit tests
• fix also the unit test with a different flag (use the same pattern)
• document also in the unit test that the comparison is case sensitive

[vstinner]

Ok, I hope that it's the last attempt: patch 5

• Remove hash(b) != hash(a): only keep tests on hash(b)==hash(a) when b==a
• Replace re.ASCII flag with no flag to test two patterns with different flags

[serhiy-storchaka]

There is a problem with locale-depending flags. The same pattern may be compiled with the LOCALE flag to different pattern objects in different locales. Perhaps we should compare the compiled code instead of pattern strings. Or both.

[vstinner]

Serhiy: "There is a problem with locale-depending flags. The same pattern may be compiled with the LOCALE flag to different pattern objects in different locales."

Oh, I didn't know and you are right.

"Perhaps we should compare the compiled code instead of pattern strings. Or both."

PatternObject contains many fields. I used the following two fields which come from re.compile():

• pattern
• flags

I considered that they were enough because pattern_repr() only displays these ones. Other fields:

• groups
• groupindex
• indexgroup
• weakreflist
• isbytes
• codesize, code

weakreflist can be skipped, isbytes is already tested in my patch.

Would it be possible to only compare code instead of pattern? What are groups, groupindex and indexgroup: should we also compare them?

Maybe I can start from pattern_compare-4.patch and only add a test comparing code?

[serhiy-storchaka]

'[abc]' and '[cba]' are compiled to the same code. Do we want to handle them as equal? This is implementation defined.

[vstinner]

'[abc]' and '[cba]' are compiled to the same code. Do we want to handle them as equal?

Comparison must be fast. If the comparison is just memcmp(code1,
code2, codesize), it's ok.

I agree that we must put a similar somewhere to say that some parts
are implementation details. Maybe split the test in two parts, and
mark the second part with @cpython_only.

[serhiy-storchaka]

I see two options:

• Compare flags, isbytes and code. This makes some different patterns be compiled to equal objects. For example spaces in verbose mode are ignored. But this don't make equal all equivalent patterns. '[aA]' would be equal to '(?:a|A)' but still would be not equal to '(i?a)' with current implementation.

• Compare flags, isbytes, code and pattern. This makes literally different patterns be compiled to not equal objects even if the difference is not significant. '[abc]' would be different from '[cba]' despites the fact that matching both always returns the same result.

Since this issue becomes a little ambiguous, I would target the patch to 3.7 only. Maybe we will find other subtle details or will decide to change the meaning of equality of pattern objects before releasing 3.7.