Repository: denoland/deno
Pull request: fix(unstable): don't error on non-existing attrs or type attr

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: <number>
```

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.

Changed files:
filename: cli/js/40_lint.js
patch: @@ -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);

filename: cli/tools/lint/ast_buffer/buffer.rs
patch: @@ -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
   }

filename: cli/tools/lint/ast_buffer/ts_estree.rs
patch: @@ -200,8 +200,8 @@ impl From<AstNode> 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",

filename: tests/unit/lint_plugin_test.ts
patch: @@ -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);",