QmlJS checks: Warn about extra message suppressions.

Reviewed-by: Fawzi Mohamed
Change-Id: I4038cd863ba80c1719417cd03b755b047f7d8b5e
Reviewed-by: Christian Kamm <christian.d.kamm@nokia.com>

src/libs/qmljs/qmljscheck.cpp

@@ -535,7 +535,11 @@ Check::~Check()
 QList<Message> Check::operator()()
 {
     _messages.clear();
+    scanCommentsForAnnotations();
+
     Node::accept(_doc->ast(), this);
+    warnAboutUnnecessarySuppressions();
+
     return _messages;
 }
 
@@ -1147,28 +1151,15 @@ static bool hasOnlySpaces(const QString &s)
 void Check::addMessage(const Message &message)
 {
     if (message.isValid() && _enabledMessages.contains(message.type)) {
-        // check for 'ignore this message'-type comments
-        const QString &suppressMessage = message.suppressionString();
-        foreach (const SourceLocation &commentLoc, _doc->engine()->comments()) {
-            if (commentLoc.startLine > message.location.startLine)
-                break;
-            if (commentLoc.startLine < message.location.startLine - 1)
-                continue;
-
-            // only look at comments on the previous line if there's only spaces before the comment
-            // note: startColumn is 1-based and *after* the starting // or /*
-            if (commentLoc.startLine == message.location.startLine - 1
-                    && commentLoc.startColumn > 3) {
-                const QString &beforeComment = _doc->source().mid(commentLoc.begin() - commentLoc.startColumn + 1,
-                                                                  commentLoc.startColumn - 3);
-                if (!hasOnlySpaces(beforeComment))
-                    continue;
-            }
-
-            const QString &comment = _doc->source().mid(commentLoc.begin(), commentLoc.length);
-            if (comment.contains(suppressMessage))
+        if (m_disabledMessageTypesByLine.contains(message.location.startLine)) {
+            QList<MessageTypeAndSuppression> &disabledMessages = m_disabledMessageTypesByLine[message.location.startLine];
+            for (int i = 0; i < disabledMessages.size(); ++i) {
+                if (disabledMessages[i].type == message.type) {
+                    disabledMessages[i].wasSuppressed = true;
                     return;
                 }
+            }
+        }
 
         _messages += message;
     }
@@ -1179,6 +1170,58 @@ void Check::addMessage(Type type, const SourceLocation &location, const QString
     addMessage(Message(type, location, arg1, arg2));
 }
 
+void Check::scanCommentsForAnnotations()
+{
+    m_disabledMessageTypesByLine.clear();
+
+    // find all disable annotations
+    const QRegExp disableCommentPattern(QLatin1String("@disable M(\\d+)"));
+    foreach (const SourceLocation &commentLoc, _doc->engine()->comments()) {
+        const QString &comment = _doc->source().mid(commentLoc.begin(), commentLoc.length);
+        int lastOffset = -1;
+        QList<MessageTypeAndSuppression> disabledMessageTypes;
+        forever {
+            lastOffset = disableCommentPattern.indexIn(comment, lastOffset + 1);
+            if (lastOffset == -1)
+                break;
+            MessageTypeAndSuppression entry;
+            entry.type = static_cast<StaticAnalysis::Type>(disableCommentPattern.cap(1).toInt());
+            entry.wasSuppressed = false;
+            entry.suppressionSource = SourceLocation(commentLoc.offset + lastOffset,
+                                                     disableCommentPattern.matchedLength(),
+                                                     commentLoc.startLine,
+                                                     commentLoc.startColumn + lastOffset);
+            disabledMessageTypes += entry;
+        }
+        if (!disabledMessageTypes.isEmpty()) {
+            int appliesToLine = commentLoc.startLine;
+
+            // if the comment is preceded by spaces only, it applies to the next line
+            // note: startColumn is 1-based and *after* the starting // or /*
+            if (commentLoc.startColumn > 3) {
+                const QString &beforeComment = _doc->source().mid(commentLoc.begin() - commentLoc.startColumn + 1,
+                                                                  commentLoc.startColumn - 3);
+                if (hasOnlySpaces(beforeComment))
+                    ++appliesToLine;
+            }
+
+            m_disabledMessageTypesByLine[appliesToLine] += disabledMessageTypes;
+        }
+    }
+}
+
+void Check::warnAboutUnnecessarySuppressions()
+{
+    QHashIterator< int, QList<MessageTypeAndSuppression> > it(m_disabledMessageTypesByLine);
+    while (it.hasNext()) {
+        it.next();
+        foreach (const MessageTypeAndSuppression &entry, it.value()) {
+            if (!entry.wasSuppressed)
+                addMessage(WarnUnnecessaryMessageSuppression, entry.suppressionSource);
+        }
+    }
+}
+
 bool Check::visit(NewExpression *ast)
 {
     checkNewExpression(ast->expression);

src/libs/qmljs/qmljscheck.h

@@ -119,6 +119,9 @@ private:
     void addMessage(StaticAnalysis::Type type, const AST::SourceLocation &location,
                     const QString &arg1 = QString(), const QString &arg2 = QString());
 
+    void scanCommentsForAnnotations();
+    void warnAboutUnnecessarySuppressions();
+
     AST::Node *parent(int distance = 0);
 
     Document::Ptr _doc;
@@ -135,6 +138,16 @@ private:
     QStack<StringSet> m_idStack;
     QStack<StringSet> m_propertyStack;
 
+    class MessageTypeAndSuppression
+    {
+    public:
+        AST::SourceLocation suppressionSource;
+        StaticAnalysis::Type type;
+        bool wasSuppressed;
+    };
+
+    QHash< int, QList<MessageTypeAndSuppression> > m_disabledMessageTypesByLine;
+
     bool _importsOk;
 };
 

src/libs/qmljs/qmljsstaticanalysismessage.cpp

@@ -118,6 +118,8 @@ StaticAnalysisMessages::StaticAnalysisMessages()
            tr("do not use comma expressions"));
     newMsg(WarnAlreadyFormalParameter, Warning,
            tr("'%1' is already a formal parameter"), 1);
+    newMsg(WarnUnnecessaryMessageSuppression, Warning,
+           tr("unnecessary message suppression"));
     newMsg(WarnAlreadyFunction, Warning,
            tr("'%1' is already a function"), 1);
     newMsg(WarnVarUsedBeforeDeclaration, Warning,

src/libs/qmljs/qmljsstaticanalysismessage.h

@@ -73,6 +73,7 @@ enum Type
     WarnUnreachable = 28,
     WarnWith = 29,
     WarnComma = 30,
+    WarnUnnecessaryMessageSuppression = 31,
     WarnAlreadyFormalParameter = 103,
     WarnAlreadyFunction = 104,
     WarnVarUsedBeforeDeclaration = 105,

tests/auto/qml/codemodel/check/suppression.qml

@@ -0,0 +1,15 @@
+import Qt 4.7
+
+Rectangle {
+    function foo() {
+        a + b // 127 9 13
+        // @disable M127
+        a + b
+        a + b // @disable M127
+
+        // @disable M127 31 12 24
+
+        // @disable M126 31 12 24
+        a + b // 127 9 13
+    }
+}