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

feat(unstable): add JS linting plugin infrastructure #27416

Merged
merged 23 commits into from
Dec 20, 2024

Conversation

marvinhagemeister
Copy link
Contributor

@marvinhagemeister marvinhagemeister commented Dec 18, 2024

This PR extracts the core part of #27203 to make it easier to review and land in parts.

It contains:

  • The JS plugin code the deserializes and walks the buffer
  • The Rust portion to serialize SWC to the buffer format (a bunch of nodes are still todos, but imo these can land anytime later)
  • Basic lint plugin types, without the AST node types to make this PR easier to review
  • Added more code comments to explain the format etc.

setNodeGetters(ctx);

// DEV ONLY: Enable this to inspect the buffer message
// _dump(ctx);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Btw: Uncommenting this line makes debugging the binary format easier. It prints it in a text representation.

@bartlomieju bartlomieju marked this pull request as ready for review December 20, 2024 21:20
Comment on lines +275 to +276
append_u32(&mut self.buf, span.lo.0);
append_u32(&mut self.buf, span.hi.0);
Copy link
Member

@dsherret dsherret Dec 20, 2024

Choose a reason for hiding this comment

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

I think this is off by one? Maybe prefer using SourceRange instead of Span

impl TsEsTreeBuilder {
pub fn new() -> Self {
// Max values
// TODO: Maybe there is a rust macro to grab the last enum value?
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a warning comment on the enums for now?

// Max values
// TODO: Maybe there is a rust macro to grab the last enum value?
let kind_count: u8 = AstNode::TSEnumBody.into();
let prop_count: u8 = AstProp::Value.into();
Copy link
Member

Choose a reason for hiding this comment

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

Is this off by 1 since Value is the index I think?

Copy link
Contributor Author

@marvinhagemeister marvinhagemeister Dec 20, 2024

Choose a reason for hiding this comment

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

Good point, it's currently accounted for here

kind_map: vec![0; kind_size + 1],
prop_map: vec![0; prop_size + 1],

Maybe that's confusing though and the check should be put here where the values are created instead.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, or rename to prop_max_index (though probably best to just account for it here)

@marvinhagemeister marvinhagemeister changed the title unstable(lint): add JS plugin infrastructure feat(unstable): add JS linting plugin infrastructure Dec 20, 2024
todo!();
}
Decl::TsInterface(node) => {
let pos = ctx.header(AstNode::TSInterface, parent, &node.span, 0);
Copy link
Member

Choose a reason for hiding this comment

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

Is this property count wrong? I feel like we need a way to express this in the code so that it doesn't need to be manually maintained and the property count can be automatically populated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, those property counts are a bit brittle. I'll remove them once I'm back.

Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

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

Very cool and I'm excited about this! Still a lot of work to do, but this is good enough to land for iteration.

@bartlomieju bartlomieju merged commit 26425a1 into main Dec 20, 2024
17 checks passed
@bartlomieju bartlomieju deleted the js_lint_plugin_part_1 branch December 20, 2024 23:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants