Skip to content

Commit 76b956a

Browse files
NathanW2NathanW2
authored
[FEATURE][needs-docs] Don't bail on first expression error (#6838)
1 parent 8e7486b commit 76b956a

8 files changed

+93
-58
lines changed

‎python/core/expression/qgsexpression.sip.in

+3-1
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,8 @@ Implicit sharing was added in 2.14
7979

8080
ParserErrorType errorType;
8181

82+
QString errorMsg;
83+
8284
int firstLine;
8385

8486
int firstColumn;
@@ -133,7 +135,7 @@ Returns true if an error occurred when parsing the input expression
133135
Returns parser error
134136
%End
135137

136-
ParserError parserError() const;
138+
QList<QgsExpression::ParserError> parserErrors() const;
137139
%Docstring
138140
Returns parser error details including location of error.
139141

‎src/core/expression/qgsexpression.cpp

+7-7
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@
2626

2727

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

3131
///////////////////////////////////////////////
3232
// QVariant checks and conversions
@@ -99,7 +99,7 @@ bool QgsExpression::checkExpression( const QString &text, const QgsExpressionCon
9999
void QgsExpression::setExpression( const QString &expression )
100100
{
101101
detach();
102-
d->mRootNode = ::parseExpression( expression, d->mParserErrorString, d->mParserError );
102+
d->mRootNode = ::parseExpression( expression, d->mParserErrorString, d->mParserErrors );
103103
d->mEvalErrorString = QString();
104104
d->mExp = expression;
105105
}
@@ -195,7 +195,7 @@ int QgsExpression::functionCount()
195195
QgsExpression::QgsExpression( const QString &expr )
196196
: d( new QgsExpressionPrivate )
197197
{
198-
d->mRootNode = ::parseExpression( expr, d->mParserErrorString, d->mParserError );
198+
d->mRootNode = ::parseExpression( expr, d->mParserErrorString, d->mParserErrors );
199199
d->mExp = expr;
200200
Q_ASSERT( !d->mParserErrorString.isNull() || d->mRootNode );
201201
}
@@ -247,17 +247,17 @@ bool QgsExpression::isValid() const
247247

248248
bool QgsExpression::hasParserError() const
249249
{
250-
return !d->mParserErrorString.isNull();
250+
return d->mParserErrors.count() > 0;
251251
}
252252

253253
QString QgsExpression::parserErrorString() const
254254
{
255255
return d->mParserErrorString;
256256
}
257257

258-
QgsExpression::ParserError QgsExpression::parserError() const
258+
QList<QgsExpression::ParserError> QgsExpression::parserErrors() const
259259
{
260-
return d->mParserError;
260+
return d->mParserErrors;
261261
}
262262

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

349349
if ( !d->mRootNode )

‎src/core/expression/qgsexpression.h

+6-1
Original file line numberDiff line numberDiff line change
@@ -135,6 +135,11 @@ class CORE_EXPORT QgsExpression
135135
*/
136136
ParserErrorType errorType = ParserErrorType::Unknown;
137137

138+
/**
139+
* The message for the error at this location.
140+
*/
141+
QString errorMsg;
142+
138143
/**
139144
* The first line that contained the error in the parser.
140145
* Depending on the error sometimes this doesn't mean anything.
@@ -221,7 +226,7 @@ class CORE_EXPORT QgsExpression
221226
* Returns parser error details including location of error.
222227
* \since QGIS 3.0
223228
*/
224-
ParserError parserError() const;
229+
QList<QgsExpression::ParserError> parserErrors() const;
225230

226231
//! Returns root node of the expression. Root node is null is parsing has failed
227232
const QgsExpressionNode *rootNode() const;

‎src/core/qgsexpressionparser.yy

+32-18
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ extern YY_BUFFER_STATE exp__scan_string(const char* buffer, yyscan_t scanner);
4545
/** returns parsed tree, otherwise returns nullptr and sets parserErrorMsg
4646
(interface function to be called from QgsExpression)
4747
*/
48-
QgsExpressionNode* parseExpression(const QString& str, QString& parserErrorMsg, QgsExpression::ParserError& parserError);
48+
QgsExpressionNode* parseExpression(const QString& str, QString& parserErrorMsg, QList<QgsExpression::ParserError>& parserError);
4949

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

58-
// varible where the parser error will be stored
58+
// List of all errors.
59+
QList<QgsExpression::ParserError> parserErrors;
5960
QString errorMsg;
60-
QgsExpression::ParserError parserError;
61+
// Current parser error.
62+
QgsExpression::ParserError::ParserErrorType currentErrorType = QgsExpression::ParserError::Unknown;
6163
// root node of the expression
6264
QgsExpressionNode* rootNode;
6365
};
@@ -172,6 +174,11 @@ void addParserLocation(YYLTYPE* yyloc, QgsExpressionNode *node)
172174
%%
173175

174176
root: expression { parser_ctx->rootNode = $1; }
177+
| error expression
178+
{
179+
delete $2;
180+
yyerrok;
181+
}
175182
;
176183

177184
expression:
@@ -205,7 +212,7 @@ expression:
205212
// this should not actually happen because already in lexer we check whether an identifier is a known function
206213
// (if the name is not known the token is parsed as a column)
207214
QgsExpression::ParserError::ParserErrorType errorType = QgsExpression::ParserError::FunctionUnknown;
208-
parser_ctx->parserError.errorType = errorType;
215+
parser_ctx->currentErrorType = errorType;
209216
exp_error(&yyloc, parser_ctx, "Function is not known");
210217
delete $3;
211218
YYERROR;
@@ -214,7 +221,7 @@ expression:
214221
if ( !QgsExpressionNodeFunction::validateParams( fnIndex, $3, paramError ) )
215222
{
216223
QgsExpression::ParserError::ParserErrorType errorType = QgsExpression::ParserError::FunctionInvalidParams;
217-
parser_ctx->parserError.errorType = errorType;
224+
parser_ctx->currentErrorType = errorType;
218225
exp_error( &yyloc, parser_ctx, paramError.toLocal8Bit().constData() );
219226
delete $3;
220227
YYERROR;
@@ -224,7 +231,7 @@ expression:
224231
&& QgsExpression::Functions()[fnIndex]->minParams() <= $3->count() ) )
225232
{
226233
QgsExpression::ParserError::ParserErrorType errorType = QgsExpression::ParserError::FunctionWrongArgs;
227-
parser_ctx->parserError.errorType = errorType;
234+
parser_ctx->currentErrorType = errorType;
228235
exp_error(&yyloc, parser_ctx, QString( "%1 function is called with wrong number of arguments" ).arg( QgsExpression::Functions()[fnIndex]->name() ).toLocal8Bit().constData() );
229236
delete $3;
230237
YYERROR;
@@ -242,7 +249,7 @@ expression:
242249
// this should not actually happen because already in lexer we check whether an identifier is a known function
243250
// (if the name is not known the token is parsed as a column)
244251
QgsExpression::ParserError::ParserErrorType errorType = QgsExpression::ParserError::FunctionUnknown;
245-
parser_ctx->parserError.errorType = errorType;
252+
parser_ctx->currentErrorType = errorType;
246253
exp_error(&yyloc, parser_ctx, "Function is not known");
247254
YYERROR;
248255
}
@@ -251,7 +258,7 @@ expression:
251258
if ( QgsExpression::Functions()[fnIndex]->params() > 0 )
252259
{
253260
QgsExpression::ParserError::ParserErrorType errorType = QgsExpression::ParserError::FunctionWrongArgs;
254-
parser_ctx->parserError.errorType = errorType;
261+
parser_ctx->currentErrorType = errorType;
255262
exp_error(&yyloc, parser_ctx, QString( "%1 function is called with wrong number of arguments" ).arg( QgsExpression::Functions()[fnIndex]->name() ).toLocal8Bit().constData() );
256263
YYERROR;
257264
}
@@ -282,7 +289,7 @@ expression:
282289
else
283290
{
284291
QgsExpression::ParserError::ParserErrorType errorType = QgsExpression::ParserError::FunctionUnknown;
285-
parser_ctx->parserError.errorType = errorType;
292+
parser_ctx->currentErrorType = errorType;
286293
exp_error(&yyloc, parser_ctx, QString("%1 function is not known").arg(*$1).toLocal8Bit().constData());
287294
YYERROR;
288295
}
@@ -318,7 +325,7 @@ exp_list:
318325
if ( $1->hasNamedNodes() )
319326
{
320327
QgsExpression::ParserError::ParserErrorType errorType = QgsExpression::ParserError::FunctionNamedArgsError;
321-
parser_ctx->parserError.errorType = errorType;
328+
parser_ctx->currentErrorType = errorType;
322329
exp_error(&yyloc, parser_ctx, "All parameters following a named parameter must also be named.");
323330
delete $1;
324331
YYERROR;
@@ -346,7 +353,7 @@ when_then_clause:
346353

347354

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

359366
// list should be empty when parsing was OK
360-
if (res == 0) // success?
367+
if (res == 0 && ctx.parserErrors.count() == 0) // success?
361368
{
362369
return ctx.rootNode;
363370
}
364371
else // error?
365372
{
366373
parserErrorMsg = ctx.errorMsg;
367-
parserError = ctx.parserError;
374+
parserErrors = ctx.parserErrors;
368375
delete ctx.rootNode;
369376
return nullptr;
370377
}
@@ -373,9 +380,16 @@ QgsExpressionNode* parseExpression(const QString& str, QString& parserErrorMsg,
373380

374381
void exp_error(YYLTYPE* yyloc,expression_parser_context* parser_ctx, const char* msg)
375382
{
376-
parser_ctx->errorMsg = msg;
377-
parser_ctx->parserError.firstColumn = yyloc->first_column;
378-
parser_ctx->parserError.firstLine = yyloc->first_line;
379-
parser_ctx->parserError.lastColumn = yyloc->last_column;
380-
parser_ctx->parserError.lastLine = yyloc->last_line;
383+
QgsExpression::ParserError error = QgsExpression::ParserError();
384+
error.firstColumn = yyloc->first_column;
385+
error.firstLine = yyloc->first_line;
386+
error.lastColumn = yyloc->last_column;
387+
error.lastLine = yyloc->last_line;
388+
error.errorMsg = msg;
389+
error.errorType = parser_ctx->currentErrorType;
390+
parser_ctx->parserErrors.append(error);
391+
// Reest the current error type for the next error.
392+
parser_ctx->currentErrorType = QgsExpression::ParserError::Unknown;
393+
394+
parser_ctx->errorMsg = parser_ctx->errorMsg + "\n" + msg;
381395
}

‎src/core/qgsexpressionprivate.h

+2-2
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ class QgsExpressionPrivate
4444
, mRootNode( other.mRootNode ? other.mRootNode->clone() : nullptr )
4545
, mParserErrorString( other.mParserErrorString )
4646
, mEvalErrorString( other.mEvalErrorString )
47-
, mParserError( other.mParserError )
47+
, mParserErrors( other.mParserErrors )
4848
, mExp( other.mExp )
4949
, mCalc( other.mCalc )
5050
, mDistanceUnit( other.mDistanceUnit )
@@ -63,7 +63,7 @@ class QgsExpressionPrivate
6363
QString mParserErrorString;
6464
QString mEvalErrorString;
6565

66-
QgsExpression::ParserError mParserError;
66+
QList<QgsExpression::ParserError> mParserErrors;
6767

6868
QString mExp;
6969

‎src/gui/qgsexpressionbuilderwidget.cpp

+38-25
Original file line numberDiff line numberDiff line change
@@ -649,22 +649,14 @@ void QgsExpressionBuilderWidget::txtExpressionString_textChanged()
649649

650650
if ( exp.hasParserError() || exp.hasEvalError() )
651651
{
652-
QString tooltip = QStringLiteral( "<b>%1:</b><br>%2" ).arg( tr( "Parser Error" ), exp.parserErrorString() );
653-
if ( exp.hasEvalError() )
654-
tooltip += QStringLiteral( "<br><br><b>%1:</b><br>%2" ).arg( tr( "Eval Error" ), exp.evalErrorString() );
655-
656-
int errorFirstLine = exp.parserError().firstLine - 1 ;
657-
int errorFirstColumn = exp.parserError().firstColumn - 1;
658-
int errorLastColumn = exp.parserError().lastColumn - 1;
659-
int errorLastLine = exp.parserError().lastLine - 1;
660-
661-
// If we have a unknown error we just mark the point that hit the error for now
662-
// until we can handle others more.
663-
if ( exp.parserError().errorType == QgsExpression::ParserError::Unknown )
664-
{
665-
errorFirstLine = errorLastLine;
666-
errorFirstColumn = errorLastColumn - 1;
667-
}
652+
QString errorString = exp.parserErrorString().replace( "\n", "<br>" );
653+
QString tooltip;
654+
if ( exp.hasParserError() )
655+
tooltip = QStringLiteral( "<b>%1:</b>"
656+
"%2" ).arg( tr( "Parser Errors" ), errorString );
657+
// Only show the eval error if there is no parser error.
658+
if ( !exp.hasParserError() && exp.hasEvalError() )
659+
tooltip += QStringLiteral( "<b>%1:</b> %2" ).arg( tr( "Eval Error" ), exp.evalErrorString() );
668660

669661
lblPreview->setText( tr( "Expression is invalid <a href=""more"">(more info)</a>" ) );
670662
lblPreview->setStyleSheet( QStringLiteral( "color: rgba(255, 6, 10, 255);" ) );
@@ -673,10 +665,7 @@ void QgsExpressionBuilderWidget::txtExpressionString_textChanged()
673665
emit expressionParsed( false );
674666
setParserError( exp.hasParserError() );
675667
setEvalError( exp.hasEvalError() );
676-
txtExpressionString->fillIndicatorRange( errorFirstLine,
677-
errorFirstColumn,
678-
errorLastLine,
679-
errorLastColumn, exp.parserError().errorType );
668+
createErrorMarkers( exp.parserErrors() );
680669
return;
681670
}
682671
else
@@ -789,6 +778,30 @@ void QgsExpressionBuilderWidget::showEvent( QShowEvent *e )
789778
txtExpressionString->setFocus();
790779
}
791780

781+
void QgsExpressionBuilderWidget::createErrorMarkers( QList<QgsExpression::ParserError> errors )
782+
{
783+
clearErrors();
784+
for ( const QgsExpression::ParserError &error : errors )
785+
{
786+
int errorFirstLine = error.firstLine - 1 ;
787+
int errorFirstColumn = error.firstColumn - 1;
788+
int errorLastColumn = error.lastColumn - 1;
789+
int errorLastLine = error.lastLine - 1;
790+
791+
// If we have a unknown error we just mark the point that hit the error for now
792+
// until we can handle others more.
793+
if ( error.errorType == QgsExpression::ParserError::Unknown )
794+
{
795+
errorFirstLine = errorLastLine;
796+
errorFirstColumn = errorLastColumn - 1;
797+
}
798+
txtExpressionString->fillIndicatorRange( errorFirstLine,
799+
errorFirstColumn,
800+
errorLastLine,
801+
errorLastColumn, error.errorType );
802+
}
803+
}
804+
792805
void QgsExpressionBuilderWidget::createMarkers( const QgsExpressionNode *inNode )
793806
{
794807
switch ( inNode->nodeType() )
@@ -873,11 +886,11 @@ void QgsExpressionBuilderWidget::clearErrors()
873886
{
874887
int lastLine = txtExpressionString->lines() - 1;
875888
// Note: -1 here doesn't seem to do the clear all like the other functions. Will need to make this a bit smarter.
876-
txtExpressionString->clearIndicatorRange( 0, 0, lastLine, txtExpressionString->text( lastLine ).length() - 1, QgsExpression::ParserError::Unknown );
877-
txtExpressionString->clearIndicatorRange( 0, 0, lastLine, txtExpressionString->text( lastLine ).length() - 1, QgsExpression::ParserError::FunctionInvalidParams );
878-
txtExpressionString->clearIndicatorRange( 0, 0, lastLine, txtExpressionString->text( lastLine ).length() - 1, QgsExpression::ParserError::FunctionUnknown );
879-
txtExpressionString->clearIndicatorRange( 0, 0, lastLine, txtExpressionString->text( lastLine ).length() - 1, QgsExpression::ParserError::FunctionWrongArgs );
880-
txtExpressionString->clearIndicatorRange( 0, 0, lastLine, txtExpressionString->text( lastLine ).length() - 1, QgsExpression::ParserError::FunctionNamedArgsError );
889+
txtExpressionString->clearIndicatorRange( 0, 0, lastLine, txtExpressionString->text( lastLine ).length(), QgsExpression::ParserError::Unknown );
890+
txtExpressionString->clearIndicatorRange( 0, 0, lastLine, txtExpressionString->text( lastLine ).length(), QgsExpression::ParserError::FunctionInvalidParams );
891+
txtExpressionString->clearIndicatorRange( 0, 0, lastLine, txtExpressionString->text( lastLine ).length(), QgsExpression::ParserError::FunctionUnknown );
892+
txtExpressionString->clearIndicatorRange( 0, 0, lastLine, txtExpressionString->text( lastLine ).length(), QgsExpression::ParserError::FunctionWrongArgs );
893+
txtExpressionString->clearIndicatorRange( 0, 0, lastLine, txtExpressionString->text( lastLine ).length(), QgsExpression::ParserError::FunctionNamedArgsError );
881894
}
882895

883896
void QgsExpressionBuilderWidget::txtSearchEdit_textChanged()

‎src/gui/qgsexpressionbuilderwidget.h

+1
Original file line numberDiff line numberDiff line change
@@ -354,6 +354,7 @@ class GUI_EXPORT QgsExpressionBuilderWidget : public QWidget, private Ui::QgsExp
354354

355355
private:
356356
int FUNCTION_MARKER_ID = 25;
357+
void createErrorMarkers( QList<QgsExpression::ParserError> errors );
357358
void createMarkers( const QgsExpressionNode *node );
358359
void clearFunctionMarkers();
359360
void clearErrors();

‎tests/src/core/testqgsexpression.cpp

+4-4
Original file line numberDiff line numberDiff line change
@@ -281,10 +281,10 @@ class TestQgsExpression: public QObject
281281

282282
QgsExpression exp( string );
283283
QCOMPARE( exp.hasParserError(), true );
284-
QCOMPARE( exp.parserError().firstLine, firstLine );
285-
QCOMPARE( exp.parserError().firstColumn, firstColumn );
286-
QCOMPARE( exp.parserError().lastLine, lastLine );
287-
QCOMPARE( exp.parserError().lastColumn, lastColumn );
284+
QCOMPARE( exp.parserErrors().first().firstLine, firstLine );
285+
QCOMPARE( exp.parserErrors().first().firstColumn, firstColumn );
286+
QCOMPARE( exp.parserErrors().first().lastLine, lastLine );
287+
QCOMPARE( exp.parserErrors().first().lastColumn, lastColumn );
288288
}
289289

290290
void parsing_with_locale()

0 commit comments

Comments
 (0)