From fdd0edf23cd1edc5fbfab8e699dca39d54c18e22 Mon Sep 17 00:00:00 2001 From: Marvin Hagemeister Date: Fri, 27 Dec 2024 22:46:29 +0100 Subject: [PATCH] fix(unstable): don't error on non-existing attrs or type attr (#27456) When running selectors for JS linting plugins we would error when encountering an unknown attribute name: ```js // selector Foo[non-existant] // error Error: Missing string id: ``` This was caused by using `0` as the invalid marker, but also overloading `0` with an actual node type. So the fix is to reserve `0` as the invalid marker and move the property type to the next index. --- cli/js/40_lint.js | 41 +++++++++++++++++++++----- cli/tools/lint/ast_buffer/buffer.rs | 13 ++++---- cli/tools/lint/ast_buffer/ts_estree.rs | 5 ++-- tests/unit/lint_plugin_test.ts | 22 ++++++++++++++ 4 files changed, 65 insertions(+), 16 deletions(-) diff --git a/cli/js/40_lint.js b/cli/js/40_lint.js index 30b1f868840cf6..d29dc3e8508ddd 100644 --- a/cli/js/40_lint.js +++ b/cli/js/40_lint.js @@ -15,10 +15,10 @@ const { // Keep in sync with Rust // These types are expected to be present on every node. Note that this // isn't set in stone. We could revise this at a future point. -const AST_PROP_TYPE = 0; -const AST_PROP_PARENT = 1; -const AST_PROP_RANGE = 2; -const AST_PROP_LENGTH = 3; +const AST_PROP_TYPE = 1; +const AST_PROP_PARENT = 2; +const AST_PROP_RANGE = 3; +const AST_PROP_LENGTH = 4; // Keep in sync with Rust // Each node property is tagged with this enum to denote @@ -421,10 +421,12 @@ class MatchCtx { /** * @param {AstContext["buf"]} buf * @param {AstContext["strTable"]} strTable + * @param {AstContext["strByType"]} strByType */ - constructor(buf, strTable) { + constructor(buf, strTable, strByType) { this.buf = buf; this.strTable = strTable; + this.strByType = strByType; } /** @@ -452,7 +454,19 @@ class MatchCtx { getAttrPathValue(offset, propIds, idx) { const { buf } = this; - offset = findPropOffset(buf, offset, propIds[idx]); + const propId = propIds[idx]; + + switch (propId) { + case AST_PROP_TYPE: { + const type = this.getType(offset); + return getString(this.strTable, this.strByType[type]); + } + case AST_PROP_PARENT: + case AST_PROP_RANGE: + throw new Error(`Not supported`); + } + + offset = findPropOffset(buf, offset, propId); if (offset === -1) return undefined; const _prop = buf[offset++]; const kind = buf[offset++]; @@ -499,7 +513,18 @@ class MatchCtx { hasAttrPath(offset, propIds, idx) { const { buf } = this; - offset = findPropOffset(buf, offset, propIds[idx]); + const propId = propIds[idx]; + // If propId is 0 then the property doesn't exist in the AST + if (propId === 0) return false; + + switch (propId) { + case AST_PROP_TYPE: + case AST_PROP_PARENT: + case AST_PROP_RANGE: + return true; + } + + offset = findPropOffset(buf, offset, propId); if (offset === -1) return false; if (idx === propIds.length - 1) return true; @@ -736,7 +761,7 @@ function createAstContext(buf) { strByType, typeByStr, propByStr, - matcher: new MatchCtx(buf, strTable), + matcher: new MatchCtx(buf, strTable, strByType), }; setNodeGetters(ctx); diff --git a/cli/tools/lint/ast_buffer/buffer.rs b/cli/tools/lint/ast_buffer/buffer.rs index f37041eff20b1d..d162ee3de18718 100644 --- a/cli/tools/lint/ast_buffer/buffer.rs +++ b/cli/tools/lint/ast_buffer/buffer.rs @@ -204,11 +204,11 @@ impl SerializeCtx { prop_map: vec![0; prop_size + 1], }; - ctx.str_table.insert(""); + let empty_str = ctx.str_table.insert(""); // Placeholder node is always 0 ctx.append_node(0, NodeRef(0), &DUMMY_SP, 0); - ctx.kind_map[0] = 0; + ctx.kind_map[0] = empty_str; ctx.start_buf = NodeRef(ctx.buf.len()); // Insert default props that are always present @@ -218,10 +218,11 @@ impl SerializeCtx { let length_str = ctx.str_table.insert("length"); // These values are expected to be in this order on the JS side - ctx.prop_map[0] = type_str; - ctx.prop_map[1] = parent_str; - ctx.prop_map[2] = range_str; - ctx.prop_map[3] = length_str; + ctx.prop_map[0] = empty_str; + ctx.prop_map[1] = type_str; + ctx.prop_map[2] = parent_str; + ctx.prop_map[3] = range_str; + ctx.prop_map[4] = length_str; ctx } diff --git a/cli/tools/lint/ast_buffer/ts_estree.rs b/cli/tools/lint/ast_buffer/ts_estree.rs index 599499aa8d92a2..64dbd82cde256c 100644 --- a/cli/tools/lint/ast_buffer/ts_estree.rs +++ b/cli/tools/lint/ast_buffer/ts_estree.rs @@ -200,8 +200,8 @@ impl From for u8 { #[derive(Debug, Clone)] pub enum AstProp { - // Base, these three must be in sync with JS. The - // order here for these 3 fields is important. + // Base, these must be in sync with JS in the same order. + Invalid, Type, Parent, Range, @@ -318,6 +318,7 @@ pub enum AstProp { impl Display for AstProp { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { let s = match self { + AstProp::Invalid => "__invalid__", // unused AstProp::Parent => "parent", AstProp::Range => "range", AstProp::Type => "type", diff --git a/tests/unit/lint_plugin_test.ts b/tests/unit/lint_plugin_test.ts index 9506c3e0a8fad3..38a7e1b091279b 100644 --- a/tests/unit/lint_plugin_test.ts +++ b/tests/unit/lint_plugin_test.ts @@ -212,6 +212,28 @@ Deno.test("Plugin - visitor attr", () => { assertEquals(result[0].node.name, "foo"); }); +Deno.test("Plugin - visitor attr to check type", () => { + let result = testVisit( + "foo", + "Identifier[type]", + ); + assertEquals(result[0].node.type, "Identifier"); + + result = testVisit( + "foo", + "Identifier[type='Identifier']", + ); + assertEquals(result[0].node.type, "Identifier"); +}); + +Deno.test("Plugin - visitor attr non-existing", () => { + const result = testVisit( + "foo", + "[non-existing]", + ); + assertEquals(result, []); +}); + Deno.test("Plugin - visitor attr length special case", () => { let result = testVisit( "foo(1); foo(1, 2);",