qiurui
/
qtdeclarative
mirror of https://github.com/qt/qtdeclarative.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

qmllint: Use type propagator to provide unqualfied access warnings 

Do not rely on checkidentifiers to provide unqualified access warnings anymore. Using the type propagator ought to be more accurate and will help remove the relatively hacky checkidentifiers code completely later on.

Change-Id: I40cc040b9455962abbd2ec84cdb494fcec1ab79d
Reviewed-by: Andrei Golubev <andrei.golubev@qt.io>
Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io>
This commit is contained in:
Maximilian Goldstein 2021-07-09 10:42:51 +02:00
parent 5e01411c1e
commit 2eb6d20246
9 changed files with 165 additions and 95 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
src/qmlcompiler/qqmljsimportvisitor_p.h
View File

@ -67,6 +67,10 @@ public:
QHash<QString, QQmlJSScope::ConstPtr> imports() const { return m_rootScopeImports; }
QHash<QString, QQmlJSScope::ConstPtr> addressableScopes() const { return m_scopesById; }
QHash<QQmlJS::SourceLocation, QQmlJSMetaSignalHandler> signalHandlers() const
{
return m_signalHandlers;
}
static QString implicitImportDirectory(
const QString &localFile, QQmlJSResourceFileMapper *mapper);

123
src/qmlcompiler/qqmljstypepropagator.cpp
View File

@ -42,10 +42,13 @@ QQmlJSTypePropagator::~QQmlJSTypePropagator() { }
QQmlJSTypePropagator::TypePropagationResult QQmlJSTypePropagator::propagateTypes(
const QV4::Compiler::Context *context, const QQmlJS::AST::BoundNames &arguments,
const QQmlJSScope::ConstPtr &returnType, const QQmlJSScope::ConstPtr &qmlScope,
bool isSignalHandler, Error *error)
const QHash<QString, QQmlJSScope::ConstPtr> &addressableScopes, bool isSignalHandler,
Error *error)
{
m_error = error;
m_currentScope = qmlScope;
m_currentContext = context;
m_addressableScopes = addressableScopes;
m_arguments = arguments;
m_returnType = m_typeResolver->globalType(returnType);
m_isSignalHandler = isSignalHandler;
@ -234,13 +237,127 @@ void QQmlJSTypePropagator::generate_LoadGlobalLookup(int index)
generate_LoadName(m_jsUnitGenerator->lookupNameIndex(index));
}
void QQmlJSTypePropagator::handleUnqualifiedAccess(const QString &name)
{
Q_ASSERT(m_currentContext->sourceLocationTable);
const auto &entries = m_currentContext->sourceLocationTable->entries;
auto item = std::lower_bound(entries.begin(), entries.end(), currentInstructionOffset(),
[](auto entry, uint offset) { return entry.offset < offset; });
Q_ASSERT(item != entries.end());
auto location = item->location;
if (m_currentScope->isInCustomParserParent()) {
Q_ASSERT(!m_currentScope->baseType().isNull());
// Only ignore custom parser based elements if it's not Connections.
if (m_currentScope->baseType()->internalName() != u"QQmlConnections"_qs)
return;
}
m_logger->logWarning(QLatin1String("Unqualified access"), Log_UnqualifiedAccess, location);
auto childScopes = m_currentScope->childScopes();
for (qsizetype i = 0; i < m_currentScope->childScopes().length(); i++) {
auto &scope = childScopes[i];
if (location.offset > scope->sourceLocation().offset) {
if (i + 1 < childScopes.length()
&& childScopes.at(i + 1)->sourceLocation().offset < location.offset)
continue;
if (scope->childScopes().length() == 0)
continue;
const auto jsId = scope->childScopes().first()->findJSIdentifier(name);
if (jsId.has_value() && jsId->kind == QQmlJSScope::JavaScriptIdentifier::Injected) {
FixSuggestion suggestion { Log_UnqualifiedAccess, QtInfoMsg, {} };
const QQmlJSScope::JavaScriptIdentifier id = jsId.value();
QQmlJS::SourceLocation fixLocation = id.location;
Q_UNUSED(fixLocation)
fixLocation.length = 0;
const auto handler = m_typeResolver->signalHandlers()[id.location];
QString fixString = handler.isMultiline ? u" function("_qs : u" ("_qs;
const auto parameters = handler.signalParameters;
for (int numParams = parameters.size(); numParams > 0; --numParams) {
fixString += parameters.at(parameters.size() - numParams);
if (numParams > 1)
fixString += u", "_qs;
}
fixString += handler.isMultiline ? u") "_qs : u") => "_qs;
suggestion.fixes << FixSuggestion::Fix {
name
+ QString::fromLatin1(" is accessible in this scope because "
"you are handling a signal at %1:%2\n")
.arg(id.location.startLine)
.arg(id.location.startColumn),
QtInfoMsg, fixLocation, fixString
};
m_logger->suggestFix(suggestion);
}
break;
}
}
for (QQmlJSScope::ConstPtr scope = m_currentScope; !scope.isNull();
scope = scope->parentScope()) {
if (scope->hasProperty(name)) {
auto it = std::find_if(m_addressableScopes.constBegin(), m_addressableScopes.constEnd(),
[&](const QQmlJSScope::ConstPtr ptr) { return ptr == scope; });
FixSuggestion suggestion { Log_UnqualifiedAccess, QtInfoMsg, {} };
QQmlJS::SourceLocation fixLocation = location;
fixLocation.length = 0;
QString id = it == m_addressableScopes.constEnd() ? u"<id>"_qs : it.key();
suggestion.fixes << FixSuggestion::Fix {
name + QLatin1String(" is a member of a parent element\n")
+ QLatin1String(" You can qualify the access with its id "
"to avoid this warning:\n"),
QtInfoMsg, fixLocation, id + u"."_qs
};
if (it == m_addressableScopes.constEnd()) {
suggestion.fixes << FixSuggestion::Fix {
u"You first have to give the element an id"_qs,
QtInfoMsg,
QQmlJS::SourceLocation {},
{}
};
}
m_logger->suggestFix(suggestion);
}
}
}
void QQmlJSTypePropagator::generate_LoadQmlContextPropertyLookup(int index)
{
const QString name =
m_jsUnitGenerator->stringForIndex(m_jsUnitGenerator->lookupNameIndex(index));
m_state.accumulatorOut = m_typeResolver->scopedType(m_currentScope, name);
m_state.accumulatorOut = m_typeResolver->scopedType(m_currentScope, m_state.savedPrefix + name);
if (!m_state.accumulatorOut.isValid() && m_typeResolver->isPrefix(name)) {
m_state.savedPrefix = name + u"."_qs;
m_state.accumulatorOut = m_state.accumulatorIn.isValid()
? m_state.accumulatorIn
: m_typeResolver->globalType(m_currentScope);
return;
}
m_state.savedPrefix.clear();
if (!m_state.accumulatorOut.isValid()) {
setError(u"Cannot access value for name "_qs + name);
setError(u"Cannot access value for name "_qs + name, true);
handleUnqualifiedAccess(name);
} else if (m_typeResolver->genericType(m_state.accumulatorOut.storedType()).isNull()) {
// It should really be valid.
// We get the generic type from aotContext->loadQmlContextPropertyIdLookup().

19
src/qmlcompiler/qqmljstypepropagator_p.h
View File

@ -61,6 +61,7 @@ struct QQmlJSTypePropagator : public QV4::Moth::ByteCodeHandler
{
QString message;
int instructionOffset;
bool hasLoggerMessage;
bool isSet() const { return !message.isEmpty(); }
};
@ -72,11 +73,11 @@ struct QQmlJSTypePropagator : public QV4::Moth::ByteCodeHandler
using TypePropagationResult = QHash<int, InstructionAnnotation>;
TypePropagationResult propagateTypes(const QV4::Compiler::Context *context,
const QQmlJS::AST::BoundNames &arguments,
const QQmlJSScope::ConstPtr &returnType,
const QQmlJSScope::ConstPtr &qmlScope,
bool isSignalHandler, Error *error);
TypePropagationResult
propagateTypes(const QV4::Compiler::Context *context, const QQmlJS::AST::BoundNames &arguments,
const QQmlJSScope::ConstPtr &returnType, const QQmlJSScope::ConstPtr &qmlScope,
const QHash<QString, QQmlJSScope::ConstPtr> &addressableScopes,
bool isSignalHandler, Error *error);
void generate_Ret() override;
void generate_Debug() override;
@ -223,9 +224,10 @@ struct QQmlJSTypePropagator : public QV4::Moth::ByteCodeHandler
};
private:
void setError(const QString &message)
void setError(const QString &message, bool hasLoggerMessage = false)
{
m_error->message = message;
m_error->hasLoggerMessage = hasLoggerMessage;
m_error->instructionOffset = currentInstructionOffset();
}
@ -243,6 +245,7 @@ private:
QQmlJSVirtualRegisters registers;
QQmlJSRegisterContent accumulatorIn;
QQmlJSRegisterContent accumulatorOut;
QString savedPrefix;
QHash<int, InstructionAnnotation> annotations;
@ -257,6 +260,8 @@ private:
bool needsMorePasses = false;
};
void handleUnqualifiedAccess(const QString &name);
void propagateBinaryOperation(QSOperator::Op op, int lhs);
void propagateCall(const QList<QQmlJSMetaMethod> &methods, int argc, int argv);
void propagatePropertyLookup(const QString &name);
@ -283,6 +288,8 @@ private:
QV4::Compiler::JSUnitGenerator *m_jsUnitGenerator = nullptr;
QQmlJSScope::ConstPtr m_currentScope;
const QV4::Compiler::Context *m_currentContext;
QHash<QString, QQmlJSScope::ConstPtr> m_addressableScopes;
QQmlJSLogger *m_logger;
// Not part of the state, as the back jumps are the reason for running multiple passes

1
src/qmlcompiler/qqmljstyperesolver.cpp
View File

@ -100,6 +100,7 @@ QQmlJSTypeResolver::QQmlJSTypeResolver(
QQueue<QQmlJSScope::Ptr> objects;
objects.enqueue(visitor.result());
m_objectsById = visitor.addressableScopes();
m_signalHandlers = visitor.signalHandlers();
while (!objects.isEmpty()) {
const QQmlJSScope::Ptr object = objects.dequeue();

9
src/qmlcompiler/qqmljstyperesolver_p.h
View File

@ -95,6 +95,8 @@ public:
QQmlJSScope::ConstPtr scopeForLocation(const QV4::CompiledData::Location &location) const;
QQmlJSScope::ConstPtr scopeForId(const QString &id) const;
bool isPrefix(const QString &name) { return m_imports.contains(name) && !m_imports[name]; }
QQmlJSScope::ConstPtr typeForName(const QString &name) const { return m_imports[name]; }
QQmlJSScope::ConstPtr typeFromAST(QQmlJS::AST::Type *type) const;
QQmlJSScope::ConstPtr typeForConst(QV4::ReturnedValue rv) const;
@ -136,6 +138,12 @@ public:
storedType(const QQmlJSScope::ConstPtr &type,
ComponentIsGeneric allowComponent = ComponentIsGeneric::No) const;
const QHash<QString, QQmlJSScope::ConstPtr> &objectsById() { return m_objectsById; }
const QHash<QQmlJS::SourceLocation, QQmlJSMetaSignalHandler> &signalHandlers()
{
return m_signalHandlers;
}
private:
QQmlJSScope::ConstPtr merge(const QQmlJSScope::ConstPtr &a,
const QQmlJSScope::ConstPtr &b) const;
@ -171,6 +179,7 @@ private:
QHash<QString, QQmlJSScope::ConstPtr> m_objectsById;
QHash<QV4::CompiledData::Location, QQmlJSScope::ConstPtr> m_objectsByLocation;
QHash<QString, QQmlJSScope::ConstPtr> m_imports;
QHash<QQmlJS::SourceLocation, QQmlJSMetaSignalHandler> m_signalHandlers;
TypeStorage m_typeStorage = Direct;
Semantics m_semantics = Dynamic;

5
tests/auto/qml/qmllint/tst_qmllint.cpp
View File

@ -128,8 +128,6 @@ void TestQmllint::testUnqualified_data()
QTest::addColumn<int>("warningLine");
QTest::addColumn<int>("warningColumn");
// check for false positive due to and warning about with statement
QTest::newRow("WithStatement") << QStringLiteral("WithStatement.qml") << QStringLiteral("with statements are strongly discouraged") << 10 << 25;
// id from nowhere (as with setContextProperty)
QTest::newRow("IdFromOuterSpaceDirect") << QStringLiteral("IdFromOuterSpace.qml") << "alien.x" << 4 << 8;
QTest::newRow("IdFromOuterSpaceAccess") << QStringLiteral("IdFromOuterSpace.qml") << "console.log(alien)" << 7 << 21;
@ -627,6 +625,9 @@ void TestQmllint::dirtyQmlCode_data()
QTest::newRow("nestedInlineComponents")
<< QStringLiteral("nestedInlineComponents.qml")
<< QStringLiteral("Nested inline components are not supported") << QString() << false;
QTest::newRow("WithStatement")
<< QStringLiteral("WithStatement.qml")
<< QStringLiteral("with statements are strongly discouraged") << QString() << false;
}
void TestQmllint::dirtyQmlCode()

65
tools/qmllint/checkidentifiers.cpp
View File

@ -402,71 +402,10 @@ void CheckIdentifiers::operator()(
if (baseIsPrefixed) {
m_logger->logWarning(QLatin1String("Type not found in namespace"), Log_Type,
location);
} else {
m_logger->logWarning(QLatin1String("Unqualified access"), Log_UnqualifiedAccess,
location);
}
// root(JS) --> (first element)
const auto firstElement = root->childScopes()[0];
FixSuggestion suggestion { Log_UnqualifiedAccess, QtWarningMsg, {} };
if ((firstElement->hasProperty(memberAccessBase.m_name)
|| firstElement->hasMethod(memberAccessBase.m_name)
|| firstElement->hasEnumeration(memberAccessBase.m_name))) {
QQmlJS::SourceLocation fixLocation = location;
fixLocation.length = 0;
suggestion.fixes << FixSuggestion::Fix {
memberAccessBase.m_name + QLatin1String(" is a member of the root element\n")
+ QLatin1String(" You can qualify the access with its id "
"to avoid this warning:\n"),
QtInfoMsg, fixLocation, rootId + u"."_qs
};
m_logger->suggestFix(suggestion);
if (rootId == QLatin1String("<id>")) {
suggestion.fixes << FixSuggestion::Fix {
u"You first have to give the root element an id"_qs,
QtInfoMsg,
QQmlJS::SourceLocation(),
{}
};
}
} else if (jsId.has_value()
&& jsId->kind == QQmlJSScope::JavaScriptIdentifier::Injected) {
const QQmlJSScope::JavaScriptIdentifier id = jsId.value();
QQmlJS::SourceLocation fixLocation = id.location;
fixLocation.length = 0;
const auto handler = signalHandlers[id.location];
QString fixString = handler.isMultiline ? u" function("_qs : u" ("_qs;
const auto parameters = handler.signalParameters;
for (int numParams = parameters.size(); numParams > 0; --numParams) {
fixString += parameters.at(parameters.size() - numParams);
if (numParams > 1)
fixString += u", "_qs;
}
fixString += handler.isMultiline ? u") "_qs : u") => "_qs;
suggestion.fixes << FixSuggestion::Fix {
memberAccessBase.m_name
+ QString::fromLatin1(" is accessible in this scope because "
"you are handling a signal at %1:%2:%3\n")
.arg(m_fileName)
.arg(id.location.startLine)
.arg(id.location.startColumn),
QtInfoMsg, fixLocation, fixString
};
m_logger->suggestFix(suggestion);
}
Q_UNUSED(signalHandlers)
Q_UNUSED(rootId)
}
const auto childScopes = currentScope->childScopes();
for (auto const &childScope : childScopes)

31
tools/qmllint/codegen.cpp
View File

@ -121,7 +121,10 @@ Codegen::compileBinding(const QV4::Compiler::Context *context, const QmlIR::Bind
&& m_objectType->hasProperty(signalName.chopped(strlen("Changed")))) {
isSignal = true;
} else {
const auto methods = m_objectType->methods(signalName);
const bool isConnections = !m_objectType->baseType().isNull()
&& m_objectType->baseType()->internalName() == u"QQmlConnections";
const auto methods = isConnections ? m_objectType->parentScope()->methods(signalName)
: m_objectType->methods(signalName);
for (const auto &method : methods) {
if (method.methodType() == QQmlJSMetaMethod::Signal) {
isSignal = true;
@ -248,24 +251,13 @@ QQmlJS::DiagnosticMessage Codegen::diagnose(const QString &message, QtMsgType ty
void Codegen::instructionOffsetToSrcLocation(const QV4::Compiler::Context *context, uint offset,
QQmlJS::SourceLocation *srcLoc) const
{
auto findLine = [](const QV4::CompiledData::CodeOffsetToLine &entry, uint offset) {
return entry.codeOffset < offset;
};
const QV4::CompiledData::CodeOffsetToLine *codeToLine =
std::lower_bound(context->lineNumberMapping.constBegin(),
context->lineNumberMapping.constEnd(), offset + 1, findLine)
- 1;
Q_ASSERT(context->sourceLocationTable);
const auto &entries = context->sourceLocationTable->entries;
auto item = std::lower_bound(entries.begin(), entries.end(), offset,
[](auto entry, uint offset) { return entry.offset < offset; });
QStringList lines = m_code.split(u'\n');
srcLoc->startLine = codeToLine->line;
srcLoc->startColumn = 0;
srcLoc->length = 0;
srcLoc->length = lines[srcLoc->startLine - 1].size();
srcLoc->offset = std::accumulate(lines.begin(), lines.begin() + codeToLine->line - 1, 0,
[](int a, QString s) { return s.length() + a; })
+ codeToLine->line - 1;
Q_ASSERT(item != entries.end());
*srcLoc = item->location;
}
bool Codegen::generateFunction(const QV4::Compiler::Context *context, Function *function) const
@ -325,6 +317,7 @@ bool Codegen::generateFunction(const QV4::Compiler::Context *context, Function *
QQmlJSTypePropagator::Error propagationError;
auto typePropagationResult = propagator.propagateTypes(
context, arguments, function->returnType, function->qmlScope,
m_typeResolver->objectsById(),
!function->returnType && function->contextType == QV4::Compiler::ContextType::Binding,
&propagationError);
if (propagationError.isSet()) {
@ -333,7 +326,7 @@ bool Codegen::generateFunction(const QV4::Compiler::Context *context, Function *
instructionOffsetToSrcLocation(context, propagationError.instructionOffset, &msg.loc);
msg.message = propagationError.message;
function->error = msg;
return false;
return propagationError.hasLoggerMessage;
}
return true;

3
tools/qmllint/main.cpp
View File

@ -182,7 +182,6 @@ static bool lint_file(const QString &filename, const bool silent, QJsonArray *js
success = v.check();
Codegen codegen { &importer, filename, qmltypesFiles, &v.logger(), code };
QQmlJSSaveFunction saveFunction = [](const QV4::CompiledData::SaveableUnitPointer &,
const QQmlJSAotFunctionMap &,
QString *) { return true; };
@ -191,7 +190,7 @@ static bool lint_file(const QString &filename, const bool silent, QJsonArray *js
QLoggingCategory::setFilterRules(u"qt.qml.compiler=false"_qs);
qCompileQmlFile(filename, saveFunction, &codegen, &error);
qCompileQmlFile(filename, saveFunction, &codegen, &error, true);
success &= !v.logger().hasWarnings() && !v.logger().hasErrors();