diff options
Diffstat (limited to 'src/libs/qmljs/qmljscheck.cpp')
-rw-r--r-- | src/libs/qmljs/qmljscheck.cpp | 746 |
1 files changed, 448 insertions, 298 deletions
diff --git a/src/libs/qmljs/qmljscheck.cpp b/src/libs/qmljs/qmljscheck.cpp index 59ddb40333..07a9d24732 100644 --- a/src/libs/qmljs/qmljscheck.cpp +++ b/src/libs/qmljs/qmljscheck.cpp @@ -34,8 +34,11 @@ #include "qmljsbind.h" #include "qmljscontext.h" #include "qmljsevaluate.h" +#include "qmljsutils.h" #include "parser/qmljsast_p.h" +#include <utils/qtcassert.h> + #include <QtCore/QDebug> #include <QtCore/QDir> #include <QtGui/QColor> @@ -43,99 +46,14 @@ using namespace QmlJS; using namespace QmlJS::AST; - -QColor QmlJS::toQColor(const QString &qmlColorString) -{ - QColor color; - if (qmlColorString.size() == 9 && qmlColorString.at(0) == QLatin1Char('#')) { - bool ok; - const int alpha = qmlColorString.mid(1, 2).toInt(&ok, 16); - if (ok) { - QString name(qmlColorString.at(0)); - name.append(qmlColorString.right(6)); - if (QColor::isValidColor(name)) { - color.setNamedColor(name); - color.setAlpha(alpha); - } - } - } else { - if (QColor::isValidColor(qmlColorString)) - color.setNamedColor(qmlColorString); - } - return color; -} - -SourceLocation QmlJS::locationFromRange(const SourceLocation &start, - const SourceLocation &end) -{ - return SourceLocation(start.offset, - end.end() - start.begin(), - start.startLine, - start.startColumn); -} - -SourceLocation QmlJS::fullLocationForQualifiedId(AST::UiQualifiedId *qualifiedId) -{ - SourceLocation start = qualifiedId->identifierToken; - SourceLocation end = qualifiedId->identifierToken; - - for (UiQualifiedId *iter = qualifiedId; iter; iter = iter->next) { - if (iter->identifierToken.isValid()) - end = iter->identifierToken; - } - - return locationFromRange(start, end); -} - - -DiagnosticMessage QmlJS::errorMessage(const AST::SourceLocation &loc, const QString &message) -{ - return DiagnosticMessage(DiagnosticMessage::Error, loc, message); -} - -namespace { -class SharedData -{ -public: - SharedData() - { - validBuiltinPropertyNames.insert(QLatin1String("action")); - validBuiltinPropertyNames.insert(QLatin1String("bool")); - validBuiltinPropertyNames.insert(QLatin1String("color")); - validBuiltinPropertyNames.insert(QLatin1String("date")); - validBuiltinPropertyNames.insert(QLatin1String("double")); - validBuiltinPropertyNames.insert(QLatin1String("enumeration")); - validBuiltinPropertyNames.insert(QLatin1String("font")); - validBuiltinPropertyNames.insert(QLatin1String("int")); - validBuiltinPropertyNames.insert(QLatin1String("list")); - validBuiltinPropertyNames.insert(QLatin1String("point")); - validBuiltinPropertyNames.insert(QLatin1String("real")); - validBuiltinPropertyNames.insert(QLatin1String("rect")); - validBuiltinPropertyNames.insert(QLatin1String("size")); - validBuiltinPropertyNames.insert(QLatin1String("string")); - validBuiltinPropertyNames.insert(QLatin1String("time")); - validBuiltinPropertyNames.insert(QLatin1String("url")); - validBuiltinPropertyNames.insert(QLatin1String("variant")); - validBuiltinPropertyNames.insert(QLatin1String("vector3d")); - validBuiltinPropertyNames.insert(QLatin1String("alias")); - } - - QSet<QString> validBuiltinPropertyNames; -}; -} // anonymous namespace -Q_GLOBAL_STATIC(SharedData, sharedData) - -bool QmlJS::isValidBuiltinPropertyType(const QString &name) -{ - return sharedData()->validBuiltinPropertyNames.contains(name); -} +using namespace QmlJS::StaticAnalysis; namespace { class AssignmentCheck : public ValueVisitor { public: - DiagnosticMessage operator()( + Message operator()( const Document::Ptr &document, const SourceLocation &location, const Value *lhsValue, @@ -143,8 +61,8 @@ public: Node *ast) { _doc = document; - _message = DiagnosticMessage(DiagnosticMessage::Error, location, QString()); _rhsValue = rhsValue; + _location = location; if (ExpressionStatement *expStmt = cast<ExpressionStatement *>(ast)) _ast = expStmt->expression; else @@ -156,23 +74,28 @@ public: return _message; } + void setMessage(Type type) + { + _message = Message(type, _location); + } + virtual void visit(const NumberValue *value) { - if (const QmlEnumValue *enumValue = dynamic_cast<const QmlEnumValue *>(value)) { + if (const QmlEnumValue *enumValue = value_cast<QmlEnumValue>(value)) { if (StringLiteral *stringLiteral = cast<StringLiteral *>(_ast)) { const QString valueName = stringLiteral->value.toString(); if (!enumValue->keys().contains(valueName)) { - _message.message = Check::tr("unknown value for enum"); + setMessage(ErrInvalidEnumValue); } } else if (! _rhsValue->asStringValue() && ! _rhsValue->asNumberValue() - && ! _rhsValue->asUndefinedValue()) { - _message.message = Check::tr("enum value is not a string or number"); + && ! _rhsValue->asUnknownValue()) { + setMessage(ErrEnumValueMustBeStringOrNumber); } } else { if (cast<TrueLiteral *>(_ast) || cast<FalseLiteral *>(_ast)) { - _message.message = Check::tr("numerical value expected"); + setMessage(ErrNumberValueExpected); } } } @@ -184,7 +107,7 @@ public: if (cast<StringLiteral *>(_ast) || cast<NumericLiteral *>(_ast) || (unaryMinus && cast<NumericLiteral *>(unaryMinus->expression))) { - _message.message = Check::tr("boolean value expected"); + setMessage(ErrBooleanValueExpected); } } @@ -196,14 +119,14 @@ public: || (unaryMinus && cast<NumericLiteral *>(unaryMinus->expression)) || cast<TrueLiteral *>(_ast) || cast<FalseLiteral *>(_ast)) { - _message.message = Check::tr("string value expected"); + setMessage(ErrStringValueExpected); } if (value && value->asUrlValue()) { if (StringLiteral *literal = cast<StringLiteral *>(_ast)) { QUrl url(literal->value.toString()); if (!url.isValid() && !url.isEmpty()) { - _message.message = Check::tr("not a valid url"); + setMessage(ErrInvalidUrl); } else { QString fileName = url.toLocalFile(); if (!fileName.isEmpty()) { @@ -212,8 +135,7 @@ public: fileName.prepend(_doc->path()); } if (!QFileInfo(fileName).exists()) { - _message.message = Check::tr("file or directory does not exist"); - _message.kind = DiagnosticMessage::Warning; + setMessage(WarnFileOrDirectoryDoesNotExist); } } } @@ -225,7 +147,7 @@ public: { if (StringLiteral *stringLiteral = cast<StringLiteral *>(_ast)) { if (!toQColor(stringLiteral->value.toString()).isValid()) - _message.message = Check::tr("not a valid color"); + setMessage(ErrInvalidColor); } else { visit((StringValue *)0); } @@ -233,12 +155,13 @@ public: virtual void visit(const AnchorLineValue *) { - if (! (_rhsValue->asAnchorLineValue() || _rhsValue->asUndefinedValue())) - _message.message = Check::tr("expected anchor line"); + if (! (_rhsValue->asAnchorLineValue() || _rhsValue->asUnknownValue())) + setMessage(ErrAnchorLineExpected); } Document::Ptr _doc; - DiagnosticMessage _message; + Message _message; + SourceLocation _location; const Value *_rhsValue; ExpressionNode *_ast; }; @@ -411,11 +334,11 @@ protected: class MarkUnreachableCode : protected ReachesEndCheck { - QList<DiagnosticMessage> _messages; + QList<Message> _messages; bool _emittedWarning; public: - QList<DiagnosticMessage> operator()(Node *ast) + QList<Message> operator()(Node *ast) { _messages.clear(); check(ast); @@ -438,12 +361,12 @@ protected: return; _emittedWarning = true; - DiagnosticMessage message(DiagnosticMessage::Warning, SourceLocation(), Check::tr("unreachable")); + Message message(WarnUnreachable, SourceLocation()); if (Statement *statement = node->statementCast()) - message.loc = locationFromRange(statement->firstSourceLocation(), statement->lastSourceLocation()); + message.location = locationFromRange(statement->firstSourceLocation(), statement->lastSourceLocation()); else if (ExpressionNode *expr = node->expressionCast()) - message.loc = locationFromRange(expr->firstSourceLocation(), expr->lastSourceLocation()); - if (message.loc.isValid()) + message.location = locationFromRange(expr->firstSourceLocation(), expr->lastSourceLocation()); + if (message.isValid()) _messages += message; } }; @@ -451,10 +374,9 @@ protected: class DeclarationsCheck : protected Visitor { public: - QList<DiagnosticMessage> operator()(FunctionExpression *function, Check::Options options) + QList<Message> operator()(FunctionExpression *function) { clear(); - _options = options; for (FormalParameterList *plist = function->formals; plist; plist = plist->next) { if (!plist->name.isEmpty()) _formalParameterNames += plist->name.toString(); @@ -464,10 +386,9 @@ public: return _messages; } - QList<DiagnosticMessage> operator()(Node *node, Check::Options options) + QList<Message> operator()(Node *node) { clear(); - _options = options; Node::accept(node, this); return _messages; } @@ -503,8 +424,8 @@ protected: bool visit(VariableStatement *ast) { - if (_options & Check::WarnDeclarationsNotStartOfFunction && _seenNonDeclarationStatement) { - warning(ast->declarationKindToken, Check::tr("declarations should be at the start of a function")); + if (_seenNonDeclarationStatement) { + addMessage(HintDeclarationsShouldBeAtStartOfFunction, ast->declarationKindToken); } return true; } @@ -515,21 +436,17 @@ protected: return true; const QString &name = ast->name.toString(); - if (_options & Check::WarnDuplicateDeclaration) { - if (_formalParameterNames.contains(name)) { - warning(ast->identifierToken, Check::tr("already a formal parameter")); - } else if (_declaredFunctions.contains(name)) { - warning(ast->identifierToken, Check::tr("already declared as function")); - } else if (_declaredVariables.contains(name)) { - warning(ast->identifierToken, Check::tr("duplicate declaration")); - } + if (_formalParameterNames.contains(name)) { + addMessage(WarnAlreadyFormalParameter, ast->identifierToken, name); + } else if (_declaredFunctions.contains(name)) { + addMessage(WarnAlreadyFunction, ast->identifierToken, name); + } else if (_declaredVariables.contains(name)) { + addMessage(WarnDuplicateDeclaration, ast->identifierToken, name); } if (_possiblyUndeclaredUses.contains(name)) { - if (_options & Check::WarnUseBeforeDeclaration) { - foreach (const SourceLocation &loc, _possiblyUndeclaredUses.value(name)) { - warning(loc, Check::tr("variable is used before being declared")); - } + foreach (const SourceLocation &loc, _possiblyUndeclaredUses.value(name)) { + addMessage(WarnVarUsedBeforeDeclaration, loc, name); } _possiblyUndeclaredUses.remove(name); } @@ -540,8 +457,8 @@ protected: bool visit(FunctionDeclaration *ast) { - if (_options & Check::WarnDeclarationsNotStartOfFunction &&_seenNonDeclarationStatement) { - warning(ast->functionToken, Check::tr("declarations should be at the start of a function")); + if (_seenNonDeclarationStatement) { + addMessage(HintDeclarationsShouldBeAtStartOfFunction, ast->functionToken); } return visit(static_cast<FunctionExpression *>(ast)); @@ -553,22 +470,18 @@ protected: return false; const QString &name = ast->name.toString(); - if (_options & Check::WarnDuplicateDeclaration) { - if (_formalParameterNames.contains(name)) { - warning(ast->identifierToken, Check::tr("already a formal parameter")); - } else if (_declaredVariables.contains(name)) { - warning(ast->identifierToken, Check::tr("already declared as var")); - } else if (_declaredFunctions.contains(name)) { - warning(ast->identifierToken, Check::tr("duplicate declaration")); - } + if (_formalParameterNames.contains(name)) { + addMessage(WarnAlreadyFormalParameter, ast->identifierToken, name); + } else if (_declaredVariables.contains(name)) { + addMessage(WarnAlreadyVar, ast->identifierToken, name); + } else if (_declaredFunctions.contains(name)) { + addMessage(WarnDuplicateDeclaration, ast->identifierToken, name); } if (FunctionDeclaration *decl = cast<FunctionDeclaration *>(ast)) { if (_possiblyUndeclaredUses.contains(name)) { - if (_options & Check::WarnUseBeforeDeclaration) { - foreach (const SourceLocation &loc, _possiblyUndeclaredUses.value(name)) { - warning(loc, Check::tr("function is used before being declared")); - } + foreach (const SourceLocation &loc, _possiblyUndeclaredUses.value(name)) { + addMessage(WarnFunctionUsedBeforeDeclaration, loc, name); } _possiblyUndeclaredUses.remove(name); } @@ -579,13 +492,12 @@ protected: } private: - void warning(const SourceLocation &loc, const QString &message) + void addMessage(Type type, const SourceLocation &loc, const QString &arg1 = QString()) { - _messages.append(DiagnosticMessage(DiagnosticMessage::Warning, loc, message)); + _messages.append(Message(type, loc, arg1)); } - Check::Options _options; - QList<DiagnosticMessage> _messages; + QList<Message> _messages; QStringList _formalParameterNames; QHash<QString, VariableDeclaration *> _declaredVariables; QHash<QString, FunctionDeclaration *> _declaredFunctions; @@ -595,33 +507,52 @@ private: } // end of anonymous namespace - Check::Check(Document::Ptr doc, const ContextPtr &context) : _doc(doc) , _context(context) , _scopeChain(doc, _context) , _scopeBuilder(&_scopeChain) - , _options(WarnDangerousNonStrictEqualityChecks | WarnBlocks | WarnWith - | WarnVoid | WarnCommaExpression | WarnExpressionStatement - | WarnAssignInCondition | WarnUseBeforeDeclaration | WarnDuplicateDeclaration - | WarnCaseWithoutFlowControlEnd | WarnNonCapitalizedNew - | WarnCallsOfCapitalizedFunctions | WarnUnreachablecode - | ErrCheckTypeErrors) , _lastValue(0) + , _importsOk(false) { + const Imports *imports = context->imports(doc.data()); + if (imports && !imports->importFailed()) + _importsOk = true; + + _enabledMessages = Message::allMessageTypes().toSet(); + disableMessage(HintAnonymousFunctionSpacing); + disableMessage(HintDeclareVarsInOneLine); + disableMessage(HintDeclarationsShouldBeAtStartOfFunction); + disableMessage(HintBinaryOperatorSpacing); + disableMessage(HintOneStatementPerLine); + disableMessage(HintExtraParentheses); } Check::~Check() { } -QList<DiagnosticMessage> Check::operator()() +QList<Message> Check::operator()() { _messages.clear(); + scanCommentsForAnnotations(); + Node::accept(_doc->ast(), this); + warnAboutUnnecessarySuppressions(); + return _messages; } +void Check::enableMessage(Type type) +{ + _enabledMessages.insert(type); +} + +void Check::disableMessage(Type type) +{ + _enabledMessages.remove(type); +} + bool Check::preVisit(Node *ast) { _chain.append(ast); @@ -665,11 +596,10 @@ void Check::endVisit(UiObjectInitializer *) void Check::checkProperty(UiQualifiedId *qualifiedId) { - const QString id = Bind::toString(qualifiedId); + const QString id = toString(qualifiedId); if (id.at(0).isLower()) { if (m_propertyStack.top().contains(id)) { - error(fullLocationForQualifiedId(qualifiedId), - Check::tr("properties can only be assigned once")); + addMessage(ErrPropertiesCanOnlyHaveOneBinding, fullLocationForQualifiedId(qualifiedId)); } m_propertyStack.top().insert(id); } @@ -703,35 +633,30 @@ void Check::visitQmlObject(Node *ast, UiQualifiedId *typeId, } bool typeError = false; - const SourceLocation typeErrorLocation = fullLocationForQualifiedId(typeId); - const ObjectValue *prototype = _context->lookupType(_doc.data(), typeId); - if (!prototype) { - typeError = true; - if (_options & ErrCheckTypeErrors) - error(typeErrorLocation, - Check::tr("unknown type")); - } else { - PrototypeIterator iter(prototype, _context); - QList<const ObjectValue *> prototypes = iter.all(); - if (iter.error() != PrototypeIterator::NoError) + if (_importsOk) { + const SourceLocation typeErrorLocation = fullLocationForQualifiedId(typeId); + const ObjectValue *prototype = _context->lookupType(_doc.data(), typeId); + if (!prototype) { typeError = true; - if (_options & ErrCheckTypeErrors) { + addMessage(ErrUnknownComponent, typeErrorLocation); + } else { + PrototypeIterator iter(prototype, _context); + QList<const ObjectValue *> prototypes = iter.all(); + if (iter.error() != PrototypeIterator::NoError) + typeError = true; const ObjectValue *lastPrototype = prototypes.last(); if (iter.error() == PrototypeIterator::ReferenceResolutionError) { if (const QmlPrototypeReference *ref = - dynamic_cast<const QmlPrototypeReference *>(lastPrototype->prototype())) { - error(typeErrorLocation, - Check::tr("could not resolve the prototype %1 of %2").arg( - Bind::toString(ref->qmlTypeName()), lastPrototype->className())); + value_cast<QmlPrototypeReference>(lastPrototype->prototype())) { + addMessage(ErrCouldNotResolvePrototypeOf, typeErrorLocation, + toString(ref->qmlTypeName()), lastPrototype->className()); } else { - error(typeErrorLocation, - Check::tr("could not resolve the prototype of %1").arg( - lastPrototype->className())); + addMessage(ErrCouldNotResolvePrototype, typeErrorLocation, + lastPrototype->className()); } } else if (iter.error() == PrototypeIterator::CycleError) { - error(typeErrorLocation, - Check::tr("prototype cycle, the last non-repeated object is %1").arg( - lastPrototype->className())); + addMessage(ErrPrototypeCycle, typeErrorLocation, + lastPrototype->className()); } } } @@ -762,7 +687,7 @@ bool Check::visit(UiScriptBinding *ast) ExpressionStatement *expStmt = cast<ExpressionStatement *>(ast->statement); if (!expStmt) { - error(loc, Check::tr("expected id")); + addMessage(ErrIdExpected, loc); return false; } @@ -771,19 +696,19 @@ bool Check::visit(UiScriptBinding *ast) id = idExp->name.toString(); } else if (StringLiteral *strExp = cast<StringLiteral *>(expStmt->expression)) { id = strExp->value.toString(); - warning(loc, Check::tr("using string literals for ids is discouraged")); + addMessage(ErrInvalidId, loc); } else { - error(loc, Check::tr("expected id")); + addMessage(ErrIdExpected, loc); return false; } if (id.isEmpty() || (!id.at(0).isLower() && id.at(0) != '_')) { - error(loc, Check::tr("ids must be lower case or start with underscore")); + addMessage(ErrInvalidId, loc); return false; } if (m_idStack.top().contains(id)) { - error(loc, Check::tr("ids must be unique")); + addMessage(ErrDuplicateId, loc); return false; } m_idStack.top().insert(id); @@ -802,9 +727,9 @@ bool Check::visit(UiScriptBinding *ast) const SourceLocation loc = locationFromRange(ast->statement->firstSourceLocation(), ast->statement->lastSourceLocation()); AssignmentCheck assignmentCheck; - DiagnosticMessage message = assignmentCheck(_doc, loc, lhsValue, rhsValue, ast->statement); - if (! message.message.isEmpty()) - _messages += message; + Message message = assignmentCheck(_doc, loc, lhsValue, rhsValue, ast->statement); + if (message.isValid()) + addMessage(message); } checkBindingRhs(ast->statement); @@ -827,66 +752,83 @@ bool Check::visit(UiArrayBinding *ast) bool Check::visit(UiPublicMember *ast) { - // check if the member type is valid - if (!ast->memberType.isEmpty()) { - const QString &name = ast->memberType.toString(); - if (!name.isEmpty() && name.at(0).isLower()) { - if (!isValidBuiltinPropertyType(name)) - error(ast->typeToken, tr("'%1' is not a valid property type").arg(name)); + if (ast->type == UiPublicMember::Property) { + // check if the member type is valid + if (!ast->memberType.isEmpty()) { + const QString &name = ast->memberType.toString(); + if (!name.isEmpty() && name.at(0).isLower()) { + if (!isValidBuiltinPropertyType(name)) + addMessage(ErrInvalidPropertyType, ast->typeToken, name); + } + + // warn about dubious use of var/variant + if (name == QLatin1String("variant") || name == QLatin1String("var")) { + Evaluate evaluator(&_scopeChain); + const Value *init = evaluator(ast->statement); + QString preferedType; + if (init->asNumberValue()) + preferedType = tr("'int' or 'real'"); + if (init->asStringValue()) + preferedType = QLatin1String("'string'"); + if (init->asBooleanValue()) + preferedType = QLatin1String("'bool'"); + if (!preferedType.isEmpty()) + addMessage(HintPreferNonVarPropertyType, ast->typeToken, preferedType); + } } - } - checkBindingRhs(ast->statement); + checkBindingRhs(ast->statement); - _scopeBuilder.push(ast); - Node::accept(ast->statement, this); - Node::accept(ast->binding, this); - _scopeBuilder.pop(); + _scopeBuilder.push(ast); + Node::accept(ast->statement, this); + Node::accept(ast->binding, this); + _scopeBuilder.pop(); + } return false; } -bool Check::visit(IdentifierExpression *ast) +bool Check::visit(IdentifierExpression *) { // currently disabled: too many false negatives return true; - _lastValue = 0; - if (!ast->name.isEmpty()) { - Evaluate evaluator(&_scopeChain); - _lastValue = evaluator.reference(ast); - if (!_lastValue) - error(ast->identifierToken, tr("unknown identifier")); - if (const Reference *ref = value_cast<const Reference *>(_lastValue)) { - _lastValue = _context->lookupReference(ref); - if (!_lastValue) - error(ast->identifierToken, tr("could not resolve")); - } - } - return false; +// _lastValue = 0; +// if (!ast->name.isEmpty()) { +// Evaluate evaluator(&_scopeChain); +// _lastValue = evaluator.reference(ast); +// if (!_lastValue) +// addMessage(ErrUnknownIdentifier, ast->identifierToken); +// if (const Reference *ref = value_cast<Reference>(_lastValue)) { +// _lastValue = _context->lookupReference(ref); +// if (!_lastValue) +// error(ast->identifierToken, tr("could not resolve")); +// } +// } +// return false; } -bool Check::visit(FieldMemberExpression *ast) +bool Check::visit(FieldMemberExpression *) { // currently disabled: too many false negatives return true; - Node::accept(ast->base, this); - if (!_lastValue) - return false; - const ObjectValue *obj = _lastValue->asObjectValue(); - if (!obj) { - error(locationFromRange(ast->base->firstSourceLocation(), ast->base->lastSourceLocation()), - tr("does not have members")); - } - if (!obj || ast->name.isEmpty()) { - _lastValue = 0; - return false; - } - _lastValue = obj->lookupMember(ast->name.toString(), _context); - if (!_lastValue) - error(ast->identifierToken, tr("unknown member")); - return false; +// Node::accept(ast->base, this); +// if (!_lastValue) +// return false; +// const ObjectValue *obj = _lastValue->asObjectValue(); +// if (!obj) { +// error(locationFromRange(ast->base->firstSourceLocation(), ast->base->lastSourceLocation()), +// tr("does not have members")); +// } +// if (!obj || ast->name.isEmpty()) { +// _lastValue = 0; +// return false; +// } +// _lastValue = obj->lookupMember(ast->name.toString(), _context); +// if (!_lastValue) +// error(ast->identifierToken, tr("unknown member")); +// return false; } bool Check::visit(FunctionDeclaration *ast) @@ -896,13 +838,22 @@ bool Check::visit(FunctionDeclaration *ast) bool Check::visit(FunctionExpression *ast) { - DeclarationsCheck bodyCheck; - _messages += bodyCheck(ast, _options); - if (_options & WarnUnreachablecode) { - MarkUnreachableCode unreachableCheck; - _messages += unreachableCheck(ast->body); + if (ast->name.isEmpty()) { + SourceLocation locfunc = ast->functionToken; + SourceLocation loclparen = ast->lparenToken; + if (locfunc.isValid() && loclparen.isValid() + && (locfunc.startLine != loclparen.startLine + || locfunc.end() + 1 != loclparen.begin())) { + addMessage(HintAnonymousFunctionSpacing, locationFromRange(locfunc, loclparen)); + } } + DeclarationsCheck bodyCheck; + addMessages(bodyCheck(ast)); + + MarkUnreachableCode unreachableCheck; + addMessages(unreachableCheck(ast->body)); + Node::accept(ast->formals, this); _scopeBuilder.push(ast); Node::accept(ast->body, this); @@ -912,9 +863,8 @@ bool Check::visit(FunctionExpression *ast) static bool shouldAvoidNonStrictEqualityCheck(const Value *lhs, const Value *rhs) { - // we currently use undefined as a "we don't know" value - if (lhs->asUndefinedValue() || rhs->asUndefinedValue()) - return true; + if (lhs->asUnknownValue() || rhs->asUnknownValue()) + return true; // may coerce or not if (lhs->asStringValue() && rhs->asNumberValue()) return true; // coerces string to number @@ -925,7 +875,8 @@ static bool shouldAvoidNonStrictEqualityCheck(const Value *lhs, const Value *rhs if (lhs->asObjectValue() && rhs->asStringValue()) return true; // coerces object to primitive - if (lhs->asBooleanValue() && !rhs->asBooleanValue()) + if (lhs->asBooleanValue() && (!rhs->asBooleanValue() + && !rhs->asUndefinedValue())) return true; // coerces bool to number return false; @@ -933,27 +884,61 @@ static bool shouldAvoidNonStrictEqualityCheck(const Value *lhs, const Value *rhs bool Check::visit(BinaryExpression *ast) { + const QString source = _doc->source(); + + // check spacing + SourceLocation op = ast->operatorToken; + if ((op.begin() > 0 && !source.at(op.begin() - 1).isSpace()) + || (int(op.end()) < source.size() && !source.at(op.end()).isSpace())) { + addMessage(HintBinaryOperatorSpacing, op); + } + + // check ==, != if (ast->op == QSOperator::Equal || ast->op == QSOperator::NotEqual) { - bool warn = _options & WarnAllNonStrictEqualityChecks; - if (!warn && _options & WarnDangerousNonStrictEqualityChecks) { - Evaluate eval(&_scopeChain); - const Value *lhs = eval(ast->left); - const Value *rhs = eval(ast->right); - warn = shouldAvoidNonStrictEqualityCheck(lhs, rhs) - || shouldAvoidNonStrictEqualityCheck(rhs, lhs); + Evaluate eval(&_scopeChain); + const Value *lhsValue = eval(ast->left); + const Value *rhsValue = eval(ast->right); + if (shouldAvoidNonStrictEqualityCheck(lhsValue, rhsValue) + || shouldAvoidNonStrictEqualityCheck(rhsValue, lhsValue)) { + addMessage(MaybeWarnEqualityTypeCoercion, ast->operatorToken); } - if (warn) { - warning(ast->operatorToken, tr("== and != perform type coercion, use === or !== instead to avoid")); + } + + // check odd + ++ combinations + const QLatin1Char newline('\n'); + if (ast->op == QSOperator::Add || ast->op == QSOperator::Sub) { + QChar match; + Type msg; + if (ast->op == QSOperator::Add) { + match = QLatin1Char('+'); + msg = WarnConfusingPluses; + } else { + QTC_CHECK(ast->op == QSOperator::Sub); + match = QLatin1Char('-'); + msg = WarnConfusingMinuses; + } + + if (int(op.end()) + 1 < source.size()) { + const QChar next = source.at(op.end()); + if (next.isSpace() && next != newline + && source.at(op.end() + 1) == match) + addMessage(msg, SourceLocation(op.begin(), 3, op.startLine, op.startColumn)); + } + if (op.begin() >= 2) { + const QChar prev = source.at(op.begin() - 1); + if (prev.isSpace() && prev != newline + && source.at(op.begin() - 2) == match) + addMessage(msg, SourceLocation(op.begin() - 2, 3, op.startLine, op.startColumn - 2)); } } + return true; } bool Check::visit(Block *ast) { if (Node *p = parent()) { - if (_options & WarnBlocks - && !cast<UiScriptBinding *>(p) + if (!cast<UiScriptBinding *>(p) && !cast<UiPublicMember *>(p) && !cast<TryStatement *>(p) && !cast<Catch *>(p) @@ -967,12 +952,12 @@ bool Check::visit(Block *ast) && !cast<IfStatement *>(p) && !cast<SwitchStatement *>(p) && !cast<WithStatement *>(p)) { - warning(ast->lbraceToken, tr("blocks do not introduce a new scope, avoid")); + addMessage(WarnBlock, ast->lbraceToken); } if (!ast->statements - && cast<UiPublicMember *>(p)) { - warning(locationFromRange(ast->firstSourceLocation(), ast->lastSourceLocation()), - tr("unintentional empty block, use ({}) for empty object literal")); + && cast<UiPublicMember *>(p) + && ast->lbraceToken.startLine == ast->rbraceToken.startLine) { + addMessage(WarnUnintentinalEmptyBlock, locationFromRange(ast->firstSourceLocation(), ast->lastSourceLocation())); } } return true; @@ -980,25 +965,23 @@ bool Check::visit(Block *ast) bool Check::visit(WithStatement *ast) { - if (_options & WarnWith) - warning(ast->withToken, tr("use of the with statement is not recommended, use a var instead")); + addMessage(WarnWith, ast->withToken); return true; } bool Check::visit(VoidExpression *ast) { - if (_options & WarnVoid) - warning(ast->voidToken, tr("use of void is usually confusing and not recommended")); + addMessage(WarnVoid, ast->voidToken); return true; } bool Check::visit(Expression *ast) { - if (_options & WarnCommaExpression && ast->left && ast->right) { + if (ast->left && ast->right) { Node *p = parent(); if (!cast<ForStatement *>(p) && !cast<LocalForStatement *>(p)) { - warning(ast->commaToken, tr("avoid comma expressions")); + addMessage(WarnComma, ast->commaToken); } } return true; @@ -1006,7 +989,7 @@ bool Check::visit(Expression *ast) bool Check::visit(ExpressionStatement *ast) { - if (_options & WarnExpressionStatement && ast->expression) { + if (ast->expression) { bool ok = cast<CallExpression *>(ast->expression) || cast<DeleteExpression *>(ast->expression) || cast<PreDecrementExpression *>(ast->expression) @@ -1050,8 +1033,8 @@ bool Check::visit(ExpressionStatement *ast) } if (!ok) { - warning(locationFromRange(ast->firstSourceLocation(), ast->lastSourceLocation()), - tr("expression statements should be assignments, calls or delete expressions only")); + addMessage(WarnConfusingExpressionStatement, + locationFromRange(ast->firstSourceLocation(), ast->lastSourceLocation())); } } return true; @@ -1122,14 +1105,12 @@ static QString functionName(ExpressionNode *ast, SourceLocation *location) void Check::checkNewExpression(ExpressionNode *ast) { - if (!(_options & WarnNonCapitalizedNew)) - return; SourceLocation location; const QString name = functionName(ast, &location); if (name.isEmpty()) return; if (!name.at(0).isUpper()) { - warning(location, tr("'new' should only be used with functions that start with an uppercase letter")); + addMessage(WarnNewWithLowercaseFunction, location); } } @@ -1139,10 +1120,110 @@ void Check::checkBindingRhs(Statement *statement) return; DeclarationsCheck bodyCheck; - _messages += bodyCheck(statement, _options); - if (_options & WarnUnreachablecode) { - MarkUnreachableCode unreachableCheck; - _messages += unreachableCheck(statement); + addMessages(bodyCheck(statement)); + + MarkUnreachableCode unreachableCheck; + addMessages(unreachableCheck(statement)); +} + +void Check::checkExtraParentheses(ExpressionNode *expression) +{ + if (NestedExpression *nested = cast<NestedExpression *>(expression)) { + addMessage(HintExtraParentheses, nested->lparenToken); + } +} + +void Check::addMessages(const QList<Message> &messages) +{ + foreach (const Message &msg, messages) + addMessage(msg); +} + +static bool hasOnlySpaces(const QString &s) +{ + for (int i = 0; i < s.size(); ++i) + if (!s.at(i).isSpace()) + return false; + return true; +} + +void Check::addMessage(const Message &message) +{ + if (message.isValid() && _enabledMessages.contains(message.type)) { + 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; + } +} + +void Check::addMessage(Type type, const SourceLocation &location, const QString &arg1, const QString &arg2) +{ + addMessage(Message(type, location, arg1, arg2)); +} + +void Check::scanCommentsForAnnotations() +{ + m_disabledMessageTypesByLine.clear(); + const QRegExp disableCommentPattern(Message::suppressionPattern()); + + foreach (const SourceLocation &commentLoc, _doc->engine()->comments()) { + const QString &comment = _doc->source().mid(commentLoc.begin(), commentLoc.length); + + // enable all checks annotation + if (comment.contains(QLatin1String("@enable-all-checks"))) { + _enabledMessages = Message::allMessageTypes().toSet(); + } + + // find all disable annotations + 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); + } } } @@ -1155,19 +1236,101 @@ bool Check::visit(NewExpression *ast) bool Check::visit(NewMemberExpression *ast) { checkNewExpression(ast->base); + + // check for Number, Boolean, etc constructor usage + if (IdentifierExpression *idExp = cast<IdentifierExpression *>(ast->base)) { + const QStringRef name = idExp->name; + if (name == QLatin1String("Number")) { + addMessage(WarnNumberConstructor, idExp->identifierToken); + } else if (name == QLatin1String("Boolean")) { + addMessage(WarnBooleanConstructor, idExp->identifierToken); + } else if (name == QLatin1String("String")) { + addMessage(WarnStringConstructor, idExp->identifierToken); + } else if (name == QLatin1String("Object")) { + addMessage(WarnObjectConstructor, idExp->identifierToken); + } else if (name == QLatin1String("Array")) { + bool ok = false; + if (ast->arguments && ast->arguments->expression && !ast->arguments->next) { + Evaluate evaluate(&_scopeChain); + const Value *arg = evaluate(ast->arguments->expression); + if (arg->asNumberValue() || arg->asUnknownValue()) + ok = true; + } + if (!ok) + addMessage(WarnArrayConstructor, idExp->identifierToken); + } else if (name == QLatin1String("Function")) { + addMessage(WarnFunctionConstructor, idExp->identifierToken); + } + } + return true; } bool Check::visit(CallExpression *ast) { // check for capitalized function name being called - if (_options & WarnCallsOfCapitalizedFunctions) { - SourceLocation location; - const QString name = functionName(ast->base, &location); - if (!name.isEmpty() && name.at(0).isUpper()) { - warning(location, tr("calls of functions that start with an uppercase letter should use 'new'")); + SourceLocation location; + const QString name = functionName(ast->base, &location); + if (!name.isEmpty() && name.at(0).isUpper() + && name != QLatin1String("String") + && name != QLatin1String("Boolean") + && name != QLatin1String("Date") + && name != QLatin1String("Number") + && name != QLatin1String("Object")) { + addMessage(WarnExpectedNewWithUppercaseFunction, location); + } + if (cast<IdentifierExpression *>(ast->base) && name == QLatin1String("eval")) + addMessage(WarnEval, location); + return true; +} + +bool Check::visit(StatementList *ast) +{ + SourceLocation warnStart; + SourceLocation warnEnd; + unsigned currentLine = 0; + for (StatementList *it = ast; it; it = it->next) { + if (!it->statement) + continue; + const SourceLocation itLoc = it->statement->firstSourceLocation(); + if (itLoc.startLine != currentLine) { // first statement on a line + if (warnStart.isValid()) + addMessage(HintOneStatementPerLine, locationFromRange(warnStart, warnEnd)); + warnStart = SourceLocation(); + currentLine = itLoc.startLine; + } else { // other statements on the same line + if (!warnStart.isValid()) + warnStart = itLoc; + warnEnd = it->statement->lastSourceLocation(); } } + if (warnStart.isValid()) + addMessage(HintOneStatementPerLine, locationFromRange(warnStart, warnEnd)); + + return true; +} + +bool Check::visit(ReturnStatement *ast) +{ + checkExtraParentheses(ast->expression); + return true; +} + +bool Check::visit(ThrowStatement *ast) +{ + checkExtraParentheses(ast->expression); + return true; +} + +bool Check::visit(DeleteExpression *ast) +{ + checkExtraParentheses(ast->expression); + return true; +} + +bool Check::visit(TypeOfExpression *ast) +{ + checkExtraParentheses(ast->expression); return true; } @@ -1176,6 +1339,9 @@ bool Check::visit(CallExpression *ast) /// ### Maybe put this into the context as a helper method. const Value *Check::checkScopeObjectMember(const UiQualifiedId *id) { + if (!_importsOk) + return 0; + QList<const ObjectValue *> scopeObjects = _scopeChain.qmlScopeObjects(); if (scopeObjects.isEmpty()) return 0; @@ -1210,8 +1376,7 @@ const Value *Check::checkScopeObjectMember(const UiQualifiedId *id) break; } if (!value) { - error(id->identifierToken, - Check::tr("'%1' is not a valid property name").arg(propertyName)); + addMessage(ErrInvalidPropertyName, id->identifierToken, propertyName); return 0; } @@ -1226,10 +1391,9 @@ const Value *Check::checkScopeObjectMember(const UiQualifiedId *id) // member lookup const UiQualifiedId *idPart = id; while (idPart->next) { - const ObjectValue *objectValue = value_cast<const ObjectValue *>(value); + const ObjectValue *objectValue = value_cast<ObjectValue>(value); if (! objectValue) { - error(idPart->identifierToken, - Check::tr("'%1' does not have members").arg(propertyName)); + addMessage(ErrDoesNotHaveMembers, idPart->identifierToken, propertyName); return 0; } @@ -1244,9 +1408,7 @@ const Value *Check::checkScopeObjectMember(const UiQualifiedId *id) value = objectValue->lookupMember(propertyName, _context); if (! value) { - error(idPart->identifierToken, - Check::tr("'%1' is not a member of '%2'").arg( - propertyName, objectValue->className())); + addMessage(ErrInvalidMember, idPart->identifierToken, propertyName, objectValue->className()); return 0; } } @@ -1256,35 +1418,23 @@ const Value *Check::checkScopeObjectMember(const UiQualifiedId *id) void Check::checkAssignInCondition(AST::ExpressionNode *condition) { - if (_options & WarnAssignInCondition) { - if (BinaryExpression *binary = cast<BinaryExpression *>(condition)) { - if (binary->op == QSOperator::Assign) - warning(binary->operatorToken, tr("avoid assignments in conditions")); - } + if (BinaryExpression *binary = cast<BinaryExpression *>(condition)) { + if (binary->op == QSOperator::Assign) + addMessage(WarnAssignmentInCondition, binary->operatorToken); } } void Check::checkEndsWithControlFlow(StatementList *statements, SourceLocation errorLoc) { - if (!statements || !(_options & WarnCaseWithoutFlowControlEnd)) + if (!statements) return; ReachesEndCheck check; if (check(statements)) { - warning(errorLoc, tr("case is not terminated and not empty")); + addMessage(WarnCaseWithoutFlowControl, errorLoc); } } -void Check::error(const AST::SourceLocation &loc, const QString &message) -{ - _messages.append(DiagnosticMessage(DiagnosticMessage::Error, loc, message)); -} - -void Check::warning(const AST::SourceLocation &loc, const QString &message) -{ - _messages.append(DiagnosticMessage(DiagnosticMessage::Warning, loc, message)); -} - Node *Check::parent(int distance) { const int index = _chain.size() - 2 - distance; |