Skip to content
Permalink
Browse files
[BUGFIX][needs-docs] Allow expression parser to report better error l…
…ocation

We return the line and column to allow builder to highlight
that location for the user.
  • Loading branch information
NathanW2 committed Apr 12, 2018
1 parent b6242b4 commit 76843be62de1b2d590ed41ac3be4c9dd374972d9
Show file tree
Hide file tree
Showing 9 changed files with 214 additions and 17 deletions.
@@ -66,6 +66,28 @@ Implicit sharing was added in 2.14
%End
public:

struct ParserError
{
enum ParserErrorType
{
Unknown,
FunctionUnknown,
FunctionWrongArgs,
FunctionInvalidParams,
FunctionNamedArgsError
};

ParserErrorType errorType;

int firstLine;

int firstColumn;

int lastLine;

int lastColumn;
};

QgsExpression( const QString &expr );
%Docstring
Creates a new expression based on the provided string.
@@ -109,6 +131,13 @@ Returns true if an error occurred when parsing the input expression
QString parserErrorString() const;
%Docstring
Returns parser error
%End

ParserError parserError() const;
%Docstring
Returns parser error details including location of error.

.. versionadded:: 3.0
%End

const QgsExpressionNode *rootNode() const;
@@ -26,7 +26,7 @@


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

///////////////////////////////////////////////
// QVariant checks and conversions
@@ -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->mRootNode = ::parseExpression( expression, d->mParserErrorString, d->mParserError );
d->mEvalErrorString = QString();
d->mExp = expression;
}
@@ -195,7 +195,7 @@ int QgsExpression::functionCount()
QgsExpression::QgsExpression( const QString &expr )
: d( new QgsExpressionPrivate )
{
d->mRootNode = ::parseExpression( expr, d->mParserErrorString );
d->mRootNode = ::parseExpression( expr, d->mParserErrorString, d->mParserError );
d->mExp = expr;
Q_ASSERT( !d->mParserErrorString.isNull() || d->mRootNode );
}
@@ -255,6 +255,11 @@ QString QgsExpression::parserErrorString() const
return d->mParserErrorString;
}

QgsExpression::ParserError QgsExpression::parserError() const
{
return d->mParserError;
}

QSet<QString> QgsExpression::referencedColumns() const
{
if ( !d->mRootNode )
@@ -338,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->mRootNode = ::parseExpression( d->mExp, d->mParserErrorString, d->mParserError );
}

if ( !d->mRootNode )
@@ -115,6 +115,49 @@ class CORE_EXPORT QgsExpression
Q_DECLARE_TR_FUNCTIONS( QgsExpression )
public:

/**
* Details about any parser errors that were found when parsing the expression.
* \since QGIS 3.0
*/
struct CORE_EXPORT ParserError
{
enum ParserErrorType
{
Unknown = 0, //!< Unknown error type.
FunctionUnknown = 1, //!< Function was unknown.
FunctionWrongArgs = 2, //!< Function was called with the wrong number of args.
FunctionInvalidParams = 3, //!< Function was called with invalid args.
FunctionNamedArgsError = 4 //!< Non named function arg used after named arg.
};

/**
* The type of parser error that was found.
*/
ParserErrorType errorType = ParserErrorType::Unknown;

/**
* The first line that contained the error in the parser.
* Depending on the error sometimes this doesn't mean anything.
*/
int firstLine = 0;

/**
* The first column that contained the error in the parser.
* Depending on the error sometimes this doesn't mean anything.
*/
int firstColumn = 0;

/**
* The last line that contained the error in the parser.
*/
int lastLine = 0;

/**
* The last column that contained the error in the parser.
*/
int lastColumn = 0;
};

/**
* Creates a new expression based on the provided string.
* The string will immediately be parsed. For optimization
@@ -174,6 +217,12 @@ class CORE_EXPORT QgsExpression
//! Returns parser error
QString parserErrorString() const;

/**
* Returns parser error details including location of error.
* \since QGIS 3.0
*/
ParserError parserError() const;

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

@@ -20,8 +20,11 @@
%option prefix="exp_"
// this makes flex generate lexer with context + init/destroy functions
%option reentrant
%option yylineno
// this makes Bison send yylex another argument to use instead of using the global variable yylval
%option bison-bridge
%option bison-locations


// ensure that lexer will be 8-bit (and not just 7-bit)
%option 8bit
@@ -54,6 +57,19 @@
#define TEXT yylval->text = new QString( QString::fromUtf8(yytext) );
#define TEXT_FILTER(filter_fn) yylval->text = new QString( filter_fn( QString::fromUtf8(yytext) ) );

#define YY_USER_ACTION \
yylloc->first_line = yylloc->last_line; \
yylloc->first_column = yylloc->last_column; \
for(int i = 0; yytext[i] != '\0'; i++) { \
if(yytext[i] == '\n') { \
yylloc->last_line++; \
yylloc->last_column = 0; \
} \
else { \
yylloc->last_column++; \
} \
}

static QString stripText(QString text)
{
// strip single quotes on start,end
@@ -38,16 +38,16 @@ typedef void* yyscan_t;
typedef struct yy_buffer_state* YY_BUFFER_STATE;
extern int exp_lex_init(yyscan_t* scanner);
extern int exp_lex_destroy(yyscan_t scanner);
extern int exp_lex(YYSTYPE* yylval_param, yyscan_t yyscanner);
extern int exp_lex(YYSTYPE* yylval_param, YYLTYPE* yyloc, yyscan_t yyscanner);
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);
QgsExpressionNode* parseExpression(const QString& str, QString& parserErrorMsg, QgsExpression::ParserError& parserError);

/** error handler for bison */
void exp_error(expression_parser_context* parser_ctx, const char* msg);
void exp_error(YYLTYPE* yyloc, expression_parser_context* parser_ctx, const char* msg);

struct expression_parser_context
{
@@ -56,6 +56,7 @@ struct expression_parser_context

// varible where the parser error will be stored
QString errorMsg;
QgsExpression::ParserError parserError;
// root node of the expression
QgsExpressionNode* rootNode;
};
@@ -70,6 +71,7 @@ struct expression_parser_context
%}

// make the parser reentrant
%locations
%define api.pure
%lex-param {void * scanner}
%parse-param {expression_parser_context* parser_ctx}
@@ -193,22 +195,28 @@ 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)
exp_error(parser_ctx, "Function is not known");
QgsExpression::ParserError::ParserErrorType errorType = QgsExpression::ParserError::FunctionUnknown;
parser_ctx->parserError.errorType = errorType;
exp_error(&yyloc, parser_ctx, "Function is not known");
delete $3;
YYERROR;
}
QString paramError;
if ( !QgsExpressionNodeFunction::validateParams( fnIndex, $3, paramError ) )
{
exp_error( parser_ctx, paramError.toLocal8Bit().constData() );
QgsExpression::ParserError::ParserErrorType errorType = QgsExpression::ParserError::FunctionInvalidParams;
parser_ctx->parserError.errorType = errorType;
exp_error( &yyloc, parser_ctx, paramError.toLocal8Bit().constData() );
delete $3;
YYERROR;
}
if ( QgsExpression::Functions()[fnIndex]->params() != -1
&& !( QgsExpression::Functions()[fnIndex]->params() >= $3->count()
&& QgsExpression::Functions()[fnIndex]->minParams() <= $3->count() ) )
{
exp_error(parser_ctx, QString( "%1 function is called with wrong number of arguments" ).arg( QgsExpression::Functions()[fnIndex]->name() ).toLocal8Bit().constData() );
QgsExpression::ParserError::ParserErrorType errorType = QgsExpression::ParserError::FunctionWrongArgs;
parser_ctx->parserError.errorType = 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;
}
@@ -223,14 +231,18 @@ 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)
exp_error(parser_ctx, "Function is not known");
QgsExpression::ParserError::ParserErrorType errorType = QgsExpression::ParserError::FunctionUnknown;
parser_ctx->parserError.errorType = errorType;
exp_error(&yyloc, parser_ctx, "Function is not known");
YYERROR;
}
// 0 parameters is expected, -1 parameters means leave it to the
// implementation
if ( QgsExpression::Functions()[fnIndex]->params() > 0 )
{
exp_error(parser_ctx, QString( "%1 function is called with wrong number of arguments" ).arg( QgsExpression::Functions()[fnIndex]->name() ).toLocal8Bit().constData() );
QgsExpression::ParserError::ParserErrorType errorType = QgsExpression::ParserError::FunctionWrongArgs;
parser_ctx->parserError.errorType = errorType;
exp_error(&yyloc, parser_ctx, QString( "%1 function is called with wrong number of arguments" ).arg( QgsExpression::Functions()[fnIndex]->name() ).toLocal8Bit().constData() );
YYERROR;
}
$$ = new QgsExpressionNodeFunction(fnIndex, new QgsExpressionNode::NodeList());
@@ -258,7 +270,9 @@ expression:
}
else
{
exp_error(parser_ctx, QString("%1 function is not known").arg(*$1).toLocal8Bit().constData());
QgsExpression::ParserError::ParserErrorType errorType = QgsExpression::ParserError::FunctionUnknown;
parser_ctx->parserError.errorType = errorType;
exp_error(&yyloc, parser_ctx, QString("%1 function is not known").arg(*$1).toLocal8Bit().constData());
YYERROR;
}
delete $1;
@@ -292,7 +306,9 @@ exp_list:
{
if ( $1->hasNamedNodes() )
{
exp_error(parser_ctx, "All parameters following a named parameter must also be named.");
QgsExpression::ParserError::ParserErrorType errorType = QgsExpression::ParserError::FunctionNamedArgsError;
parser_ctx->parserError.errorType = errorType;
exp_error(&yyloc, parser_ctx, "All parameters following a named parameter must also be named.");
delete $1;
YYERROR;
}
@@ -319,7 +335,7 @@ when_then_clause:


// returns parsed tree, otherwise returns nullptr and sets parserErrorMsg
QgsExpressionNode* parseExpression(const QString& str, QString& parserErrorMsg)
QgsExpressionNode* parseExpression(const QString& str, QString& parserErrorMsg, QgsExpression::ParserError &parserError)
{
expression_parser_context ctx;
ctx.rootNode = 0;
@@ -337,13 +353,18 @@ QgsExpressionNode* parseExpression(const QString& str, QString& parserErrorMsg)
else // error?
{
parserErrorMsg = ctx.errorMsg;
parserError = ctx.parserError;
delete ctx.rootNode;
return nullptr;
}
}


void exp_error(expression_parser_context* parser_ctx, const char* msg)
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;
}
@@ -44,6 +44,7 @@ class QgsExpressionPrivate
, mRootNode( other.mRootNode ? other.mRootNode->clone() : nullptr )
, mParserErrorString( other.mParserErrorString )
, mEvalErrorString( other.mEvalErrorString )
, mParserError( other.mParserError )
, mExp( other.mExp )
, mCalc( other.mCalc )
, mDistanceUnit( other.mDistanceUnit )
@@ -62,6 +63,8 @@ class QgsExpressionPrivate
QString mParserErrorString;
QString mEvalErrorString;

QgsExpression::ParserError mParserError;

QString mExp;

std::shared_ptr<QgsDistanceArea> mCalc;

8 comments on commit 76843be

@saberraz
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This commit is giving me an error for my build:
../src/gui/qgsexpressionbuilderwidget.cpp: In constructor ‘QgsExpressionBuilderWidget::QgsExpressionBuilderWidget(QWidget*)’: ../src/gui/qgsexpressionbuilderwidget.cpp:126:56: error: ‘TriangleIndicator’ is not a member of ‘QgsCodeEditor’ txtExpressionString->indicatorDefine( QgsCodeEditor::TriangleIndicator, QgsExpression::ParserError::Unknown );

@alexbruy
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comment above, seems TriangleIndicator was added in newer QScintilla version than available in most distributions.

@NathanW2
Copy link
Member Author

@NathanW2 NathanW2 commented on 76843be Apr 13, 2018 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexbruy
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2.9.1 on Ubuntu 16.04, 2.7.2 in OSGeo4W64, 2.8.4 in OSGeo4W.

@NathanW2
Copy link
Member Author

@NathanW2 NathanW2 commented on 76843be Apr 13, 2018 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@saberraz
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexbruy you mean: NOT available in most distribution?
I am on Ubuntu 17.10 and QsciScintilla is 2.9.

@alexbruy
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, my English is not very clear. I just was saying that TriangleIndicator was added in never QsciScintilla version, while in most ditros we still have 2.9.x

@saberraz
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @alexbruy for clarifying that! 👍

Please sign in to comment.