From 92e60aaa6344d43704a501ae305edaf44d72fb83 Mon Sep 17 00:00:00 2001 From: mr25mr <100434800+mr25mr@users.noreply.github.com> Date: Wed, 19 Jul 2023 18:15:00 +0200 Subject: [PATCH] fix: unjustified errors for property binding info (#639) * fix: unjustified error for property binding info * chore: change set * fix: review comments * fix: add html equivalent --- .changeset/shiny-maps-guess.md | 8 + packages/binding-parser/src/api.ts | 6 +- packages/binding-parser/src/lexer/token.ts | 2 +- .../src/types/binding-parser.ts | 9 +- .../binding-parser/src/utils/expression.ts | 148 +++++++++++++++ packages/binding-parser/src/utils/index.ts | 14 ++ .../test/unit/utils/expression.test.ts | 172 ++++++++++++++++++ .../providers/property-binding-info.ts | 13 +- .../property-binding-info-validator.ts | 14 +- packages/binding/src/utils/cursor.ts | 10 +- packages/binding/src/utils/expression.ts | 50 ----- packages/binding/src/utils/index.ts | 6 +- .../property-binding-info-validator.test.ts | 29 +++ .../test/unit/utils/expression.test.ts | 71 +------- 14 files changed, 401 insertions(+), 151 deletions(-) create mode 100644 .changeset/shiny-maps-guess.md create mode 100644 packages/binding-parser/src/utils/expression.ts create mode 100644 packages/binding-parser/src/utils/index.ts create mode 100644 packages/binding-parser/test/unit/utils/expression.test.ts diff --git a/.changeset/shiny-maps-guess.md b/.changeset/shiny-maps-guess.md new file mode 100644 index 000000000..f1c628994 --- /dev/null +++ b/.changeset/shiny-maps-guess.md @@ -0,0 +1,8 @@ +--- +"@ui5-language-assistant/vscode-ui5-language-assistant-bas-ext": patch +"vscode-ui5-language-assistant": patch +"@ui5-language-assistant/binding-parser": patch +"@ui5-language-assistant/binding": patch +--- + +fix unjustified errors for property binding info diff --git a/packages/binding-parser/src/api.ts b/packages/binding-parser/src/api.ts index 5632b2881..e627f967a 100644 --- a/packages/binding-parser/src/api.ts +++ b/packages/binding-parser/src/api.ts @@ -7,7 +7,11 @@ export { isBeforeAdjacentRange, positionContained, rangeContained, -} from "./utils/position"; + isBindingExpression, + isMetadataPath, + isModel, + isPropertyBindingInfo, +} from "./utils"; import { Value, diff --git a/packages/binding-parser/src/lexer/token.ts b/packages/binding-parser/src/lexer/token.ts index 0cdcb98eb..35b245f8f 100644 --- a/packages/binding-parser/src/lexer/token.ts +++ b/packages/binding-parser/src/lexer/token.ts @@ -25,7 +25,7 @@ const whiteSpace = createToken({ const specialChars = createToken({ name: SPECIAL_CHARS, pattern: - /(?:#|!|"|\$|%|&|'|\(|\)|\*|\+|-|\.|\/|;|<|=|>|\?|@|\\|\^|_|`|~|\||)+/, + /(?:#|>|/|/|!|"|\$|%|&|'|\(|\)|\*|\+|-|\.|\/|;|<|=|>|\?|@|\\|\^|_|`|~|\||)+/, }); const leftCurly = createToken({ diff --git a/packages/binding-parser/src/types/binding-parser.ts b/packages/binding-parser/src/types/binding-parser.ts index 95e7a1a7d..2ce75e2f7 100644 --- a/packages/binding-parser/src/types/binding-parser.ts +++ b/packages/binding-parser/src/types/binding-parser.ts @@ -136,14 +136,15 @@ export interface Template { spaces: WhiteSpaces[]; } +export interface ParseResultErrors { + lexer: LexerError[]; + parse: ParseError[]; +} export interface ParseResult { cst: CstNode; ast: Template; tokens: IToken[]; - errors: { - lexer: LexerError[]; - parse: ParseError[]; - }; + errors: ParseResultErrors; } export interface CreateToken { diff --git a/packages/binding-parser/src/utils/expression.ts b/packages/binding-parser/src/utils/expression.ts new file mode 100644 index 000000000..20313dd71 --- /dev/null +++ b/packages/binding-parser/src/utils/expression.ts @@ -0,0 +1,148 @@ +import type { + ParseResultErrors, + StructureValue, +} from "../types/binding-parser"; +import { isAfterAdjacentRange, isBeforeAdjacentRange } from "./position"; + +/** + * Syntax of a binding expression can be represented by `{=expression}` or `{:=expression}` + * If an input text starts with either `{=` or `{:=`, input text is considered as binding expression + */ +export const isBindingExpression = (input: string): boolean => { + input = input.trim(); + return /^{(=|:=)/.test(input); +}; + +/** + * Check model + * + * It is considered as model when it starts with `>` or its HTML equivalent after first key without any quotes e.g oData> or oData>/... + */ +export const isModel = ( + binding: StructureValue, + errors?: ParseResultErrors +): boolean => { + if (!errors) { + return false; + } + const modelSign = errors.lexer.find( + (i) => + i.type === "special-chars" && + (i.text.startsWith(">") || i.text.startsWith(">")) + ); + if (!modelSign) { + return false; + } + // check model should appears after first key without quotes + if ( + binding.elements[0]?.key?.originalText === binding.elements[0]?.key?.text && + isBeforeAdjacentRange(binding.elements[0]?.key?.range, modelSign.range) + ) { + return true; + } + return false; +}; + +/** + * Check metadata path + * + * It is considered metadata path when it is `/` or its HTML equivalent as separator and + * + * a. is before first key e.g /key + * + * b. is after first key e.g. key/ + */ +export const isMetadataPath = ( + binding: StructureValue, + errors?: ParseResultErrors +): boolean => { + if (!errors) { + return false; + } + const metadataSeparator = errors.lexer.find( + (i) => + i.type === "special-chars" && + (i.text.startsWith("/") || + i.text.startsWith("/") || + i.text.startsWith("/")) + ); + if (!metadataSeparator) { + return false; + } + // check metadata separator is before first key e.g /key + if ( + binding.elements[0]?.key?.range.start && + isBeforeAdjacentRange( + metadataSeparator.range, + binding.elements[0].key.range + ) + ) { + return true; + } + // check metadata separator is after first key e.g. key/ + if ( + isAfterAdjacentRange( + metadataSeparator.range, + binding.elements[0]?.key?.range + ) + ) { + return true; + } + return false; +}; + +/** + * An input is considered property binding syntax when + * + * a. is empty curly bracket e.g `{}` or `{ }` + * + * b. has starting or closing curly bracket and key property with colon e.g `{anyKey: }` or `{"anyKey":}` or `{'anyKey':}` + * + * c. empty string [for initial code completion snippet] + * + * d. is not model e.g {i18n>...} or {oData>...} + * + * e. is not OData path e.g {/path/to/...} or {path/to/...} + */ +export const isPropertyBindingInfo = ( + input: string, + binding?: StructureValue, + errors?: ParseResultErrors +): boolean => { + // check empty string + if (input.trim().length === 0) { + return true; + } + + if (!binding) { + return false; + } + + // check if model + if (isModel(binding, errors)) { + return false; + } + + // check if metadata path + if (isMetadataPath(binding, errors)) { + return false; + } + + if ( + binding.leftCurly && + binding.leftCurly.text && + binding.elements.length === 0 + ) { + // check empty curly brackets + return true; + } + // check it has at least one key with colon + const result = binding.elements.find( + /* istanbul ignore next */ + (item) => item.key?.text && item.colon?.text + ); + if (result && binding.leftCurly && binding.leftCurly.text) { + return true; + } + return false; +}; diff --git a/packages/binding-parser/src/utils/index.ts b/packages/binding-parser/src/utils/index.ts new file mode 100644 index 000000000..3facc993d --- /dev/null +++ b/packages/binding-parser/src/utils/index.ts @@ -0,0 +1,14 @@ +export { + isAfterAdjacentRange, + isBefore, + isBeforeAdjacentRange, + positionContained, + rangeContained, +} from "./position"; + +export { + isBindingExpression, + isMetadataPath, + isModel, + isPropertyBindingInfo, +} from "./expression"; diff --git a/packages/binding-parser/test/unit/utils/expression.test.ts b/packages/binding-parser/test/unit/utils/expression.test.ts new file mode 100644 index 000000000..b89d31ebe --- /dev/null +++ b/packages/binding-parser/test/unit/utils/expression.test.ts @@ -0,0 +1,172 @@ +import { parseBinding } from "../../../src/parser"; +import { + isBindingExpression, + isMetadataPath, + isModel, + isPropertyBindingInfo, +} from "../../../src/utils"; + +describe("expression", () => { + describe("isBindingExpression", () => { + it("check binding expression {=", () => { + const result = isBindingExpression("{="); + expect(result).toBeTrue(); + }); + it("check binding expression {:=", () => { + const result = isBindingExpression("{:="); + expect(result).toBeTrue(); + }); + it("check other false cases", () => { + const result = isBindingExpression("{"); + expect(result).toBeFalse(); + }); + }); + describe("isPropertyBindingInfo", () => { + it("empty string", () => { + const input = " "; + const { ast, errors } = parseBinding(input); + const result = isPropertyBindingInfo(input, ast.bindings[0], errors); + expect(result).toBeTrue(); + }); + it("string value", () => { + const input = "40"; + const { ast, errors } = parseBinding(input); + const result = isPropertyBindingInfo(input, ast.bindings[0], errors); + expect(result).toBeFalse(); + }); + it("empty curly bracket without space", () => { + const input = "{}"; + const { ast, errors } = parseBinding(input); + const result = isPropertyBindingInfo(input, ast.bindings[0], errors); + expect(result).toBeTrue(); + }); + it("empty curly bracket with space", () => { + const input = "{ }"; + const { ast, errors } = parseBinding(input); + const result = isPropertyBindingInfo(input, ast.bindings[0], errors); + expect(result).toBeTrue(); + }); + it("key with colone [true]", () => { + const input = ' {path: "some/path"}'; + const { ast, errors } = parseBinding(input); + const result = isPropertyBindingInfo(input, ast.bindings[0], errors); + expect(result).toBeTrue(); + }); + it("key with colone any where [true]", () => { + const input = ' {path "some/path", thisKey: {}}'; + const { ast, errors } = parseBinding(input); + const result = isPropertyBindingInfo(input, ast.bindings[0], errors); + expect(result).toBeTrue(); + }); + it("missing colon [false]", () => { + const input = '{path "some/path"}'; + const { ast, errors } = parseBinding(input); + const result = isPropertyBindingInfo(input, ast.bindings[0], errors); + expect(result).toBeFalse(); + }); + it("contains > after first key [false]", () => { + const input = "{i18n>myTestModel}"; + const { ast, errors } = parseBinding(input); + const result = isPropertyBindingInfo(input, ast.bindings[0], errors); + expect(result).toBeFalse(); + }); + it("contains / before first key [false]", () => { + const input = "{/oData/path/to/some/dynamic/value}"; + const { ast, errors } = parseBinding(input); + const result = isPropertyBindingInfo(input, ast.bindings[0], errors); + expect(result).toBeFalse(); + }); + it("contains / after first key [false]", () => { + const input = "{/oData/path/to/some/dynamic/value}"; + const { ast, errors } = parseBinding(input); + const result = isPropertyBindingInfo(input, ast.bindings[0], errors); + expect(result).toBeFalse(); + }); + }); + + describe("isModel", () => { + it("return false if errors is undefined", () => { + const input = "{path: 'acceptable'}"; + const { ast } = parseBinding(input); + expect(isModel(ast.bindings[0])).toBe(false); + }); + + it("return true if model sign appears after first key", () => { + const input = "{oData>/path/to/a/value}"; + const { ast, errors } = parseBinding(input); + expect(isModel(ast.bindings[0], errors)).toBe(true); + }); + it("return true if model sign as HTML equivalent appears after first key", () => { + const input = "{oData>/path/to/a/value}"; + const { ast, errors } = parseBinding(input); + expect(isModel(ast.bindings[0], errors)).toBe(true); + }); + + it("return false if model sign does not appear after first key", () => { + const input = "{i18n >}"; // space is not allowed + const { ast, errors } = parseBinding(input); + expect(isModel(ast.bindings[0], errors)).toBe(false); + }); + + it("return false if model sign is not found", () => { + const input = "{path: 'acceptable'}"; + const { ast, errors } = parseBinding(input); + expect(isModel(ast.bindings[0], errors)).toBe(false); + }); + it("return false if key is with single quote", () => { + const input = "{'path'>: ''}"; + const { ast, errors } = parseBinding(input); + expect(isModel(ast.bindings[0], errors)).toBe(false); + }); + it("return false if key is with double quotes", () => { + const input = '{"path">: ""}'; + const { ast, errors } = parseBinding(input); + expect(isModel(ast.bindings[0], errors)).toBe(false); + }); + it("return false if key is with single quote [HTML equivalent]", () => { + const input = "{'path'>: ''}"; + const { ast, errors } = parseBinding(input); + expect(isModel(ast.bindings[0], errors)).toBe(false); + }); + it("return false if key is with double quotes [HTML equivalent]", () => { + const input = "{"path">: ''}"; + const { ast, errors } = parseBinding(input); + expect(isModel(ast.bindings[0], errors)).toBe(false); + }); + }); + describe("isMetadataPath", () => { + it("return false if errors is undefined", () => { + const input = "{/path/to/a/value}'}"; + const { ast } = parseBinding(input); + expect(isMetadataPath(ast.bindings[0])).toBe(false); + }); + it("return false if there is no metadata separator", () => { + const input = "{path: 'acceptable'}"; + const { ast, errors } = parseBinding(input); + expect(isMetadataPath(ast.bindings[0], errors)).toBe(false); + }); + + it("return true if the metadata separator is before adjacent first key", () => { + const input = "{/path/to/a/value}"; + const { ast, errors } = parseBinding(input); + expect(isMetadataPath(ast.bindings[0], errors)).toBe(true); + }); + + it("return true if the metadata separator is after adjacent first key", () => { + const input = "{path/to/a/value}"; + const { ast, errors } = parseBinding(input); + expect(isMetadataPath(ast.bindings[0], errors)).toBe(true); + }); + it("return true if the metadata separator as HTML equivalent is before adjacent first key", () => { + const input = "{/path/to/a/value}"; + const { ast, errors } = parseBinding(input); + expect(isMetadataPath(ast.bindings[0], errors)).toBe(true); + }); + + it("return true if the metadata separator as HTML equivalent is after adjacent first key", () => { + const input = "{path/to/a/value}"; + const { ast, errors } = parseBinding(input); + expect(isMetadataPath(ast.bindings[0], errors)).toBe(true); + }); + }); +}); diff --git a/packages/binding/src/services/completion/providers/property-binding-info.ts b/packages/binding/src/services/completion/providers/property-binding-info.ts index 2441badee..6056783ba 100644 --- a/packages/binding/src/services/completion/providers/property-binding-info.ts +++ b/packages/binding/src/services/completion/providers/property-binding-info.ts @@ -2,6 +2,8 @@ import { parseBinding, BindingParserTypes as BindingTypes, rangeContained, + isPropertyBindingInfo, + isBindingExpression, } from "@ui5-language-assistant/binding-parser"; import type { Position } from "vscode-languageserver-types"; import { AttributeValueCompletionOptions } from "@xml-tools/content-assist"; @@ -10,12 +12,7 @@ import { getUI5PropertyByXMLAttributeKey } from "@ui5-language-assistant/logic-u import { BindContext } from "../../../types"; import { createInitialSnippet } from "./create-initial-snippet"; -import { - extractBindingExpression, - getCursorContext, - isBindingExpression, - isPropertyBindingInfo, -} from "../../../utils"; +import { extractBindingExpression, getCursorContext } from "../../../utils"; import { createAllSupportedElements } from "./create-all-supported-elements"; import { createKeyProperties } from "./create-key-properties"; import { createValue } from "./create-value"; @@ -80,7 +77,7 @@ export function propertyBindingInfoSuggestions({ character: (value?.startColumn ?? 0) + startIndex, line: value?.startLine ? value.startLine - 1 : 0, // zero based index }; - const { ast } = parseBinding(expression, position); + const { ast, errors } = parseBinding(expression, position); const input = expression; if (input.trim() === "") { completionItems.push(...createInitialSnippet()); @@ -96,7 +93,7 @@ export function propertyBindingInfoSuggestions({ if (!binding) { continue; } - if (!isPropertyBindingInfo(text, binding)) { + if (!isPropertyBindingInfo(text, binding, errors)) { continue; } completionItems.push(...getCompletionItems(context, binding, ast.spaces)); diff --git a/packages/binding/src/services/diagnostics/validators/property-binding-info-validator.ts b/packages/binding/src/services/diagnostics/validators/property-binding-info-validator.ts index 264e37148..36b9718cf 100644 --- a/packages/binding/src/services/diagnostics/validators/property-binding-info-validator.ts +++ b/packages/binding/src/services/diagnostics/validators/property-binding-info-validator.ts @@ -3,14 +3,13 @@ import { Context } from "@ui5-language-assistant/context"; import { getUI5PropertyByXMLAttributeKey } from "@ui5-language-assistant/logic-utils"; import { BindingIssue } from "../../../types"; import { BINDING_ISSUE_TYPE } from "../../../constant"; +import { extractBindingExpression, getLogger } from "../../../utils"; +import { Position } from "vscode-languageserver-types"; import { - extractBindingExpression, - getLogger, - isBindingExpression, + parseBinding, isPropertyBindingInfo, -} from "../../../utils"; -import { Position } from "vscode-languageserver-types"; -import { parseBinding } from "@ui5-language-assistant/binding-parser"; + isBindingExpression, +} from "@ui5-language-assistant/binding-parser"; import { checkAst } from "./issue-collector"; import { filterLexerError, filterParseError } from "../../../utils/expression"; @@ -49,9 +48,10 @@ export function validatePropertyBindingInfo( }; const { ast, errors } = parseBinding(expression, position); for (const binding of ast.bindings) { - if (!isPropertyBindingInfo(expression, binding)) { + if (!isPropertyBindingInfo(expression, binding, errors)) { continue; } + issues.push(...checkAst(context, binding, errors)); /** diff --git a/packages/binding/src/utils/cursor.ts b/packages/binding/src/utils/cursor.ts index a0793f8b4..682a735c7 100644 --- a/packages/binding/src/utils/cursor.ts +++ b/packages/binding/src/utils/cursor.ts @@ -24,7 +24,7 @@ export const getCursorContext = ( const el = elements.find((item) => positionContained(item.range, position)); if (el) { // check key - if (positionContained(el.key?.range, position)) { + if (el.key && positionContained(el.key.range, position)) { return { type: "key", kind: "properties-excluding-duplicate", @@ -32,7 +32,7 @@ export const getCursorContext = ( }; } // check colon => value - if (positionContained(el.colon?.range, position)) { + if (el.colon && positionContained(el.colon.range, position)) { return { type: "value", kind: "value", @@ -40,7 +40,7 @@ export const getCursorContext = ( }; } // check value - if (positionContained(el.value?.range, position)) { + if (el.value && positionContained(el.value.range, position)) { return { type: "value", kind: "value", @@ -56,7 +56,7 @@ export const getCursorContext = ( // further check parts of adjacent element for (const el of elements) { // after adjacent key => value - if (isAfterAdjacentRange(spaceEl.range, el.key?.range)) { + if (el.key && isAfterAdjacentRange(spaceEl.range, el.key.range)) { // this happen when colon is missing return { type: "value", @@ -65,7 +65,7 @@ export const getCursorContext = ( }; } // after adjacent colon => value - if (isAfterAdjacentRange(spaceEl.range, el.colon?.range)) { + if (el.colon && isAfterAdjacentRange(spaceEl.range, el.colon.range)) { return { type: "value", kind: "value", diff --git a/packages/binding/src/utils/expression.ts b/packages/binding/src/utils/expression.ts index 5be29bf93..0858bb8a8 100644 --- a/packages/binding/src/utils/expression.ts +++ b/packages/binding/src/utils/expression.ts @@ -4,15 +4,6 @@ import { } from "@ui5-language-assistant/binding-parser"; import { ExtractBindingExpression } from "..//types"; -/** - * Syntax of a binding expression can be represented by `{=expression}` or `{:=expression}` - * If an input text starts with either `{=` or `{:=`, input text is considered as binding expression - */ -export const isBindingExpression = (input: string): boolean => { - input = input.trim(); - return /^{(=|:=)/.test(input); -}; - export const filterLexerError = ( binding: BindingTypes.StructureValue, errors: { @@ -45,47 +36,6 @@ export const filterParseError = ( return result; }; -/** - * An input is considered property binding syntax when - * - * a. is empty curly bracket e.g `{}` or `{ }` - * - * b. has starting and closing curly bracket and key property with colon e.g `{anyKey: }` or `{"anyKey":}` or `{'anyKey':}` - * - * c. empty string [for initial code completion snippet] - */ -export const isPropertyBindingInfo = ( - input: string, - binding?: BindingTypes.StructureValue -): boolean => { - // check empty string - if (input.trim().length === 0) { - return true; - } - - if (!binding) { - return false; - } - - // check empty curly brackets - if ( - binding.leftCurly && - binding.leftCurly.text && - binding.elements.length === 0 - ) { - return true; - } - // check it has at least one key with colon - const result = binding.elements.find( - /* istanbul ignore next */ - (item) => item.key?.text && item.colon?.text - ); - if (result && binding.leftCurly && binding.leftCurly.text) { - return true; - } - return false; -}; - /** * Regular expression to extract binding syntax. * diff --git a/packages/binding/src/utils/index.ts b/packages/binding/src/utils/index.ts index f33c6c163..338127782 100644 --- a/packages/binding/src/utils/index.ts +++ b/packages/binding/src/utils/index.ts @@ -1,8 +1,4 @@ -export { - isBindingExpression, - extractBindingExpression, - isPropertyBindingInfo, -} from "./expression"; +export { extractBindingExpression } from "./expression"; export { typesToValue, diff --git a/packages/binding/test/unit/services/diagnostics/validators/property-binding-info-validator.test.ts b/packages/binding/test/unit/services/diagnostics/validators/property-binding-info-validator.test.ts index 0e98fd64b..be90c129e 100644 --- a/packages/binding/test/unit/services/diagnostics/validators/property-binding-info-validator.test.ts +++ b/packages/binding/test/unit/services/diagnostics/validators/property-binding-info-validator.test.ts @@ -75,6 +75,12 @@ describe("property-binding-info-validator", () => { const result = await validateView(snippet); expect(result.map((item) => issueToSnapshot(item))).toStrictEqual([]); }); + it("do not check metadata binding with contains colon", async () => { + const snippet = ` +