diff options
| -rw-r--r-- | ChangeLog | 5 | ||||
| -rw-r--r-- | doc/whatsnew/2.0.rst | 7 | ||||
| -rw-r--r-- | pylint/exceptions.py | 8 | ||||
| -rw-r--r-- | pylint/test/unittest_lint.py | 37 | ||||
| -rw-r--r-- | pylint/test/unittest_utils.py | 143 | ||||
| -rw-r--r-- | pylint/utils.py | 59 |
6 files changed, 240 insertions, 19 deletions
@@ -131,6 +131,11 @@ Release date: tba * Added proper exception type inference for 'missing-raises-doc'. + * Added InvalidMessageError exception class to replace asserts in + pylint.utils. + + * More thorough validation in MessagesStore.register_messages() to avoid + one message accidentally overwriting another. What's new in Pylint 1.6.3? =========================== diff --git a/doc/whatsnew/2.0.rst b/doc/whatsnew/2.0.rst index 11aa517a0..000b5843b 100644 --- a/doc/whatsnew/2.0.rst +++ b/doc/whatsnew/2.0.rst @@ -142,6 +142,9 @@ Other Changes applies to variable names, only with an inverse rule: you want long descriptive names for variables with bigger scope, like globals. +* Add ``InvalidMessageError`` exception class and replace ``assert`` in + pylint.utils with ``raise InvalidMessageError``. + Bug fixes ========= @@ -168,6 +171,10 @@ Bug fixes * Fix false positives of ``missing-[raises|params|type]-doc`` due to not recognizing valid keyword synonyms supported by Sphinx. +* More thorough validation in ``MessagesStore.register_messages()`` to detect + conflicts between a new message and any existing message id, symbol, + or ``old_names``. + Removed Changes =============== diff --git a/pylint/exceptions.py b/pylint/exceptions.py new file mode 100644 index 000000000..20c569be6 --- /dev/null +++ b/pylint/exceptions.py @@ -0,0 +1,8 @@ +# 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 + +"""Exception classes raised by various operations within pylint.""" + + +class InvalidMessageError(Exception): + """raised when a message creation, registration or addition is rejected""" diff --git a/pylint/test/unittest_lint.py b/pylint/test/unittest_lint.py index 9c6c6cf93..0b253ddb8 100644 --- a/pylint/test/unittest_lint.py +++ b/pylint/test/unittest_lint.py @@ -22,6 +22,7 @@ from pylint.lint import PyLinter, Run, preprocess_options, \ from pylint.utils import MSG_STATE_SCOPE_CONFIG, MSG_STATE_SCOPE_MODULE, MSG_STATE_CONFIDENCE, \ MessagesStore, PyLintASTWalker, MessageDefinition, FileState, \ build_message_def, tokenize_module, UnknownMessage +from pylint.exceptions import InvalidMessageError from pylint.testutils import TestReporter from pylint.reporters import text from pylint import checkers @@ -444,6 +445,26 @@ class PyLinterTC(unittest.TestCase): ['C: 1: Line too long (1/2)', 'C: 2: Line too long (3/4)'], self.linter.reporter.messages) + def test_addmessage_invalid(self): + self.linter.set_reporter(TestReporter()) + self.linter.open() + self.linter.set_current_module('0123') + + with self.assertRaises(InvalidMessageError) as cm: + self.linter.add_message('line-too-long', args=(1,2)) + self.assertEqual(str(cm.exception), + "Message C0301 must provide line, got None") + + with self.assertRaises(InvalidMessageError) as cm: + self.linter.add_message('line-too-long', line=2, node='fake_node', args=(1, 2)) + self.assertEqual(str(cm.exception), + "Message C0301 must only provide line, got line=2, node=fake_node") + + with self.assertRaises(InvalidMessageError) as cm: + self.linter.add_message('C0321') + self.assertEqual(str(cm.exception), + "Message C0321 must provide Node, got None") + def test_init_hooks_called_before_load_plugins(self): self.assertRaises(RuntimeError, Run, ['--load-plugins', 'unexistant', '--init-hook', 'raise RuntimeError']) @@ -661,12 +682,26 @@ class MessagesStoreTC(unittest.TestCase): self.assertEqual('msg-symbol', self.store.check_message_id('old-bad-name').symbol) + def test_add_renamed_message_invalid(self): + # conflicting message ID + with self.assertRaises(InvalidMessageError) as cm: + self.store.add_renamed_message( + 'W1234', 'old-msg-symbol', 'duplicate-keyword-arg') + self.assertEqual(str(cm.exception), + "Message id 'W1234' is already defined") + # conflicting message symbol + with self.assertRaises(InvalidMessageError) as cm: + self.store.add_renamed_message( + 'W1337', 'msg-symbol', 'duplicate-keyword-arg') + self.assertEqual(str(cm.exception), + "Message symbol 'msg-symbol' is already defined") + def test_renamed_message_register(self): self.assertEqual('msg-symbol', self.store.check_message_id('W0001').symbol) self.assertEqual('msg-symbol', self.store.check_message_id('old-symbol').symbol) - + if __name__ == '__main__': unittest.main() diff --git a/pylint/test/unittest_utils.py b/pylint/test/unittest_utils.py index 7e075f016..7034429bb 100644 --- a/pylint/test/unittest_utils.py +++ b/pylint/test/unittest_utils.py @@ -13,6 +13,7 @@ from pylint import __pkginfo__ from pylint import utils from pylint import interfaces from pylint.checkers.utils import check_messages +from pylint.exceptions import InvalidMessageError class PyLintASTWalkerTest(unittest.TestCase): @@ -84,5 +85,147 @@ class RegexBlacklistTest(unittest.TestCase): self.assertFalse(utils._basename_in_blacklist_re("test_utils.py", patterns)) self.assertFalse(utils._basename_in_blacklist_re("enchilad.py", patterns)) + +class MessagesStoreRegisterMessagesTest(unittest.TestCase): + def setUp(self): + self.store = utils.MessagesStore() + + def test_register_error_inconsistent_checker_id(self): + class Checker(object): + name = 'checker' + msgs = { + 'W1234': ('message one', 'msg-symbol-one', 'msg description'), + 'W4321': ('message two', 'msg-symbol-two', 'msg description'), + } + with self.assertRaises(InvalidMessageError) as cm: + self.store.register_messages(Checker()) + self.assertEqual(str(cm.exception), + r"Inconsistent checker part in message id 'W4321' (expected 'x12xx')") + + def test_register_error_new_id_duplicate_of_new(self): + class CheckerOne(object): + name = 'checker_one' + msgs = { + 'W1234': ('message one', 'msg-symbol-one', 'msg description.'), + } + class CheckerTwo(object): + name = 'checker_two' + msgs = { + 'W1234': ('message two', 'msg-symbol-two', 'another msg description.'), + } + self.store.register_messages(CheckerOne()) + with self.assertRaises(InvalidMessageError) as cm: + self.store.register_messages(CheckerTwo()) + self.assertEqual(str(cm.exception), + "Message id 'W1234' is already defined") + + def test_register_error_new_id_duplicate_of_old(self): + class Checker(object): + name = 'checker' + msgs = { + 'W1233': ('message two', 'msg-symbol-two', 'msg description', + {'old_names': [('W1234', 'old-symbol')]}), + 'W1234': ('message one', 'msg-symbol-one', 'msg description'), + } + with self.assertRaises(InvalidMessageError) as cm: + self.store.register_messages(Checker()) + self.assertEqual(str(cm.exception), + "Message id 'W1234' is already defined") + + def test_register_error_old_id_duplicate_of_new(self): + class Checker(object): + name = 'checker' + msgs = { + 'W1234': ('message one', 'msg-symbol-one', 'msg description'), + 'W1235': ('message two', 'msg-symbol-two', 'msg description', + {'old_names': [('W1234', 'old-symbol')]}), + } + with self.assertRaises(InvalidMessageError) as cm: + self.store.register_messages(Checker()) + self.assertEqual(str(cm.exception), + "Message id 'W1234' is already defined") + + def test_register_error_old_id_duplicate_of_old(self): + class Checker(object): + name = 'checker' + msgs = { + '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')]}), + } + with self.assertRaises(InvalidMessageError) as cm: + self.store.register_messages(Checker()) + self.assertEqual(str(cm.exception), + "Message id 'W1201' is already defined") + + + def test_register_error_new_symbol_duplicate_of_new(self): + class Checker(object): + name = 'checker' + msgs = { + 'W1234': ('message one', 'msg-symbol', 'msg description'), + 'W1235': ('message two', 'msg-symbol', 'msg description'), + } + with self.assertRaises(InvalidMessageError) as cm: + self.store.register_messages(Checker()) + self.assertEqual(str(cm.exception), + "Message symbol 'msg-symbol' is already defined") + + def test_register_error_new_symbol_duplicate_of_old(self): + class Checker(object): + name = 'checker' + msgs = { + 'W1233': ('message two', 'msg-symbol-two', 'msg description', + {'old_names': [('W1230', 'msg-symbol-one')]}), + 'W1234': ('message one', 'msg-symbol-one', 'msg description'), + } + with self.assertRaises(InvalidMessageError) as cm: + self.store.register_messages(Checker()) + self.assertEqual(str(cm.exception), + "Message symbol 'msg-symbol-one' is already defined") + + def test_register_error_old_symbol_duplicate_of_new(self): + class Checker(object): + name = 'checker' + msgs = { + 'W1234': ('message one', 'msg-symbol-one', 'msg description'), + 'W1235': ('message two', 'msg-symbol-two', 'msg description', + {'old_names': [('W1230', 'msg-symbol-one')]}), + } + with self.assertRaises(InvalidMessageError) as cm: + self.store.register_messages(Checker()) + self.assertEqual(str(cm.exception), + "Message symbol 'msg-symbol-one' is already defined") + + def test_register_error_old_symbol_duplicate_of_old(self): + class Checker(object): + name = 'checker' + msgs = { + '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')]}), + } + with self.assertRaises(InvalidMessageError) as cm: + self.store.register_messages(Checker()) + self.assertEqual(str(cm.exception), + "Message alternate name 'old-symbol-one' is already defined") + +class MessageDefinitionTest(unittest.TestCase): + def test_create_invalid_msgid(self): + with self.assertRaises(InvalidMessageError) as cm: + utils.MessageDefinition('checker', 'W12345', + 'msg', 'descr', 'symbol', 'scope') + self.assertEqual(str(cm.exception), + "Invalid message id 'W12345'") + + def test_create_invalid_message_type(self): + with self.assertRaises(InvalidMessageError) as cm: + utils.MessageDefinition('checker', 'Q1234', + 'msg', 'descr', 'symbol', 'scope') + self.assertEqual(str(cm.exception), + "Bad message type Q in 'Q1234'") + if __name__ == '__main__': unittest.main() diff --git a/pylint/utils.py b/pylint/utils.py index b51890d5d..14c464af9 100644 --- a/pylint/utils.py +++ b/pylint/utils.py @@ -26,6 +26,7 @@ from astroid import modutils from pylint.interfaces import IRawChecker, ITokenChecker, UNDEFINED, implements from pylint.reporters.ureports.nodes import Section +from pylint.exceptions import InvalidMessageError class UnknownMessage(Exception): @@ -157,9 +158,11 @@ class MessageDefinition(object): def __init__(self, checker, msgid, msg, descr, symbol, scope, minversion=None, maxversion=None, old_names=None): self.checker = checker - assert len(msgid) == 5, 'Invalid message id %s' % msgid - assert msgid[0] in MSG_TYPES, \ - 'Bad message type %s in %r' % (msgid[0], msgid) + if len(msgid) != 5: + raise InvalidMessageError('Invalid message id %r' % msgid) + if not msgid[0] in MSG_TYPES: + raise InvalidMessageError( + 'Bad message type %s in %r' % (msgid[0], msgid)) self.msgid = msgid self.msg = msg self.descr = descr @@ -375,11 +378,18 @@ class MessagesHandlerMixIn(object): # does not apply to them. if msgid[0] not in _SCOPE_EXEMPT: if msg_info.scope == WarningScope.LINE: - assert node is None and line is not None, ( - 'Message %s must only provide line, got line=%s, node=%s' % (msgid, line, node)) + if line is None: + raise InvalidMessageError( + 'Message %s must provide line, got None' % msgid) + if node is not None: + raise InvalidMessageError( + 'Message %s must only provide line, ' + 'got line=%s, node=%s' % (msgid, line, node)) elif msg_info.scope == WarningScope.NODE: # Node-based warnings may provide an override line. - assert node is not None, 'Message %s must provide Node, got None' + if node is None: + raise InvalidMessageError( + 'Message %s must provide Node, got None' % msgid) if line is None and node is not None: line = node.fromlineno @@ -669,8 +679,8 @@ class MessagesStore(object): """ msg = self.check_message_id(new_symbol) msg.old_names.append((old_id, old_symbol)) - self._alternative_names[old_id] = msg - self._alternative_names[old_symbol] = msg + self._register_alternative_name(msg, old_id) + self._register_alternative_name(msg, old_symbol) def register_messages(self, checker): """register a dictionary of messages @@ -682,23 +692,36 @@ class MessagesStore(object): are the checker id and the two last the message id in this checker """ chkid = None - for msgid, msg_tuple in six.iteritems(checker.msgs): + for msgid, msg_tuple in sorted(six.iteritems(checker.msgs)): msg = build_message_def(checker, msgid, msg_tuple) - assert msg.symbol not in self._messages, \ - 'Message symbol %r is already defined' % msg.symbol # avoid duplicate / malformed ids - assert msg.msgid not in self._alternative_names, \ - 'Message id %r is already defined' % msgid - assert chkid is None or chkid == msg.msgid[1:3], \ - 'Inconsistent checker part in message id %r' % msgid + if msg.symbol in self._messages or msg.symbol in self._alternative_names: + raise InvalidMessageError( + 'Message symbol %r is already defined' % msg.symbol) + if chkid is not None and chkid != msg.msgid[1:3]: + raise InvalidMessageError( + "Inconsistent checker part in message id %r (expected 'x%sxx')" + % (msgid, chkid)) chkid = msg.msgid[1:3] self._messages[msg.symbol] = msg - self._alternative_names[msg.msgid] = msg + self._register_alternative_name(msg, msg.msgid) for old_id, old_symbol in msg.old_names: - self._alternative_names[old_id] = msg - self._alternative_names[old_symbol] = msg + self._register_alternative_name(msg, old_id) + self._register_alternative_name(msg, old_symbol) self._msgs_by_category[msg.msgid[0]].append(msg.msgid) + def _register_alternative_name(self, msg, name): + """helper for register_message()""" + if name in self._messages and self._messages[name] != msg: + raise InvalidMessageError( + 'Message symbol %r is already defined' % name) + if name in self._alternative_names and self._alternative_names[name] != msg: + raise InvalidMessageError( + 'Message %s %r is already defined' % ( + 'id' if len(name) == 5 and name[0] in MSG_TYPES else 'alternate name', + name)) + self._alternative_names[name] = msg + def check_message_id(self, msgid): """returns the Message object for this message. |