[pylint.message] Simplification with 1-1 link between symbol and msgid

Also add a lot of unit tests.

4 files changed, 225 insertions, 293 deletions

diff --git a/pylint/message/message_definition_store.py b/pylint/message/message_definition_store.py
index fdd1f0b54..4d40c25e7 100644
--- a/pylint/message/message_definition_store.py
+++ b/pylint/message/message_definition_store.py
@@ -7,27 +7,27 @@ from __future__ import print_function
 
 import collections
 
-from pylint.exceptions import InvalidMessageError, UnknownMessageError
+from pylint.exceptions import UnknownMessageError
+from pylint.message.message_id_store import MessageIdStore
 
 
 class MessageDefinitionStore:
 
-    """The messages store knows information about every possible message but has
+    """The messages store knows information about every possible message definition but has
     no particular state during analysis.
     """
 
     def __init__(self):
-        # Primary registry for all active messages (i.e. all messages
-        # that can be emitted by pylint for the underlying Python
-        # version). It contains the 1:1 mapping from symbolic names
-        # to message definition objects.
-        # Keys are msg ids, values are a 2-uple with the msg type and the
-        # msg itself
+        self.message_id_store = MessageIdStore()
+        # Primary registry for all active messages definitions.
+        # It contains the 1:1 mapping from msgid to MessageDefinition.
+        # Keys are msgid, values are MessageDefinition
         self._messages_definitions = {}
-        # Maps alternative names (numeric IDs, deprecated names) to
-        # message definitions. May contain several names for each definition
-        # object.
-        self._alternative_names = {}
+        # Secondary registry for all old names kept for compatibility reasons
+        # May contain identical values under different MessageId
+        # (ie a MessageDefinition was renamed more than once)
+        self._old_message_definitions = {}
+        # MessageDefinition kept by category
         self._msgs_by_category = collections.defaultdict(list)
 
     @property
@@ -36,7 +36,7 @@ class MessageDefinitionStore:
         return self._messages_definitions.values()
 
     def register_messages_from_checker(self, checker):
-        """Register all messages from a checker.
+        """Register all messages definitions from a checker.
 
         :param BaseChecker checker:
         """
@@ -49,106 +49,13 @@ class MessageDefinitionStore:
 
         :param MessageDefinition message: The message definition being added.
         """
-        self._check_id_and_symbol_consistency(message.msgid, message.symbol)
-        self._check_symbol(message.msgid, message.symbol)
-        self._check_msgid(message.msgid, message.symbol)
-        for old_name in message.old_names:
-            self._check_symbol(message.msgid, old_name[1])
-        self._messages_definitions[message.symbol] = message
-        self._register_alternative_name(message, message.msgid, message.symbol)
-        for old_id, old_symbol in message.old_names:
-            self._register_alternative_name(message, old_id, old_symbol)
+        self.message_id_store.register_message_definition(message)
+        self._messages_definitions[message.msgid] = message
+        self._old_message_definitions[message.msgid] = message
+        for old_msgid, _ in message.old_names:
+            self._old_message_definitions[old_msgid] = message
         self._msgs_by_category[message.msgid[0]].append(message.msgid)
 
-    def _register_alternative_name(self, msg, msgid, symbol):
-        """helper for register_message()"""
-        self._check_id_and_symbol_consistency(msgid, symbol)
-        self._alternative_names[msgid] = msg
-        self._alternative_names[symbol] = msg
-
-    def _check_symbol(self, msgid, symbol):
-        """Check that a symbol is not already used. """
-        other_message = self._messages_definitions.get(symbol)
-        if other_message:
-            self._raise_duplicate_msgid(symbol, msgid, other_message.msgid)
-        else:
-            alternative_msgid = None
-        alternative_message = self._alternative_names.get(symbol)
-        if alternative_message:
-            if alternative_message.symbol == symbol:
-                alternative_msgid = alternative_message.msgid
-            else:
-                for old_msgid, old_symbol in alternative_message.old_names:
-                    if old_symbol == symbol:
-                        alternative_msgid = old_msgid
-                        break
-            if msgid != alternative_msgid:
-                self._raise_duplicate_msgid(symbol, msgid, alternative_msgid)
-
-    def _check_msgid(self, msgid, symbol):
-        for message in self._messages_definitions.values():
-            if message.msgid == msgid:
-                self._raise_duplicate_symbol(msgid, symbol, message.symbol)
-
-    def _check_id_and_symbol_consistency(self, msgid, symbol):
-        try:
-            alternative = self._alternative_names[msgid]
-        except KeyError:
-            alternative = False
-        try:
-            if not alternative:
-                alternative = self._alternative_names[symbol]
-        except KeyError:
-            # There is no alternative names concerning this msgid/symbol.
-            # So nothing to check
-            return None
-        old_symbolic_name = None
-        old_symbolic_id = None
-        for alternate_msgid, alternate_symbol in alternative.old_names:
-            if alternate_msgid == msgid or alternate_symbol == symbol:
-                old_symbolic_id = alternate_msgid
-                old_symbolic_name = alternate_symbol
-        if symbol not in (alternative.symbol, old_symbolic_name):
-            if msgid == old_symbolic_id:
-                self._raise_duplicate_symbol(msgid, symbol, old_symbolic_name)
-            else:
-                self._raise_duplicate_symbol(msgid, symbol, alternative.symbol)
-        return None
-
-    @staticmethod
-    def _raise_duplicate_symbol(msgid, symbol, other_symbol):
-        """Raise an error when a symbol is duplicated.
-
-        :param str msgid: The msgid corresponding to the symbols
-        :param str symbol: Offending symbol
-        :param str other_symbol: Other offending symbol
-        :raises InvalidMessageError:"""
-        symbols = [symbol, other_symbol]
-        symbols.sort()
-        error_message = "Message id '{msgid}' cannot have both ".format(msgid=msgid)
-        error_message += "'{other_symbol}' and '{symbol}' as symbolic name.".format(
-            other_symbol=symbols[0], symbol=symbols[1]
-        )
-        raise InvalidMessageError(error_message)
-
-    @staticmethod
-    def _raise_duplicate_msgid(symbol, msgid, other_msgid):
-        """Raise an error when a msgid is duplicated.
-
-        :param str symbol: The symbol corresponding to the msgids
-        :param str msgid: Offending msgid
-        :param str other_msgid: Other offending msgid
-        :raises InvalidMessageError:"""
-        msgids = [msgid, other_msgid]
-        msgids.sort()
-        error_message = "Message symbol '{symbol}' cannot be used for ".format(
-            symbol=symbol
-        )
-        error_message += "'{other_msgid}' and '{msgid}' at the same time.".format(
-            other_msgid=msgids[0], msgid=msgids[1]
-        )
-        raise InvalidMessageError(error_message)
-
     def get_message_definitions(self, msgid_or_symbol: str) -> list:
         """Returns the Message object for this message.
         :param str msgid_or_symbol: msgid_or_symbol may be either a numeric or symbolic id.
@@ -156,32 +63,29 @@ class MessageDefinitionStore:
         :rtype: List of MessageDefinition
         :return: A message definition corresponding to msgid_or_symbol
         """
-        # Only msgid can have a digit as second letter
-        is_msgid = msgid_or_symbol[1:].isdigit()
-        if is_msgid:
-            msgid_or_symbol = msgid_or_symbol.upper()
-        for source in (self._alternative_names, self._messages_definitions):
-            try:
-                return [source[msgid_or_symbol]]
-            except KeyError:
-                pass
-        error_msg = "No such message id or symbol '{msgid_or_symbol}'.".format(
-            msgid_or_symbol=msgid_or_symbol
-        )
-        raise UnknownMessageError(error_msg)
-
-    def get_msg_display_string(self, msgid):
+        message_definitions = []
+        message_ids = self.message_id_store.get_active_msgids(msgid_or_symbol)
+        for message_id in message_ids:
+            message_definition = self._messages_definitions.get(message_id)
+            if message_definition is None:
+                message_definition = self._old_message_definitions.get(message_id)
+            message_definitions.append(message_definition)
+        return message_definitions
+
+    def get_msg_display_string(self, msgid_or_symbol: str):
         """Generates a user-consumable representation of a message. """
-        message_definitions = self.get_message_definitions(msgid)
+        message_definitions = self.get_message_definitions(msgid_or_symbol)
         if len(message_definitions) == 1:
             return repr(message_definitions[0].symbol)
         return repr([md.symbol for md in message_definitions])
 
-    def help_message(self, msgids):
+    def help_message(self, msgids_or_symbols: list):
         """Display help messages for the given message identifiers"""
-        for msgid in msgids:
+        for msgids_or_symbol in msgids_or_symbols:
             try:
-                for message_definition in self.get_message_definitions(msgid):
+                for message_definition in self.get_message_definitions(
+                    msgids_or_symbol
+                ):
                     print(message_definition.format_help(checkerref=True))
                     print("")
             except UnknownMessageError as ex:
diff --git a/pylint/message/message_handler_mix_in.py b/pylint/message/message_handler_mix_in.py
index 0750c9ee6..ad8a11f45 100644
--- a/pylint/message/message_handler_mix_in.py
+++ b/pylint/message/message_handler_mix_in.py
@@ -105,10 +105,9 @@ class MessagesHandlerMixIn:
 
         # msgid is a checker name?
         if msgid.lower() in self._checkers:
-            msgs_store = self.msgs_store
             for checker in self._checkers[msgid.lower()]:
                 for _msgid in checker.msgs:
-                    if _msgid in msgs_store._alternative_names:
+                    if _msgid in self.msgs_store._old_message_definitions:
                         self._set_msg_status(_msgid, enable, scope, line)
             return
 
diff --git a/tests/message/unittest_message.py b/tests/message/unittest_message.py
index d11eb3681..205d06dec 100644
--- a/tests/message/unittest_message.py
+++ b/tests/message/unittest_message.py
@@ -3,141 +3,54 @@
 # Licensed under the GPL: https://www.gnu.org/licenses/old-licenses/gpl-2.0.html
 # For details: https://github.com/PyCQA/pylint/blob/master/COPYING
 
-import pytest
-
-from pylint.checkers import BaseChecker
-from pylint.exceptions import InvalidMessageError
-from pylint.message import MessageDefinition, MessageDefinitionStore
-
-
-@pytest.fixture
-def store():
-    return MessageDefinitionStore()
-
-
-@pytest.mark.parametrize(
-    "messages,expected",
-    [
-        (
-            {
-                "W1234": ("message one", "msg-symbol-one", "msg description"),
-                "W4321": ("message two", "msg-symbol-two", "msg description"),
-            },
-            r"Inconsistent checker part in message id 'W4321' (expected 'x12xx' because we already had ['W1234']).",
-        ),
-        (
-            {
-                "W1233": (
-                    "message two",
-                    "msg-symbol-two",
-                    "msg description",
-                    {"old_names": [("W1234", "old-symbol")]},
-                ),
-                "W1234": ("message one", "msg-symbol-one", "msg description"),
-            },
-            "Message id 'W1234' cannot have both 'msg-symbol-one' and 'old-symbol' as symbolic name.",
-        ),
-        (
-            {
-                "W1234": ("message one", "msg-symbol-one", "msg description"),
-                "W1235": (
-                    "message two",
-                    "msg-symbol-two",
-                    "msg description",
-                    {"old_names": [("W1234", "old-symbol")]},
-                ),
-            },
-            "Message id 'W1234' cannot have both 'msg-symbol-one' and 'old-symbol' as symbolic name.",
-        ),
-        (
-            {
-                "W1234": (
-                    "message one",
-                    "msg-symbol-one",
-                    "msg description",
-                    {"old_names": [("W1201", "old-symbol-one")]},
-                ),
-                "W1235": (
-                    "message two",
-                    "msg-symbol-two",
-                    "msg description",
-                    {"old_names": [("W1201", "old-symbol-two")]},
-                ),
-            },
-            "Message id 'W1201' cannot have both 'old-symbol-one' and 'old-symbol-two' as symbolic name.",
-        ),
-        (
-            {
-                "W1234": ("message one", "msg-symbol", "msg description"),
-                "W1235": ("message two", "msg-symbol", "msg description"),
-            },
-            "Message symbol 'msg-symbol' cannot be used for 'W1234' and 'W1235' at the same time.",
-        ),
-        (
-            {
-                "W1233": (
-                    "message two",
-                    "msg-symbol-two",
-                    "msg description",
-                    {"old_names": [("W1230", "msg-symbol-one")]},
-                ),
-                "W1234": ("message one", "msg-symbol-one", "msg description"),
-            },
-            "Message symbol 'msg-symbol-one' cannot be used for 'W1230' and 'W1234' at the same time.",
-        ),
-        (
-            {
-                "W1234": ("message one", "msg-symbol-one", "msg description"),
-                "W1235": (
-                    "message two",
-                    "msg-symbol-two",
-                    "msg description",
-                    {"old_names": [("W1230", "msg-symbol-one")]},
-                ),
-            },
-            "Message symbol 'msg-symbol-one' cannot be used for 'W1234' and 'W1235' at the same time.",
-        ),
-        (
-            {
-                "W1234": (
-                    "message one",
-                    "msg-symbol-one",
-                    "msg description",
-                    {"old_names": [("W1230", "old-symbol-one")]},
-                ),
-                "W1235": (
-                    "message two",
-                    "msg-symbol-two",
-                    "msg description",
-                    {"old_names": [("W1231", "old-symbol-one")]},
-                ),
-            },
-            "Message symbol 'old-symbol-one' cannot be used for 'W1230' and 'W1235' at the same time.",
-        ),
-    ],
-)
-def test_register_error(store, messages, expected):
-    class Checker(BaseChecker):
-        name = "checker"
-        msgs = messages
-
-    with pytest.raises(InvalidMessageError) as cm:
-        store.register_messages_from_checker(Checker())
-    assert str(cm.value) == expected
-
-
-def test_register_error_new_id_duplicate_of_new(store):
-    class CheckerOne(BaseChecker):
-        name = "checker_one"
-        msgs = {"W1234": ("message one", "msg-symbol-one", "msg description.")}
-
-    class CheckerTwo(BaseChecker):
-        name = "checker_two"
-        msgs = {"W1234": ("message two", "msg-symbol-two", "another msg description.")}
-
-    store.register_messages_from_checker(CheckerOne())
-    test_register_error(
-        store,
-        {"W1234": ("message two", "msg-symbol-two", "another msg description.")},
-        "Message id 'W1234' cannot have both 'msg-symbol-one' and 'msg-symbol-two' as symbolic name.",
+from pylint.message import Message
+
+from .generic_fixtures import message_definitions, store
+
+
+def test_new_message(message_definitions):
+    def build_message(message_definition, location_value):
+        return Message(
+            symbol=message_definition.symbol,
+            msg_id=message_definition.msgid,
+            location=[
+                location_value["abspath"],
+                location_value["path"],
+                location_value["module"],
+                location_value["obj"],
+                location_value["line"],
+                location_value["column"],
+            ],
+            msg=message_definition.msg,
+            confidence="high",
+        )
+
+    template = "{path}:{line}:{column}: {msg_id}: {msg} ({symbol})"
+    for message_definition in message_definitions:
+        if message_definition.msgid == "E1234":
+            e1234_message_definition = message_definition
+        if message_definition.msgid == "W1234":
+            w1234_message_definition = message_definition
+    e1234_location_values = {
+        "abspath": "1",
+        "path": "2",
+        "module": "3",
+        "obj": "4",
+        "line": "5",
+        "column": "6",
+    }
+    w1234_location_values = {
+        "abspath": "7",
+        "path": "8",
+        "module": "9",
+        "obj": "10",
+        "line": "11",
+        "column": "12",
+    }
+    expected = (
+        "2:5:6: E1234: Duplicate keyword argument %r in %s call (duplicate-keyword-arg)"
     )
+    e1234 = build_message(e1234_message_definition, e1234_location_values)
+    w1234 = build_message(w1234_message_definition, w1234_location_values)
+    assert e1234.format(template) == expected
+    assert w1234.format(template) == "8:11:12: W1234: message (msg-symbol)"
diff --git a/tests/message/unittest_message_definition_store.py b/tests/message/unittest_message_definition_store.py
index c656781fe..3a4ebf349 100644
--- a/tests/message/unittest_message_definition_store.py
+++ b/tests/message/unittest_message_definition_store.py
@@ -10,32 +10,141 @@ import pytest
 
 from pylint.checkers import BaseChecker
 from pylint.exceptions import InvalidMessageError, UnknownMessageError
-from pylint.message import MessageDefinition, MessageDefinitionStore
+from pylint.message import MessageDefinition
+
+from .generic_fixtures import empty_store, store
+
+
+@pytest.mark.parametrize(
+    "messages,expected",
+    [
+        (
+            {
+                "W1234": ("message one", "msg-symbol-one", "msg description"),
+                "W4321": ("message two", "msg-symbol-two", "msg description"),
+            },
+            r"Inconsistent checker part in message id 'W4321' (expected 'x12xx' because we already had ['W1234']).",
+        ),
+        (
+            {
+                "W1233": (
+                    "message two",
+                    "msg-symbol-two",
+                    "msg description",
+                    {"old_names": [("W1234", "old-symbol")]},
+                ),
+                "W1234": ("message one", "msg-symbol-one", "msg description"),
+            },
+            "Message id 'W1234' cannot have both 'msg-symbol-one' and 'old-symbol' as symbolic name.",
+        ),
+        (
+            {
+                "W1234": ("message one", "msg-symbol-one", "msg description"),
+                "W1235": (
+                    "message two",
+                    "msg-symbol-two",
+                    "msg description",
+                    {"old_names": [("W1234", "old-symbol")]},
+                ),
+            },
+            "Message id 'W1234' cannot have both 'msg-symbol-one' and 'old-symbol' as symbolic name.",
+        ),
+        (
+            {
+                "W1234": (
+                    "message one",
+                    "msg-symbol-one",
+                    "msg description",
+                    {"old_names": [("W1201", "old-symbol-one")]},
+                ),
+                "W1235": (
+                    "message two",
+                    "msg-symbol-two",
+                    "msg description",
+                    {"old_names": [("W1201", "old-symbol-two")]},
+                ),
+            },
+            "Message id 'W1201' cannot have both 'old-symbol-one' and 'old-symbol-two' as symbolic name.",
+        ),
+        (
+            {
+                "W1234": ("message one", "msg-symbol", "msg description"),
+                "W1235": ("message two", "msg-symbol", "msg description"),
+            },
+            "Message symbol 'msg-symbol' cannot be used for 'W1234' and 'W1235' at the same time. "
+            "If you're creating an 'old_names' use 'old-msg-symbol' as the old symbol.",
+        ),
+        (
+            {
+                "W1233": (
+                    "message two",
+                    "msg-symbol-two",
+                    "msg description",
+                    {"old_names": [("W1230", "msg-symbol-one")]},
+                ),
+                "W1234": ("message one", "msg-symbol-one", "msg description"),
+            },
+            "Message symbol 'msg-symbol-one' cannot be used for 'W1230' and 'W1234' at the same time."
+            " If you're creating an 'old_names' use 'old-msg-symbol-one' as the old symbol.",
+        ),
+        (
+            {
+                "W1234": ("message one", "msg-symbol-one", "msg description"),
+                "W1235": (
+                    "message two",
+                    "msg-symbol-two",
+                    "msg description",
+                    {"old_names": [("W1230", "msg-symbol-one")]},
+                ),
+            },
+            "Message symbol 'msg-symbol-one' cannot be used for 'W1230' and 'W1234' at the same time. "
+            "If you're creating an 'old_names' use 'old-msg-symbol-one' as the old symbol.",
+        ),
+        (
+            {
+                "W1234": (
+                    "message one",
+                    "msg-symbol-one",
+                    "msg description",
+                    {"old_names": [("W1230", "old-symbol-one")]},
+                ),
+                "W1235": (
+                    "message two",
+                    "msg-symbol-two",
+                    "msg description",
+                    {"old_names": [("W1231", "old-symbol-one")]},
+                ),
+            },
+            "Message symbol 'old-symbol-one' cannot be used for 'W1230' and 'W1231' at the same time. "
+            "If you're creating an 'old_names' use 'old-old-symbol-one' as the old symbol.",
+        ),
+    ],
+)
+def test_register_error(empty_store, messages, expected):
+    class Checker(BaseChecker):
+        name = "checker"
+        msgs = messages
 
+    with pytest.raises(InvalidMessageError) as cm:
+        empty_store.register_messages_from_checker(Checker())
+    assert str(cm.value) == expected
 
-@pytest.fixture
-def store():
-    store = MessageDefinitionStore()
 
-    class Checker(BaseChecker):
-        name = "achecker"
-        msgs = {
-            "W1234": (
-                "message",
-                "msg-symbol",
-                "msg description.",
-                {"old_names": [("W0001", "old-symbol")]},
-            ),
-            "E1234": (
-                "Duplicate keyword argument %r in %s call",
-                "duplicate-keyword-arg",
-                "Used when a function call passes the same keyword argument multiple times.",
-                {"maxversion": (2, 6)},
-            ),
-        }
-
-    store.register_messages_from_checker(Checker())
-    return store
+def test_register_error_new_id_duplicate_of_new(empty_store):
+    class CheckerOne(BaseChecker):
+        name = "checker_one"
+        msgs = {"W1234": ("message one", "msg-symbol-one", "msg description.")}
+
+    class CheckerTwo(BaseChecker):
+        name = "checker_two"
+        msgs = {"W1234": ("message two", "msg-symbol-two", "another msg description.")}
+
+    empty_store.register_messages_from_checker(CheckerOne())
+    test_register_error(
+        empty_store,
+        {"W1234": ("message two", "msg-symbol-two", "another msg description.")},
+        "Message id 'W1234' cannot have both 'msg-symbol-one' and 'msg-symbol-two' as symbolic name.",
+    )
 
 
 def test_format_help(capsys, store):
@@ -70,7 +179,14 @@ class TestMessageDefinitionStore(object):
         assert desc == msg.format_help(checkerref=checkerref)
 
     def test_check_message_id(self, store):
-        assert isinstance(store.get_message_definitions("W1234")[0], MessageDefinition)
+        w1234 = store.get_message_definitions("W1234")[0]
+        w0001 = store.get_message_definitions("W0001")[0]
+        e1234 = store.get_message_definitions("E1234")[0]
+        old_symbol = store.get_message_definitions("old-symbol")[0]
+        assert isinstance(w1234, MessageDefinition)
+        assert isinstance(e1234, MessageDefinition)
+        assert w1234 == w0001
+        assert w1234 == old_symbol
         with pytest.raises(UnknownMessageError):
             store.get_message_definitions("YB12")