Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
[FEATURE][needs-docs] Don't bail on first expression error (#6838)
  • Loading branch information
NathanW2 committed Apr 23, 2018
1 parent 8e7486b commit 76b956a
Show file tree
Hide file tree
Showing 8 changed files with 93 additions and 58 deletions.
4 changes: 3 additions & 1 deletion python/core/expression/qgsexpression.sip.in
Expand Up @@ -79,6 +79,8 @@ Implicit sharing was added in 2.14

ParserErrorType errorType;

QString errorMsg;

int firstLine;

int firstColumn;
Expand Down Expand Up @@ -133,7 +135,7 @@ Returns true if an error occurred when parsing the input expression
Returns parser error
%End

ParserError parserError() const;
QList<QgsExpression::ParserError> parserErrors() const;
%Docstring
Returns parser error details including location of error.

Expand Down
14 changes: 7 additions & 7 deletions src/core/expression/qgsexpression.cpp
Expand Up @@ -26,7 +26,7 @@


// from parser
extern QgsExpressionNode *parseExpression( const QString &str, QString &parserErrorMsg, QgsExpression::ParserError &parserError );
extern QgsExpressionNode *parseExpression( const QString &str, QString &parserErrorMsg, QList<QgsExpression::ParserError> &parserErrors );

///////////////////////////////////////////////
// QVariant checks and conversions
Expand Down Expand Up @@ -99,7 +99,7 @@ bool QgsExpression::checkExpression( const QString &text, const QgsExpressionCon
void QgsExpression::setExpression( const QString &expression )
{
detach();
d->mRootNode = ::parseExpression( expression, d->mParserErrorString, d->mParserError );
d->mRootNode = ::parseExpression( expression, d->mParserErrorString, d->mParserErrors );
d->mEvalErrorString = QString();
d->mExp = expression;
}
Expand Down Expand Up @@ -195,7 +195,7 @@ int QgsExpression::functionCount()
QgsExpression::QgsExpression( const QString &expr )
: d( new QgsExpressionPrivate )
{
d->mRootNode = ::parseExpression( expr, d->mParserErrorString, d->mParserError );
d->mRootNode = ::parseExpression( expr, d->mParserErrorString, d->mParserErrors );
d->mExp = expr;
Q_ASSERT( !d->mParserErrorString.isNull() || d->mRootNode );
}
Expand Down Expand Up @@ -247,17 +247,17 @@ bool QgsExpression::isValid() const

bool QgsExpression::hasParserError() const
{
return !d->mParserErrorString.isNull();
return d->mParserErrors.count() > 0;
}

QString QgsExpression::parserErrorString() const
{
return d->mParserErrorString;
}

QgsExpression::ParserError QgsExpression::parserError() const
QList<QgsExpression::ParserError> QgsExpression::parserErrors() const
{
return d->mParserError;
return d->mParserErrors;
}

QSet<QString> QgsExpression::referencedColumns() const
Expand Down Expand Up @@ -343,7 +343,7 @@ bool QgsExpression::prepare( const QgsExpressionContext *context )
//re-parse expression. Creation of QgsExpressionContexts may have added extra
//known functions since this expression was created, so we have another try
//at re-parsing it now that the context must have been created
d->mRootNode = ::parseExpression( d->mExp, d->mParserErrorString, d->mParserError );
d->mRootNode = ::parseExpression( d->mExp, d->mParserErrorString, d->mParserErrors );
}

if ( !d->mRootNode )
Expand Down
7 changes: 6 additions & 1 deletion src/core/expression/qgsexpression.h
Expand Up @@ -135,6 +135,11 @@ class CORE_EXPORT QgsExpression
*/
ParserErrorType errorType = ParserErrorType::Unknown;

/**
* The message for the error at this location.
*/
QString errorMsg;

/**
* The first line that contained the error in the parser.
* Depending on the error sometimes this doesn't mean anything.
Expand Down Expand Up @@ -221,7 +226,7 @@ class CORE_EXPORT QgsExpression
* Returns parser error details including location of error.
* \since QGIS 3.0
*/
ParserError parserError() const;
QList<QgsExpression::ParserError> parserErrors() const;

//! Returns root node of the expression. Root node is null is parsing has failed
const QgsExpressionNode *rootNode() const;
Expand Down
50 changes: 32 additions & 18 deletions src/core/qgsexpressionparser.yy
Expand Up @@ -45,7 +45,7 @@ extern YY_BUFFER_STATE exp__scan_string(const char* buffer, yyscan_t scanner);
/** returns parsed tree, otherwise returns nullptr and sets parserErrorMsg
(interface function to be called from QgsExpression)
*/
QgsExpressionNode* parseExpression(const QString& str, QString& parserErrorMsg, QgsExpression::ParserError& parserError);
QgsExpressionNode* parseExpression(const QString& str, QString& parserErrorMsg, QList<QgsExpression::ParserError>& parserError);

/** error handler for bison */
void exp_error(YYLTYPE* yyloc, expression_parser_context* parser_ctx, const char* msg);
Expand All @@ -55,9 +55,11 @@ struct expression_parser_context
// lexer context
yyscan_t flex_scanner;

// varible where the parser error will be stored
// List of all errors.
QList<QgsExpression::ParserError> parserErrors;
QString errorMsg;
QgsExpression::ParserError parserError;
// Current parser error.
QgsExpression::ParserError::ParserErrorType currentErrorType = QgsExpression::ParserError::Unknown;
// root node of the expression
QgsExpressionNode* rootNode;
};
Expand Down Expand Up @@ -172,6 +174,11 @@ void addParserLocation(YYLTYPE* yyloc, QgsExpressionNode *node)
%%

root: expression { parser_ctx->rootNode = $1; }
| error expression
{
delete $2;
yyerrok;
}
;

expression:
Expand Down Expand Up @@ -205,7 +212,7 @@ expression:
// this should not actually happen because already in lexer we check whether an identifier is a known function
// (if the name is not known the token is parsed as a column)
QgsExpression::ParserError::ParserErrorType errorType = QgsExpression::ParserError::FunctionUnknown;
parser_ctx->parserError.errorType = errorType;
parser_ctx->currentErrorType = errorType;
exp_error(&yyloc, parser_ctx, "Function is not known");
delete $3;
YYERROR;
Expand All @@ -214,7 +221,7 @@ expression:
if ( !QgsExpressionNodeFunction::validateParams( fnIndex, $3, paramError ) )
{
QgsExpression::ParserError::ParserErrorType errorType = QgsExpression::ParserError::FunctionInvalidParams;
parser_ctx->parserError.errorType = errorType;
parser_ctx->currentErrorType = errorType;
exp_error( &yyloc, parser_ctx, paramError.toLocal8Bit().constData() );
delete $3;
YYERROR;
Expand All @@ -224,7 +231,7 @@ expression:
&& QgsExpression::Functions()[fnIndex]->minParams() <= $3->count() ) )
{
QgsExpression::ParserError::ParserErrorType errorType = QgsExpression::ParserError::FunctionWrongArgs;
parser_ctx->parserError.errorType = errorType;
parser_ctx->currentErrorType = errorType;
exp_error(&yyloc, parser_ctx, QString( "%1 function is called with wrong number of arguments" ).arg( QgsExpression::Functions()[fnIndex]->name() ).toLocal8Bit().constData() );
delete $3;
YYERROR;
Expand All @@ -242,7 +249,7 @@ expression:
// this should not actually happen because already in lexer we check whether an identifier is a known function
// (if the name is not known the token is parsed as a column)
QgsExpression::ParserError::ParserErrorType errorType = QgsExpression::ParserError::FunctionUnknown;
parser_ctx->parserError.errorType = errorType;
parser_ctx->currentErrorType = errorType;
exp_error(&yyloc, parser_ctx, "Function is not known");
YYERROR;
}
Expand All @@ -251,7 +258,7 @@ expression:
if ( QgsExpression::Functions()[fnIndex]->params() > 0 )
{
QgsExpression::ParserError::ParserErrorType errorType = QgsExpression::ParserError::FunctionWrongArgs;
parser_ctx->parserError.errorType = errorType;
parser_ctx->currentErrorType = errorType;
exp_error(&yyloc, parser_ctx, QString( "%1 function is called with wrong number of arguments" ).arg( QgsExpression::Functions()[fnIndex]->name() ).toLocal8Bit().constData() );
YYERROR;
}
Expand Down Expand Up @@ -282,7 +289,7 @@ expression:
else
{
QgsExpression::ParserError::ParserErrorType errorType = QgsExpression::ParserError::FunctionUnknown;
parser_ctx->parserError.errorType = errorType;
parser_ctx->currentErrorType = errorType;
exp_error(&yyloc, parser_ctx, QString("%1 function is not known").arg(*$1).toLocal8Bit().constData());
YYERROR;
}
Expand Down Expand Up @@ -318,7 +325,7 @@ exp_list:
if ( $1->hasNamedNodes() )
{
QgsExpression::ParserError::ParserErrorType errorType = QgsExpression::ParserError::FunctionNamedArgsError;
parser_ctx->parserError.errorType = errorType;
parser_ctx->currentErrorType = errorType;
exp_error(&yyloc, parser_ctx, "All parameters following a named parameter must also be named.");
delete $1;
YYERROR;
Expand Down Expand Up @@ -346,7 +353,7 @@ when_then_clause:


// returns parsed tree, otherwise returns nullptr and sets parserErrorMsg
QgsExpressionNode* parseExpression(const QString& str, QString& parserErrorMsg, QgsExpression::ParserError &parserError)
QgsExpressionNode* parseExpression(const QString& str, QString& parserErrorMsg, QList<QgsExpression::ParserError> &parserErrors)
{
expression_parser_context ctx;
ctx.rootNode = 0;
Expand All @@ -357,14 +364,14 @@ QgsExpressionNode* parseExpression(const QString& str, QString& parserErrorMsg,
exp_lex_destroy(ctx.flex_scanner);

// list should be empty when parsing was OK
if (res == 0) // success?
if (res == 0 && ctx.parserErrors.count() == 0) // success?
{
return ctx.rootNode;
}
else // error?
{
parserErrorMsg = ctx.errorMsg;
parserError = ctx.parserError;
parserErrors = ctx.parserErrors;
delete ctx.rootNode;
return nullptr;
}
Expand All @@ -373,9 +380,16 @@ QgsExpressionNode* parseExpression(const QString& str, QString& parserErrorMsg,

void exp_error(YYLTYPE* yyloc,expression_parser_context* parser_ctx, const char* msg)
{
parser_ctx->errorMsg = msg;
parser_ctx->parserError.firstColumn = yyloc->first_column;
parser_ctx->parserError.firstLine = yyloc->first_line;
parser_ctx->parserError.lastColumn = yyloc->last_column;
parser_ctx->parserError.lastLine = yyloc->last_line;
QgsExpression::ParserError error = QgsExpression::ParserError();
error.firstColumn = yyloc->first_column;
error.firstLine = yyloc->first_line;
error.lastColumn = yyloc->last_column;
error.lastLine = yyloc->last_line;
error.errorMsg = msg;
error.errorType = parser_ctx->currentErrorType;
parser_ctx->parserErrors.append(error);
// Reest the current error type for the next error.
parser_ctx->currentErrorType = QgsExpression::ParserError::Unknown;

parser_ctx->errorMsg = parser_ctx->errorMsg + "\n" + msg;
}
4 changes: 2 additions & 2 deletions src/core/qgsexpressionprivate.h
Expand Up @@ -44,7 +44,7 @@ class QgsExpressionPrivate
, mRootNode( other.mRootNode ? other.mRootNode->clone() : nullptr )
, mParserErrorString( other.mParserErrorString )
, mEvalErrorString( other.mEvalErrorString )
, mParserError( other.mParserError )
, mParserErrors( other.mParserErrors )
, mExp( other.mExp )
, mCalc( other.mCalc )
, mDistanceUnit( other.mDistanceUnit )
Expand All @@ -63,7 +63,7 @@ class QgsExpressionPrivate
QString mParserErrorString;
QString mEvalErrorString;

QgsExpression::ParserError mParserError;
QList<QgsExpression::ParserError> mParserErrors;

QString mExp;

Expand Down
63 changes: 38 additions & 25 deletions src/gui/qgsexpressionbuilderwidget.cpp
Expand Up @@ -649,22 +649,14 @@ void QgsExpressionBuilderWidget::txtExpressionString_textChanged()

if ( exp.hasParserError() || exp.hasEvalError() )
{
QString tooltip = QStringLiteral( "<b>%1:</b><br>%2" ).arg( tr( "Parser Error" ), exp.parserErrorString() );
if ( exp.hasEvalError() )
tooltip += QStringLiteral( "<br><br><b>%1:</b><br>%2" ).arg( tr( "Eval Error" ), exp.evalErrorString() );

int errorFirstLine = exp.parserError().firstLine - 1 ;
int errorFirstColumn = exp.parserError().firstColumn - 1;
int errorLastColumn = exp.parserError().lastColumn - 1;
int errorLastLine = exp.parserError().lastLine - 1;

// If we have a unknown error we just mark the point that hit the error for now
// until we can handle others more.
if ( exp.parserError().errorType == QgsExpression::ParserError::Unknown )
{
errorFirstLine = errorLastLine;
errorFirstColumn = errorLastColumn - 1;
}
QString errorString = exp.parserErrorString().replace( "\n", "<br>" );
QString tooltip;
if ( exp.hasParserError() )
tooltip = QStringLiteral( "<b>%1:</b>"
"%2" ).arg( tr( "Parser Errors" ), errorString );
// Only show the eval error if there is no parser error.
if ( !exp.hasParserError() && exp.hasEvalError() )
tooltip += QStringLiteral( "<b>%1:</b> %2" ).arg( tr( "Eval Error" ), exp.evalErrorString() );

lblPreview->setText( tr( "Expression is invalid <a href=""more"">(more info)</a>" ) );
lblPreview->setStyleSheet( QStringLiteral( "color: rgba(255, 6, 10, 255);" ) );
Expand All @@ -673,10 +665,7 @@ void QgsExpressionBuilderWidget::txtExpressionString_textChanged()
emit expressionParsed( false );
setParserError( exp.hasParserError() );
setEvalError( exp.hasEvalError() );
txtExpressionString->fillIndicatorRange( errorFirstLine,
errorFirstColumn,
errorLastLine,
errorLastColumn, exp.parserError().errorType );
createErrorMarkers( exp.parserErrors() );
return;
}
else
Expand Down Expand Up @@ -789,6 +778,30 @@ void QgsExpressionBuilderWidget::showEvent( QShowEvent *e )
txtExpressionString->setFocus();
}

void QgsExpressionBuilderWidget::createErrorMarkers( QList<QgsExpression::ParserError> errors )
{
clearErrors();
for ( const QgsExpression::ParserError &error : errors )
{
int errorFirstLine = error.firstLine - 1 ;
int errorFirstColumn = error.firstColumn - 1;
int errorLastColumn = error.lastColumn - 1;
int errorLastLine = error.lastLine - 1;

// If we have a unknown error we just mark the point that hit the error for now
// until we can handle others more.
if ( error.errorType == QgsExpression::ParserError::Unknown )
{
errorFirstLine = errorLastLine;
errorFirstColumn = errorLastColumn - 1;
}
txtExpressionString->fillIndicatorRange( errorFirstLine,
errorFirstColumn,
errorLastLine,
errorLastColumn, error.errorType );
}
}

void QgsExpressionBuilderWidget::createMarkers( const QgsExpressionNode *inNode )
{
switch ( inNode->nodeType() )
Expand Down Expand Up @@ -873,11 +886,11 @@ void QgsExpressionBuilderWidget::clearErrors()
{
int lastLine = txtExpressionString->lines() - 1;
// Note: -1 here doesn't seem to do the clear all like the other functions. Will need to make this a bit smarter.
txtExpressionString->clearIndicatorRange( 0, 0, lastLine, txtExpressionString->text( lastLine ).length() - 1, QgsExpression::ParserError::Unknown );
txtExpressionString->clearIndicatorRange( 0, 0, lastLine, txtExpressionString->text( lastLine ).length() - 1, QgsExpression::ParserError::FunctionInvalidParams );
txtExpressionString->clearIndicatorRange( 0, 0, lastLine, txtExpressionString->text( lastLine ).length() - 1, QgsExpression::ParserError::FunctionUnknown );
txtExpressionString->clearIndicatorRange( 0, 0, lastLine, txtExpressionString->text( lastLine ).length() - 1, QgsExpression::ParserError::FunctionWrongArgs );
txtExpressionString->clearIndicatorRange( 0, 0, lastLine, txtExpressionString->text( lastLine ).length() - 1, QgsExpression::ParserError::FunctionNamedArgsError );
txtExpressionString->clearIndicatorRange( 0, 0, lastLine, txtExpressionString->text( lastLine ).length(), QgsExpression::ParserError::Unknown );
txtExpressionString->clearIndicatorRange( 0, 0, lastLine, txtExpressionString->text( lastLine ).length(), QgsExpression::ParserError::FunctionInvalidParams );
txtExpressionString->clearIndicatorRange( 0, 0, lastLine, txtExpressionString->text( lastLine ).length(), QgsExpression::ParserError::FunctionUnknown );
txtExpressionString->clearIndicatorRange( 0, 0, lastLine, txtExpressionString->text( lastLine ).length(), QgsExpression::ParserError::FunctionWrongArgs );
txtExpressionString->clearIndicatorRange( 0, 0, lastLine, txtExpressionString->text( lastLine ).length(), QgsExpression::ParserError::FunctionNamedArgsError );
}

void QgsExpressionBuilderWidget::txtSearchEdit_textChanged()
Expand Down
1 change: 1 addition & 0 deletions src/gui/qgsexpressionbuilderwidget.h
Expand Up @@ -354,6 +354,7 @@ class GUI_EXPORT QgsExpressionBuilderWidget : public QWidget, private Ui::QgsExp

private:
int FUNCTION_MARKER_ID = 25;
void createErrorMarkers( QList<QgsExpression::ParserError> errors );
void createMarkers( const QgsExpressionNode *node );
void clearFunctionMarkers();
void clearErrors();
Expand Down
8 changes: 4 additions & 4 deletions tests/src/core/testqgsexpression.cpp
Expand Up @@ -281,10 +281,10 @@ class TestQgsExpression: public QObject

QgsExpression exp( string );
QCOMPARE( exp.hasParserError(), true );
QCOMPARE( exp.parserError().firstLine, firstLine );
QCOMPARE( exp.parserError().firstColumn, firstColumn );
QCOMPARE( exp.parserError().lastLine, lastLine );
QCOMPARE( exp.parserError().lastColumn, lastColumn );
QCOMPARE( exp.parserErrors().first().firstLine, firstLine );
QCOMPARE( exp.parserErrors().first().firstColumn, firstColumn );
QCOMPARE( exp.parserErrors().first().lastLine, lastLine );
QCOMPARE( exp.parserErrors().first().lastColumn, lastColumn );
}

void parsing_with_locale()
Expand Down

0 comments on commit 76b956a

Please sign in to comment.