Fix warnings reported by fb-infer (#1440)

* Fix warnings reported by fb-infer

* validate algebraic expression structure on evaluation

Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com>
Co-authored-by: swilly22 <roi@redislabs.com>

src/arithmetic/algebraic_expression/algebraic_expression_construction.c

@@ -189,7 +189,6 @@ static AlgebraicExpression *_AlgebraicExpression_OperandFromEdge
 ) {
 	GrB_Matrix mat;
 	uint reltype_id;
-	Graph *g = QueryCtx_GetGraph();
 	AlgebraicExpression *add = NULL;
 	AlgebraicExpression *root = NULL;
 	AlgebraicExpression *src_filter = NULL;
@@ -537,3 +536,4 @@ AlgebraicExpression **AlgebraicExpression_FromQueryGraph
 	QueryGraph_Free(g);
 	return exps;
 }
+

src/arithmetic/algebraic_expression/algebraic_expression_eval.c

@@ -29,7 +29,6 @@ static GrB_Matrix _Eval_Transpose
 static GrB_Matrix _Eval_Add(const AlgebraicExpression *exp, GrB_Matrix res) {
 	assert(exp && AlgebraicExpression_ChildCount(exp) > 1);
 
-	GrB_Info info;
 	GrB_Index nrows;                // Number of rows of operand.
 	GrB_Index ncols;                // Number of columns of operand.
 	bool res_in_use = false;        // Can we use `res` for intermediate evaluation.
@@ -48,6 +47,7 @@ static GrB_Matrix _Eval_Add(const AlgebraicExpression *exp, GrB_Matrix res) {
 		a = left->operand.matrix;
 	} else {
 		if(left->operation.op == AL_EXP_TRANSPOSE) {
+			assert(AlgebraicExpression_ChildCount(left) == 1);
 			a = left->operation.children[0]->operand.matrix;
 			if(desc == GrB_NULL) GrB_Descriptor_new(&desc);
 			GrB_Descriptor_set(desc, GrB_INP0, GrB_TRAN);
@@ -63,18 +63,16 @@ static GrB_Matrix _Eval_Add(const AlgebraicExpression *exp, GrB_Matrix res) {
 		b = right->operand.matrix;
 	} else {
 		if(right->operation.op == AL_EXP_TRANSPOSE) {
+			assert(AlgebraicExpression_ChildCount(right) == 1);
 			b = right->operation.children[0]->operand.matrix;
 			if(desc == GrB_NULL) GrB_Descriptor_new(&desc);
 			GrB_Descriptor_set(desc, GrB_INP1, GrB_TRAN);
 		} else if(res_in_use) {
 			// `res` is in use, create an additional matrix.
 			GrB_Matrix_nrows(&nrows, a);
 			GrB_Matrix_ncols(&ncols, a);
-			info = GrB_Matrix_new(&inter, GrB_BOOL, nrows, ncols);
-			if(info != GrB_SUCCESS) {
-				fprintf(stderr, "%s", GrB_error());
-				assert(false);
-			}
+			assert(GrB_Matrix_new(&inter, GrB_BOOL, nrows, ncols)
+					== GrB_SUCCESS);
 			b = _AlgebraicExpression_Eval(right, inter);
 		} else {
 			// `res` is not used just yet, use it for RHS evaluation.
@@ -83,11 +81,8 @@ static GrB_Matrix _Eval_Add(const AlgebraicExpression *exp, GrB_Matrix res) {
 	}
 
 	// Perform addition.
-	if(GrB_eWiseAdd_Matrix_Semiring(res, GrB_NULL, GrB_NULL, GxB_ANY_PAIR_BOOL, a, b,
-									desc) != GrB_SUCCESS) {
-		printf("Failed adding operands, error:%s\n", GrB_error());
-		assert(false);
-	}
+	assert(GrB_eWiseAdd_Matrix_Semiring(res, GrB_NULL, GrB_NULL,
+				GxB_ANY_PAIR_BOOL, a, b, desc)  == GrB_SUCCESS);
 
 	// Reset descriptor if non-null.
 	if(desc != GrB_NULL) GrB_Descriptor_set(desc, GrB_INP0, GxB_DEFAULT);
@@ -103,24 +98,26 @@ static GrB_Matrix _Eval_Add(const AlgebraicExpression *exp, GrB_Matrix res) {
 			b = right->operand.matrix;
 		} else {
 			if(right->operation.op == AL_EXP_TRANSPOSE) {
+				assert(AlgebraicExpression_ChildCount(right) == 1);
 				b = right->operation.children[0]->operand.matrix;
 				if(desc == GrB_NULL) GrB_Descriptor_new(&desc);
 				GrB_Descriptor_set(desc, GrB_INP1, GrB_TRAN);
-			} else if(inter == GrB_NULL) {
-				// Can't use `res`, use an intermidate matrix.
-				GrB_Matrix_nrows(&nrows, res);
-				GrB_Matrix_ncols(&ncols, res);
-				GrB_Matrix_new(&inter, GrB_BOOL, nrows, ncols);
+			} else {
+				// 'right' represents either + or * operation.
+				if(inter == GrB_NULL) {
+					// Can't use `res`, use an intermidate matrix.
+					GrB_Matrix_nrows(&nrows, res);
+					GrB_Matrix_ncols(&ncols, res);
+					assert(GrB_Matrix_new(&inter, GrB_BOOL, nrows, ncols)
+							== GrB_SUCCESS);
+				}
+				b = _AlgebraicExpression_Eval(right, inter);
 			}
-			b = _AlgebraicExpression_Eval(right, inter);
 		}
 
 		// Perform addition.
-		if(GrB_eWiseAdd_Matrix_Semiring(res, GrB_NULL, GrB_NULL, GxB_ANY_PAIR_BOOL, res, b,
-										GrB_NULL) != GrB_SUCCESS) {
-			printf("Failed adding operands, error:%s\n", GrB_error());
-			assert(false);
-		}
+		assert(GrB_eWiseAdd_Matrix_Semiring(res, GrB_NULL, GrB_NULL,
+					GxB_ANY_PAIR_BOOL, res, b, GrB_NULL) == GrB_SUCCESS);
 	}
 
 	if(inter != GrB_NULL) GrB_Matrix_free(&inter);
@@ -135,14 +132,14 @@ static GrB_Matrix _Eval_Mul(const AlgebraicExpression *exp, GrB_Matrix res) {
 
 	GrB_Matrix A;
 	GrB_Matrix B;
-	GrB_Info info;
 	GrB_Index nvals;
 	GrB_Descriptor desc = GrB_NULL;
 	AlgebraicExpression *left = CHILD_AT(exp, 0);
 	AlgebraicExpression *right = CHILD_AT(exp, 1);
 
 	if(left->type == AL_OPERATION) {
 		assert(left->operation.op == AL_EXP_TRANSPOSE);
+		assert(AlgebraicExpression_ChildCount(left) == 1);
 		if(desc == GrB_NULL) GrB_Descriptor_new(&desc);
 		GrB_Descriptor_set(desc, GrB_INP0, GrB_TRAN);
 		left = CHILD_AT(left, 0);
@@ -151,6 +148,7 @@ static GrB_Matrix _Eval_Mul(const AlgebraicExpression *exp, GrB_Matrix res) {
 
 	if(right->type == AL_OPERATION) {
 		assert(right->operation.op == AL_EXP_TRANSPOSE);
+		assert(AlgebraicExpression_ChildCount(right) == 1);
 		if(desc == GrB_NULL) GrB_Descriptor_new(&desc);
 		GrB_Descriptor_set(desc, GrB_INP1, GrB_TRAN);
 		right = CHILD_AT(right, 0);
@@ -161,20 +159,12 @@ static GrB_Matrix _Eval_Mul(const AlgebraicExpression *exp, GrB_Matrix res) {
 		// Reset descriptor, as the identity matrix does not need to be transposed.
 		if(desc != GrB_NULL) GrB_Descriptor_set(desc, GrB_INP1, GxB_DEFAULT);
 		// B is the identity matrix, Perform A * I.
-		info = GrB_Matrix_apply(res, GrB_NULL, GrB_NULL, GrB_IDENTITY_BOOL, A, desc);
-		if(info != GrB_SUCCESS) {
-			// If the multiplication failed, print error info to stderr and exit.
-			fprintf(stderr, "Encountered an error in matrix multiplication:\n%s\n", GrB_error());
-			assert(false);
-		}
+		assert(GrB_Matrix_apply(res, GrB_NULL, GrB_NULL, GrB_IDENTITY_BOOL, A,
+					desc) == GrB_SUCCESS);
 	} else {
 		// Perform multiplication.
-		info = GrB_mxm(res, GrB_NULL, GrB_NULL, GxB_ANY_PAIR_BOOL, A, B, desc);
-		if(info != GrB_SUCCESS) {
-			// If the multiplication failed, print error info to stderr and exit.
-			fprintf(stderr, "Encountered an error in matrix multiplication:\n%s\n", GrB_error());
-			assert(false);
-		}
+		assert(GrB_mxm(res, GrB_NULL, GrB_NULL, GxB_ANY_PAIR_BOOL, A, B, desc)
+				== GrB_SUCCESS);
 	}
 
 	GrB_Matrix_nvals(&nvals, res);
@@ -190,6 +180,7 @@ static GrB_Matrix _Eval_Mul(const AlgebraicExpression *exp, GrB_Matrix res) {
 		right = CHILD_AT(exp, i);
 		if(right->type == AL_OPERATION) {
 			assert(right->operation.op == AL_EXP_TRANSPOSE);
+			assert(AlgebraicExpression_ChildCount(right) == 1);
 			if(desc == GrB_NULL) GrB_Descriptor_new(&desc);
 			GrB_Descriptor_set(desc, GrB_INP1, GrB_TRAN);
 			right = CHILD_AT(right, 0);
@@ -200,12 +191,8 @@ static GrB_Matrix _Eval_Mul(const AlgebraicExpression *exp, GrB_Matrix res) {
 			// Reset descriptor, as the identity matrix does not need to be transposed.
 			if(desc != GrB_NULL) GrB_Descriptor_set(desc, GrB_INP1, GxB_DEFAULT);
 			// Perform multiplication.
-			info = GrB_mxm(res, GrB_NULL, GrB_NULL, GxB_ANY_PAIR_BOOL, res, B, desc);
-			if(info != GrB_SUCCESS) {
-				// If the multiplication failed, print error info to stderr and exit.
-				fprintf(stderr, "Encountered an error in matrix multiplication:\n%s\n", GrB_error());
-				assert(false);
-			}
+			assert(GrB_mxm(res, GrB_NULL, GrB_NULL, GxB_ANY_PAIR_BOOL, res, B,
+						desc) == GrB_SUCCESS);
 		}
 		GrB_Matrix_nvals(&nvals, res);
 		if(nvals == 0) break;

src/arithmetic/entity_funcs/entity_funcs.c

@@ -113,7 +113,7 @@ SIValue AR_PROPERTY(SIValue *argv, int argc) {
 	// We have the property string, attempt to look up the index now.
 	if(prop_idx == ATTRIBUTE_NOTFOUND) {
 		GraphContext *gc = QueryCtx_GetGraphCtx();
-		prop_idx = GraphContext_GetAttributeID(gc, argv[1].stringval);
+		prop_idx = GraphContext_GetAttributeID(gc, prop_name);
 	}
 
 	// Retrieve the property.

src/ast/ast.c

@@ -326,8 +326,6 @@ bool AST_IdentifierIsAlias(const cypher_astnode_t *root, const char *identifier)
 			const char *alias = cypher_ast_identifier_get_name(alias_node);
 			if(!strcmp(alias, identifier)) return true; // The identifier is an alias.
 		} else {
-			// Retrieve the projected expression.
-			const cypher_astnode_t *expression_node = cypher_ast_projection_get_expression(root);
 			if(cypher_astnode_type(root) == CYPHER_AST_IDENTIFIER) {
 				// If the projection itself is the identifier, it is not an alias.
 				const char *current_identifier = cypher_ast_identifier_get_name(alias_node);

src/ast/ast_build_op_contexts.c

@@ -16,8 +16,8 @@ static inline EdgeCreateCtx _NewEdgeCreateCtx(GraphContext *gc, const QGEdge *e,
 											  const cypher_astnode_t *edge) {
 	const cypher_astnode_t *props = cypher_ast_rel_pattern_get_properties(edge);
 
-	EdgeCreateCtx new_edge = {	.alias = e->alias,
-		                        .relation = e->reltypes[0],
+	EdgeCreateCtx new_edge = {  .alias = e->alias,
+								.relation = e->reltypes[0],
 								.reltypeId = e->reltypeIDs[0],
 								.properties = PropertyMap_New(gc, props),
 								.src = e->src->alias,
@@ -31,7 +31,7 @@ static inline NodeCreateCtx _NewNodeCreateCtx(GraphContext *gc, const QGNode *n,
 	const cypher_astnode_t *ast_props = cypher_ast_node_pattern_get_properties(ast_node);
 
 	NodeCreateCtx new_node = {  .alias = n->alias,
-		                        .label = n->label,
+								.label = n->label,
 								.labelId = n->labelID,
 								.properties = PropertyMap_New(gc, ast_props)
 							 };
@@ -87,7 +87,6 @@ EntityUpdateEvalCtx *AST_PrepareUpdateOp(GraphContext *gc, const cypher_astnode_
 }
 
 AR_ExpNode **AST_PrepareDeleteOp(const cypher_astnode_t *delete_clause) {
-	AST *ast = QueryCtx_GetAST();
 	uint delete_count = cypher_ast_delete_nexpressions(delete_clause);
 	AR_ExpNode **exps = array_new(AR_ExpNode *, delete_count);
 

src/ast/ast_validations.c

@@ -309,18 +309,18 @@ static AST_Validation _Validate_Path_Locations(const cypher_astnode_t *root) {
 		if(child_type == CYPHER_AST_PATTERN_PATH) {
 			const cypher_astnode_type_t root_type = cypher_astnode_type(root);
 			if(root_type != CYPHER_AST_PATTERN &&
-					root_type != CYPHER_AST_MATCH &&
-					root_type != CYPHER_AST_MERGE &&
-					root_type != CYPHER_AST_WITH &&
-					root_type != CYPHER_AST_NAMED_PATH &&
-					root_type != CYPHER_AST_UNARY_OPERATOR &&
-					root_type != CYPHER_AST_BINARY_OPERATOR) {
+			   root_type != CYPHER_AST_MATCH &&
+			   root_type != CYPHER_AST_MERGE &&
+			   root_type != CYPHER_AST_WITH &&
+			   root_type != CYPHER_AST_NAMED_PATH &&
+			   root_type != CYPHER_AST_UNARY_OPERATOR &&
+			   root_type != CYPHER_AST_BINARY_OPERATOR) {
 				QueryCtx_SetError("Encountered path traversal in unsupported location '%s'",
-						cypher_astnode_typestr(child_type));
+								  cypher_astnode_typestr(child_type));
 				return AST_INVALID;
 			}
 		}
-		if (_Validate_Path_Locations(child) != AST_VALID) return AST_INVALID;
+		if(_Validate_Path_Locations(child) != AST_VALID) return AST_INVALID;
 	}
 	return AST_VALID;
 }
@@ -1010,8 +1010,6 @@ static AST_Validation _ValidateQuerySequence(const AST *ast) {
 	// Validate the final clause
 	if(_ValidateQueryTermination(ast) != AST_VALID) return AST_INVALID;
 
-	uint clause_count = cypher_ast_query_nclauses(ast->root);
-
 	// The query cannot begin with a "WITH/RETURN *" projection.
 	const cypher_astnode_t *start_clause = cypher_ast_query_get_clause(ast->root, 0);
 	if(cypher_astnode_type(start_clause) == CYPHER_AST_WITH &&
@@ -1438,8 +1436,9 @@ static AST_Validation _ValidateUnion_Clauses(const AST *ast) {
 	uint return_clause_count = array_len(return_indices);
 	// We should have one more RETURN clause than we have UNION clauses.
 	if(return_clause_count != union_clause_count + 1) {
-			QueryCtx_SetError("Found %d UNION clauses but only %d RETURN clauses.", union_clause_count, return_clause_count);
-			return AST_INVALID;
+		QueryCtx_SetError("Found %d UNION clauses but only %d RETURN clauses.", union_clause_count,
+						  return_clause_count);
+		return AST_INVALID;
 	}
 
 	const cypher_astnode_t *return_clause = cypher_ast_query_get_clause(ast->root, return_indices[0]);

src/commands/cmd_profile.c

@@ -14,6 +14,7 @@
 #include "../execution_plan/execution_plan.h"
 
 void Graph_Profile(void *args) {
+	bool readonly = true;
 	bool lockAcquired = false;
 	ResultSet *result_set = NULL;
 	CommandCtx *command_ctx = (CommandCtx *)args;
@@ -49,7 +50,7 @@ void Graph_Profile(void *args) {
 		goto cleanup;
 	}
 
-	bool readonly = AST_ReadOnly(ast->root);
+	readonly = AST_ReadOnly(ast->root);
 
 	// Acquire the appropriate lock.
 	if(readonly) {
@@ -91,3 +92,4 @@ void Graph_Profile(void *args) {
 	CommandCtx_Free(command_ctx);
 	QueryCtx_Free(); // Reset the QueryCtx and free its allocations.
 }
+

src/commands/cmd_query.c

@@ -73,6 +73,7 @@ void Query_SetTimeOut(uint timeout, ExecutionPlan *plan) {
 }
 
 void Graph_Query(void *args) {
+	bool readonly = true;
 	bool lockAcquired = false;
 	ResultSet *result_set = NULL;
 	CommandCtx *command_ctx = (CommandCtx *)args;
@@ -103,7 +104,7 @@ void Graph_Query(void *args) {
 	}
 	if(exec_type == EXECUTION_TYPE_INVALID) goto cleanup;
 
-	bool readonly = AST_ReadOnly(ast->root);
+	readonly = AST_ReadOnly(ast->root);
 	if(!readonly && _readonly_cmd_mode(command_ctx)) {
 		QueryCtx_SetError("graph.RO_QUERY is to be executed only on read-only queries");
 		QueryCtx_EmitException();

src/execution_plan/execution_plan.c

@@ -125,7 +125,7 @@ static OpBase *_ExecutionPlan_FindLastWriter(OpBase *root) {
 }
 
 static ExecutionPlan *_process_segment(AST *ast, uint segment_start_idx,
-		uint segment_end_idx) {
+									   uint segment_end_idx) {
 	ASSERT(ast != NULL);
 	ASSERT(segment_start_idx <= segment_end_idx);
 
@@ -194,7 +194,7 @@ static ExecutionPlan **_process_segments(AST *ast) {
 }
 
 static ExecutionPlan *_tie_segments(ExecutionPlan **segments,
-		uint segment_count) {
+									uint segment_count) {
 	FT_FilterNode *ft = NULL;            // filters following WITH
 	OpBase *connecting_op = NULL;        // op connecting one segment to another
 	OpBase *prev_connecting_op = NULL;   // root of previous segment
@@ -332,7 +332,7 @@ Record ExecutionPlan_BorrowRecord(ExecutionPlan *plan) {
 	// Get a Record from the pool and set its owner and mapping.
 	Record r = ObjectPool_NewItem(plan->record_pool);
 	r->owner = plan;
-	r->mapping = plan->record_map;
+	r->mapping = mapping;
 	return r;
 }
 

src/execution_plan/execution_plan_build/build_projection_ops.c

@@ -33,7 +33,6 @@ static AR_ExpNode **_PopulateProjectAll(const cypher_astnode_t *clause) {
 static AR_ExpNode **_BuildOrderExpressions(AR_ExpNode **projections,
 										   const cypher_astnode_t *order_clause) {
 	AST *ast = QueryCtx_GetAST();
-	uint projection_count = array_len(projections);
 	uint count = cypher_ast_order_by_nitems(order_clause);
 	AR_ExpNode **order_exps = array_new(AR_ExpNode *, count);
 
@@ -164,7 +163,6 @@ static inline void _buildProjectionOps(ExecutionPlan *plan,
 	bool distinct = false;
 	bool aggregate = false;
 	int *sort_directions = NULL;
-	AST *ast = QueryCtx_GetAST();
 	AR_ExpNode **order_exps = NULL;
 	AR_ExpNode **projections = NULL;
 	const cypher_astnode_t *skip_clause = NULL;

src/execution_plan/ops/op_apply_multiplexer.c

@@ -54,7 +54,6 @@ static void _OpApplyMultiplexer_SortChildren(OpBase *op) {
 			for(int j = i + 1; j < op->childCount; j++) {
 				OpBase *candidate = op->children[j];
 				if(candidate->type == OPType_FILTER) {
-					OpBase *tmp = candidate;
 					op->children[i] = candidate;
 					op->children[j] = child;
 					swapped = true;

src/execution_plan/ops/op_filter.c

@@ -25,7 +25,7 @@ OpBase *NewFilterOp(const ExecutionPlan *plan, FT_FilterNode *filterTree) {
 /* FilterConsume next operation
  * returns OP_OK when graph passes filter tree. */
 static Record FilterConsume(OpBase *opBase) {
-	Record r;
+	Record r = NULL;
 	OpFilter *filter = (OpFilter *)opBase;
 	OpBase *child = filter->op.children[0];
 

src/execution_plan/ops/op_semi_apply.c

@@ -124,7 +124,6 @@ static OpResult SemiApplyReset(OpBase *opBase) {
 
 static inline OpBase *SemiApplyClone(const ExecutionPlan *plan, const OpBase *opBase) {
 	assert(opBase->type == OPType_SEMI_APPLY || opBase->type == OPType_ANTI_SEMI_APPLY);
-	OpSemiApply *op = (OpSemiApply *)opBase;
 	bool anti = opBase->type == OPType_ANTI_SEMI_APPLY;
 	return NewSemiApplyOp(plan, anti);
 }

src/execution_plan/ops/op_update.c

@@ -223,7 +223,6 @@ static void _EvalEntityUpdates(EntityUpdateCtx *ctx, GraphContext *gc,
 	Schema *s = NULL;
 	const char *label = NULL;
 	bool update_index = false;
-	int label_id = GRAPH_NO_LABEL;
 
 	// Get the type of the entity to update. If the expected entity was not
 	// found, make no updates but do not error.
@@ -246,7 +245,7 @@ static void _EvalEntityUpdates(EntityUpdateCtx *ctx, GraphContext *gc,
 		// Retrieve the node's local label if present.
 		label = NODE_GET_LABEL(n);
 		// Retrieve the node's Label ID from a local member or the graph.
-		label_id = NODE_GET_LABEL_ID(n, gc->g);
+		int label_id = NODE_GET_LABEL_ID(n, gc->g);
 		if((label == NULL) && (label_id != GRAPH_NO_LABEL)) {
 			// Label ID has been found but its name is unknown, retrieve its name and update the node.
 			s = GraphContext_GetSchemaByID(gc, label_id, SCHEMA_NODE);