Skip to content

Commit

Permalink
fix(unstable): don't error on non-existing attrs or type attr (#27456)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
marvinhagemeister authored Dec 27, 2024
1 parent c45d0da commit fdd0edf
Show file tree
Hide file tree
Showing 4 changed files with 65 additions and 16 deletions.
41 changes: 33 additions & 8 deletions cli/js/40_lint.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;
}

/**
Expand Down Expand Up @@ -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++];
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -736,7 +761,7 @@ function createAstContext(buf) {
strByType,
typeByStr,
propByStr,
matcher: new MatchCtx(buf, strTable),
matcher: new MatchCtx(buf, strTable, strByType),
};

setNodeGetters(ctx);
Expand Down
13 changes: 7 additions & 6 deletions cli/tools/lint/ast_buffer/buffer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
}
Expand Down
5 changes: 3 additions & 2 deletions cli/tools/lint/ast_buffer/ts_estree.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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",
Expand Down
22 changes: 22 additions & 0 deletions tests/unit/lint_plugin_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);",
Expand Down

0 comments on commit fdd0edf

Please sign in to comment.