Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds errors for non-declared procedures #245

Merged
merged 20 commits into from
Sep 18, 2024
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/chatty-plants-greet.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@neo4j-cypher/language-support': patch
---

Adds errors for undeclared procedures / functions
60 changes: 36 additions & 24 deletions packages/language-support/src/parserWrapper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
LabelNameContext,
LabelNameIsContext,
LabelOrRelTypeContext,
ProcedureNameContext,
StatementOrCommandContext,
StatementsOrCommandsContext,
SymbolicNameStringContext,
Expand Down Expand Up @@ -39,11 +40,12 @@ export interface ParsedStatement {
// A statement needs to be parsed with the .statements() rule because
// it's the one that tries to parse until the EOF
ctx: StatementsOrCommandsContext;
diagnostics: SyntaxDiagnostic[];
syntaxErrors: SyntaxDiagnostic[];
Copy link
Collaborator

Choose a reason for hiding this comment

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

why rename this? I think even if "syntaxErrors" could be more desriptive, it's nice to match the type name imo

Copy link
Collaborator

Choose a reason for hiding this comment

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

It makes the code more readable in my opinion. Especially when we are using web workers and we need that logic of: no syntax errors, then execute the web worker. It reads better than diagnostics.

Another example is what has changed in syntaxValidation.ts:

    const anySyntacticError =
      statements.filter((statement) => statement.syntaxErrors.length !== 0)
        .length > 0;

We know what's happening inside better than if it read diagnostics.

What we could do is rename this type:

export type SyntaxDiagnostic = Diagnostic & {
  offsets: { start: number; end: number };
};

where Diagnostic is coming from the vscode-types. Any suggestions?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't feel very strongly about it & agree syntaxError reads better. Part of me was assuming there could be more than errors from a "Diagnostics" though

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's beside the point but for the code example you could use [some](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/some) instead, it's more efficient as it stops iterating as soon as it finds a match

const anySyntacticError =
      statements.some((statement) => statement.syntaxErrors.length !== 0)

stopNode: ParserRuleContext;
collectedLabelOrRelTypes: LabelOrRelType[];
collectedVariables: string[];
collectedFunctions: ParsedFunction[];
collectedProcedures: ParsedProcedure[];
}

export interface ParsingResult {
Expand Down Expand Up @@ -97,7 +99,7 @@ export type LabelOrRelType = {
};

export type ParsedFunction = {
parsedName: string;
name: string;
rawText: string;
line: number;
column: number;
Expand All @@ -106,6 +108,7 @@ export type ParsedFunction = {
end: number;
};
};
export type ParsedProcedure = ParsedFunction;

export function createParsingScaffolding(query: string): ParsingScaffolding {
const inputStream = CharStreams.fromString(query);
Expand Down Expand Up @@ -171,37 +174,34 @@ export function createParsingResult(query: string): ParsingResult {
const { parser, tokens } = statementScaffolding;
const labelsCollector = new LabelAndRelTypesCollector();
const variableFinder = new VariableCollector();
const functionFinder = new FunctionCollector(tokens);
const methodsFinder = new MethodsCollector(tokens);
const errorListener = new SyntaxErrorsListener(tokens);
parser._parseListeners = [
labelsCollector,
variableFinder,
functionFinder,
];
parser._parseListeners = [labelsCollector, variableFinder, methodsFinder];
parser.addErrorListener(errorListener);
const ctx = parser.statementsOrCommands();
// The statement is empty if we cannot find anything that is not EOF or a space
const isEmptyStatement =
tokens.find(
(t) => t.text !== '<EOF>' && t.type !== CypherLexer.SPACE,
) === undefined;
const diagnostics = isEmptyStatement ? [] : errorListener.errors;
const syntaxErrors = isEmptyStatement ? [] : errorListener.errors;
const collectedCommand = parseToCommand(ctx, isEmptyStatement);

if (!_internalFeatureFlags.consoleCommands) {
diagnostics.push(...errorOnNonCypherCommands(collectedCommand));
syntaxErrors.push(...errorOnNonCypherCommands(collectedCommand));
}

return {
command: collectedCommand,
parser: parser,
tokens: tokens,
diagnostics: diagnostics,
syntaxErrors: syntaxErrors,
ctx: ctx,
stopNode: findStopNode(ctx),
collectedLabelOrRelTypes: labelsCollector.labelOrRelTypes,
collectedVariables: variableFinder.variables,
collectedFunctions: functionFinder.functions,
collectedFunctions: methodsFinder.functions,
collectedProcedures: methodsFinder.procedures,
};
});

Expand Down Expand Up @@ -310,8 +310,9 @@ class VariableCollector implements ParseTreeListener {
}
}

// This listener collects all functions
class FunctionCollector extends ParseTreeListener {
// This listener collects all functions and procedures
class MethodsCollector extends ParseTreeListener {
public procedures: ParsedProcedure[] = [];
public functions: ParsedFunction[] = [];
private tokens: Token[];

Expand All @@ -331,8 +332,11 @@ class FunctionCollector extends ParseTreeListener {
}

exitEveryRule(ctx: unknown) {
if (ctx instanceof FunctionNameContext) {
const functionName = this.getNormalizedFunctionName(ctx);
if (
ctx instanceof FunctionNameContext ||
ctx instanceof ProcedureNameContext
) {
const methodName = this.getMethodName(ctx);

const startTokenIndex = ctx.start.tokenIndex;
const stopTokenIndex = ctx.stop.tokenIndex;
Expand All @@ -344,33 +348,41 @@ class FunctionCollector extends ParseTreeListener {
})
.join('');

this.functions.push({
parsedName: functionName,
const result = {
name: methodName,
rawText: rawText,
line: ctx.start.line,
column: ctx.start.column,
offsets: {
start: ctx.start.start,
end: ctx.stop.stop + 1,
},
});
};

if (ctx instanceof FunctionNameContext) {
this.functions.push(result);
} else {
this.procedures.push(result);
}
}
}

private getNormalizedFunctionName(ctx: FunctionNameContext): string {
private getMethodName(
ctx: ProcedureNameContext | FunctionNameContext,
): string {
const namespaces = ctx.namespace().symbolicNameString_list();
const functionName = ctx.symbolicNameString();
const methodName = ctx.symbolicNameString();

const normalizedName = [...namespaces, functionName]
const normalizedName = [...namespaces, methodName]
.map((symbolicName) => {
return this.getFunctionNamespaceString(symbolicName);
return this.getNamespaceString(symbolicName);
})
.join('.');

return normalizedName;
}

private getFunctionNamespaceString(ctx: SymbolicNameStringContext): string {
private getNamespaceString(ctx: SymbolicNameStringContext): string {
const text = ctx.getText();
const isEscaped = Boolean(ctx.escapedSymbolicNameString());
const hasDot = text.includes('.');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,10 @@ export function wrappedSemanticAnalysis(
try {
let semanticErrorsResult = undefined;

if (dbSchema.functions && dbSchema.procedures) {
updateSignatureResolver({
procedures: Object.values(dbSchema.procedures),
functions: Object.values(dbSchema.functions),
});
}
updateSignatureResolver({
procedures: Object.values(dbSchema.procedures ?? {}),
functions: Object.values(dbSchema.functions ?? {}),
});
semanticAnalysis([query], (a) => {
semanticErrorsResult = a;
});
Expand Down
110 changes: 88 additions & 22 deletions packages/language-support/src/syntaxValidation/syntaxValidation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,11 @@ import {
LabelOrRelType,
LabelType,
ParsedFunction,
ParsedProcedure,
ParsedStatement,
parserWrapper,
} from '../parserWrapper';
import { Neo4jFunction } from '../types';
import { Neo4jFunction, Neo4jProcedure } from '../types';
import {
SemanticAnalysisElement,
wrappedSemanticAnalysis,
Expand Down Expand Up @@ -63,7 +64,7 @@ function detectNonDeclaredFunction(
parsedFunction: ParsedFunction,
functionsSchema: Record<string, Neo4jFunction>,
): SyntaxDiagnostic | undefined {
const lowercaseFunctionName = parsedFunction.parsedName.toLowerCase();
const lowercaseFunctionName = parsedFunction.name.toLowerCase();
const caseInsensitiveFunctionInDatabase =
functionsSchema[lowercaseFunctionName];

Expand All @@ -76,14 +77,26 @@ function detectNonDeclaredFunction(
}

const functionExistsWithExactName = Boolean(
functionsSchema[parsedFunction.parsedName],
functionsSchema[parsedFunction.name],
);
if (!functionExistsWithExactName) {
return generateFunctionNotFoundWarning(parsedFunction);
return generateFunctionNotFoundError(parsedFunction);
}
}

function generateFunctionNotFoundWarning(
function detectNonDeclaredProcedure(
parsedProcedure: ParsedProcedure,
proceduresSchema: Record<string, Neo4jProcedure>,
): SyntaxDiagnostic | undefined {
const procedureName = parsedProcedure.name;
const procedureExists = Boolean(proceduresSchema[procedureName]);

if (!procedureExists) {
return generateProcedureNotFoundError(parsedProcedure);
}
}

function generateFunctionNotFoundError(
parsedFunction: ParsedFunction,
): SyntaxDiagnostic {
const rawText = parsedFunction.rawText;
Expand All @@ -96,17 +109,43 @@ function generateFunctionNotFoundWarning(
? startColumn + rawText.length
: nameChunks.at(-1)?.length ?? 0;

const warning: SyntaxDiagnostic = {
severity: DiagnosticSeverity.Warning,
const error: SyntaxDiagnostic = {
severity: DiagnosticSeverity.Error,
range: {
start: Position.create(lineIndex, startColumn),
end: Position.create(lineIndex + linesOffset, endColumn),
},
offsets: parsedFunction.offsets,
message: `Function ${parsedFunction.parsedName} is not present in the database. Make sure you didn't misspell it or that it is available when you run this statement in your application`,
message: `Function ${parsedFunction.name} is not present in the database. Make sure you didn't misspell it or that it is available when you run this statement in your application`,
};

return warning;
return error;
}

function generateProcedureNotFoundError(
parsedProcedure: ParsedProcedure,
): SyntaxDiagnostic {
const rawText = parsedProcedure.rawText;
const nameChunks = rawText.split('\n');
const linesOffset = nameChunks.length - 1;
const lineIndex = parsedProcedure.line - 1;
const startColumn = parsedProcedure.column;
const endColumn =
linesOffset == 0
? startColumn + rawText.length
: nameChunks.at(-1)?.length ?? 0;

const error: SyntaxDiagnostic = {
severity: DiagnosticSeverity.Error,
range: {
start: Position.create(lineIndex, startColumn),
end: Position.create(lineIndex + linesOffset, endColumn),
},
offsets: parsedProcedure.offsets,
message: `Procedure ${parsedProcedure.name} is not present in the database. Make sure you didn't misspell it or that it is available when you run this statement in your application`,
};

return error;
}

function warnOnUndeclaredLabels(
Expand Down Expand Up @@ -226,12 +265,13 @@ export function lintCypherQuery(
dbSchema: DbSchema,
): SyntaxDiagnostic[] {
const syntaxErrors = validateSyntax(query, dbSchema);
if (syntaxErrors.length > 0) {
// If there are any syntactic errors in the query, do not run the semantic validation
if (syntaxErrors.find((d) => d.severity === DiagnosticSeverity.Error)) {
return syntaxErrors;
}

const semanticErrors = validateSemantics(query, dbSchema);
return semanticErrors;
return syntaxErrors.concat(semanticErrors);
}

export function validateSyntax(
Expand All @@ -243,13 +283,9 @@ export function validateSyntax(
}
const statements = parserWrapper.parse(query);
const result = statements.statementsParsing.flatMap((statement) => {
const diagnostics = statement.diagnostics;
const syntaxErrors = statement.syntaxErrors;
const labelWarnings = warnOnUndeclaredLabels(statement, dbSchema);
const functionWarnings = warnOnUndeclaredFunctions(statement, dbSchema);

return diagnostics
.concat(labelWarnings, functionWarnings)
.sort(sortByPositionAndMessage);
return syntaxErrors.concat(labelWarnings).sort(sortByPositionAndMessage);
});

return result;
Expand All @@ -266,20 +302,28 @@ export function validateSemantics(
const cachedParse = parserWrapper.parse(query);
const statements = cachedParse.statementsParsing;
const semanticErrors = statements.flatMap((current) => {
if (current.diagnostics.length === 0) {
if (current.syntaxErrors.length === 0) {
const cmd = current.command;
if (cmd.type === 'cypher' && cmd.statement.length > 0) {
const functionErrors = errorOnUndeclaredFunctions(current, dbSchema);
const procedureErrors = errorOnUndeclaredProcedures(
current,
dbSchema,
);

const { notifications, errors } = wrappedSemanticAnalysis(
cmd.statement,
dbSchema,
);

const elements = notifications.concat(errors);
const result = fixSemanticAnalysisPositions({
const semanticDiagnostics = fixSemanticAnalysisPositions({
semanticElements: elements,
parseResult: current,
}).sort(sortByPositionAndMessage);
return result;
});
return semanticDiagnostics
.concat(functionErrors, procedureErrors)
.sort(sortByPositionAndMessage);
}
}
return [];
Expand All @@ -291,7 +335,7 @@ export function validateSemantics(
return [];
}

function warnOnUndeclaredFunctions(
function errorOnUndeclaredFunctions(
parsingResult: ParsedStatement,
dbSchema: DbSchema,
): SyntaxDiagnostic[] {
Expand All @@ -312,3 +356,25 @@ function warnOnUndeclaredFunctions(

return warnings;
}

function errorOnUndeclaredProcedures(
parsingResult: ParsedStatement,
dbSchema: DbSchema,
): SyntaxDiagnostic[] {
const errors: SyntaxDiagnostic[] = [];

if (dbSchema.procedures) {
const proceduresInQuery = parsingResult.collectedProcedures;

proceduresInQuery.forEach((parsedProcedure) => {
const error = detectNonDeclaredProcedure(
parsedProcedure,
dbSchema.procedures,
);

if (error) errors.push(error);
});
ncordon marked this conversation as resolved.
Show resolved Hide resolved
}

return errors;
}
Loading