Fix parsing IRCv3 tags with missing/empty values

[ZeroKnight]

Should resolve #148.

[logan2611]

Looks like when the tag is missing, Pydle still crashes with this

DEBUG:MyOwnBot:(servername):<< @account=logan2611 :logan2611!logan2611@(ip) JOIN #logan2611 logan2611 :realname
**!** data=b'@inspircd.org/service;inspircd.org/bot :ChanServ!services@services.(domain) MODE #logan2611 +qo logan2611 :logan2611\r\n'
ERROR:asyncio:Task exception was never retrieved
future: <Task finished name='Task-2' coro=<BasicClient.handle_forever() done, defined at /mnt/hdd/Documents/ExampleBot/lib/python3.8/site-packages/pydle/client.py:363> exception=UnboundLocalError("local variable 'value' referenced before assignment")>
Traceback (most recent call last):
  File "/mnt/hdd/Documents/ExampleBot/lib/python3.8/site-packages/pydle/client.py", line 382, in handle_forever
    await self.on_data(data)
  File "example.py", line 13, in on_data
    await super().on_data(data)
  File "/mnt/hdd/Documents/ExampleBot/lib/python3.8/site-packages/pydle/client.py", line 391, in on_data
    message = self._parse_message()
  File "/mnt/hdd/Documents/ExampleBot/lib/python3.8/site-packages/pydle/features/ircv3/tags.py", line 125, in _parse_message
    return TaggedMessage.parse(message + sep, encoding=self.encoding)
  File "/mnt/hdd/Documents/ExampleBot/lib/python3.8/site-packages/pydle/features/ircv3/tags.py", line 66, in parse
    if not value:
UnboundLocalError: local variable 'value' referenced before assignment

[logan2611]

After @theunkn0wn1's change, the bot is now working as expected and handles the tag just fine

DEBUG:MyOwnBot:(servername):<< @account=logan2611 :logan2611!logan2611@((ip) JOIN #logan2611 logan2611 :realname
**!** data=b'@inspircd.org/service;inspircd.org/bot :ChanServ!services@services.(servername).com MODE #logan2611 +qo logan2611 :logan2611\r\n'
DEBUG:MyOwnBot:(servername):<< @inspircd.org/service;inspircd.org/bot :ChanServ!services@services.(servername).com MODE #logan2611 +qo logan2611 :logan2611
**!** data=b'@msgid=796~1602221579~51;account=logan2611 :logan2611!logan2611@((ip) PRIVMSG #logan2611 :ping\r\n'
DEBUG:MyOwnBot:(servername):<< @msgid=796~1602221579~51;account=logan2611 :logan2611!logan2611@((ip) PRIVMSG #logan2611 :ping
DEBUG:MyOwnBot:(servername):>> PRIVMSG #logan2611 ping

**!** data=b'@msgid=796~1602221579~52 :MyBot!mybot@((ip) PRIVMSG #logan2611 :ping\r\n'
DEBUG:MyOwnBot:(servername):<< @msgid=796~1602221579~52 :MyBot!mybot@((ip) PRIVMSG #logan2611 :ping

[theunkn0wn1]

Thanks for the update @logan2611 can you do me a favor and verify the fix with the current state of the PR?

[shizmob]

On Oct 13, 2 Reiwa, at 22:52, Alex George notifications@github.com wrote:

@ZeroKnight commented on this pull request.

It wouldn't be a bad idea to have a test case for empty tags while we're at it, like what you mentioned in #148 https://github.com/Shizmob/pydle/issues/148:

    (
            rb"@empty=;missing :unknown!orion@161D248E.B897F01F.3F699B1A.IP PRIVMSG #unkn0wndev :ping",
            {"empty": True, "missing": True}
    )

The missing bit might not be necessary since this regression test would cover it. On the other hand, the inspircd one doubles as a test for parsing vendor-prefix tags.

Empty shouldn’t be True here, as the separator is still in place. It should be the empty string, as empty values may still make sense depending on the context.

[logan2611]

Thanks for the update @logan2611 can you do me a favor and verify the fix with the current state of the PR?

Retested the PR, looks like its working fine :+1:

[ZeroKnight]

Empty shouldn’t be True here, as the separator is still in place. It should be the empty string, as empty values may still make sense depending on the context.

While an empty string may indeed be a valid interpretation in some contexts, the spec still mandates that empty and missing values be treated the same:

Implementations MUST interpret empty tag values (e.g. foo=) as equivalent to missing tag values (e.g. foo). Specifications MUST NOT differentiate meaning between tags with empty and missing values. Implementations MAY normalise tag values by converting the empty form to the missing form, but MUST NOT convert values from missing to empty, to prevent size limit issues.

Perhaps assigning None or a named constant like TAG_NO_VALUE or something to that effect would be preferable to assigning True. Still, if there's a tag out in the wild that does differentiate between empty and missing, then the tag is non-compliant and should be fixed.

[shizmob]

On Oct 13, 2 Reiwa, at 23:06, Alex George notifications@github.com wrote:

Empty shouldn’t be True here, as the separator is still in place. It should be the empty string, as empty values may still make sense depending on the context.

While an empty string may indeed be a valid interpretation in some contexts, the spec https://ircv3.net/specs/extensions/message-tags still mandates that empty and missing values be treated the same and that they not be treated differently:

Implementations MUST interpret empty tag values (e.g. foo=) as equivalent to missing tag values (e.g. foo). Specifications MUST NOT differentiate meaning between tags with empty and missing values. Implementations MAY normalise tag values by converting the empty form to the missing form, but MUST NOT convert values from missing to empty, to prevent size limit issues.

That is fair actually — it’s been a long while since I dove into the spec, and considering it says that then feel free to disregard my earlier comment.

[ZeroKnight]

That is fair actually — it’s been a long while since I dove into the spec, and considering it says that then feel free to disregard my earlier comment.

Totally understandable, IRCv3 is still evolving and there have been quite a few revisions and errata over the years, from what I've seen. In fact, I just noticed that there's even an errata for this:

Previous versions of this spec did not specify the difference between empty and missing tag values.

With that in mind, pydle was certainly compliant at the time this implementation was originally written. 🙂

[theunkn0wn1]

@Shizmob @ZeroKnight HAve the raised issues been sufficiently addressed or is there an outstanding issue that would block this merge?

[ZeroKnight]

@theunkn0wn1 Everything looks good to me. I went ahead and added a basic test for empty and missing values in addition to the regression tests you added.