[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.
qgis · Apr 12, 2018 · 76843be · 76843be · saberraz · Apr 13, 2018
1 parent b6242b4
commit 76843be
Show file tree

Hide file tree

Showing 9 changed files with 214 additions and 17 deletions.
diff --git a/python/core/expression/qgsexpression.sip.in b/python/core/expression/qgsexpression.sip.in
@@ -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;

diff --git a/src/core/expression/qgsexpression.cpp b/src/core/expression/qgsexpression.cpp
@@ -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 )

diff --git a/src/core/expression/qgsexpression.h b/src/core/expression/qgsexpression.h
@@ -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;
 

diff --git a/src/core/qgsexpressionlexer.ll b/src/core/qgsexpressionlexer.ll
@@ -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

diff --git a/src/core/qgsexpressionparser.yy b/src/core/qgsexpressionparser.yy
@@ -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;
 }
diff --git a/src/core/qgsexpressionprivate.h b/src/core/qgsexpressionprivate.h
@@ -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;