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

public API overhaul #647

Merged
merged 5 commits into from
Nov 10, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/weak-turkeys-lie.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@nomicfoundation/slang": minor
---

Require specifying an initial offset when creating a CST cursor.
4 changes: 2 additions & 2 deletions crates/codegen/parser/runtime/src/cst.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,8 @@ impl Node {
}
}

pub fn cursor(&self) -> Cursor {
Cursor::new(self.clone())
pub fn create_cursor(&self, text_offset: TextIndex) -> Cursor {
Cursor::new(self.clone(), text_offset)
}

pub fn as_rule(&self) -> Option<&Rc<RuleNode>> {
Expand Down
4 changes: 2 additions & 2 deletions crates/codegen/parser/runtime/src/cursor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,13 +87,13 @@ impl Iterator for Cursor {
}

impl Cursor {
pub(crate) fn new(node: Node) -> Self {
pub(crate) fn new(node: Node, text_offset: TextIndex) -> Self {
Self {
path: vec![],
current: PathNode {
node,
child_number: 0,
text_offset: Default::default(),
text_offset,
},
is_completed: false,
}
Expand Down
18 changes: 11 additions & 7 deletions crates/codegen/parser/runtime/src/napi/napi_cst.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ impl RuleNode {
(&self.0.text_len).into()
}

#[napi(getter, ts_return_type = "Array<cst.RuleNode | cst.TokenNode>")]
#[napi(ts_return_type = "Array<cst.RuleNode | cst.TokenNode>")]
pub fn children(&self, env: Env) -> Vec<JsObject> {
self.0
.children
Expand All @@ -51,9 +51,11 @@ impl RuleNode {
.collect()
}

#[napi(getter, ts_return_type = "cursor.Cursor")]
pub fn cursor(&self) -> Cursor {
Cursor::new(RustNode::Rule(self.0.clone()).cursor())
#[napi(ts_return_type = "cursor.Cursor")]
pub fn create_cursor(&self, text_offset: TextIndex) -> Cursor {
RustNode::Rule(self.0.clone())
.create_cursor((&text_offset).into())
.into()
}
}

Expand Down Expand Up @@ -83,9 +85,11 @@ impl TokenNode {
self.0.text.clone()
}

#[napi(getter, ts_return_type = "cursor.Cursor")]
pub fn cursor(&self) -> Cursor {
Cursor::new(RustNode::Token(self.0.clone()).cursor())
#[napi(ts_return_type = "cursor.Cursor")]
pub fn create_cursor(&self, text_offset: TextIndex) -> Cursor {
RustNode::Token(self.0.clone())
.create_cursor((&text_offset).into())
.into()
}
}

Expand Down
10 changes: 8 additions & 2 deletions crates/codegen/parser/runtime/src/napi/napi_cursor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,12 @@ use napi_text_index::*;
#[napi(namespace = "cursor")]
pub struct Cursor(Box<RustCursor>);

impl From<RustCursor> for Cursor {
fn from(value: RustCursor) -> Self {
Self(value.into())
}
}

#[napi(namespace = "cursor")]
impl Cursor {
pub(crate) fn new(cursor: RustCursor) -> Self {
Expand Down Expand Up @@ -41,7 +47,7 @@ impl Cursor {
self.0.is_completed()
}

#[napi(getter, ts_return_type = "cst.RuleNode | cst.TokenNode")]
#[napi(ts_return_type = "cst.RuleNode | cst.TokenNode")]
pub fn node(&self, env: Env) -> JsObject {
self.0.node().to_js(&env)
}
Expand All @@ -56,7 +62,7 @@ impl Cursor {
(&self.0.text_range()).into()
}

#[napi(getter, ts_return_type = "Array<cst.RuleNode>")]
#[napi(ts_return_type = "Array<cst.RuleNode>")]
pub fn path_rule_nodes(&self, env: Env) -> Vec<JsObject> {
self.0
.path_rule_nodes()
Expand Down
4 changes: 2 additions & 2 deletions crates/codegen/parser/runtime/src/napi/napi_parse_error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ impl ParseError {
}

#[napi(namespace = "parse_error")]
pub fn to_error_report(&self, source_id: String, source: String, with_colour: bool) -> String {
self.0.to_error_report(&source_id, &source, with_colour)
pub fn to_error_report(&self, source_id: String, source: String, with_color: bool) -> String {
self.0.to_error_report(&source_id, &source, with_color)
}
}
14 changes: 10 additions & 4 deletions crates/codegen/parser/runtime/src/napi/napi_parse_output.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,12 @@ impl From<RustParseOutput> for ParseOutput {

#[napi(namespace = "parse_output")]
impl ParseOutput {
#[napi(getter, ts_return_type = "cst.RuleNode | cst.TokenNode")]
pub fn parse_tree(&self, env: Env) -> napi::JsObject {
return self.0.parse_tree().to_js(&env);
#[napi(ts_return_type = "cst.RuleNode | cst.TokenNode")]
pub fn tree(&self, env: Env) -> napi::JsObject {
return self.0.tree().to_js(&env);
}

#[napi(getter, ts_return_type = "Array<parse_error.ParseError>")]
#[napi(ts_return_type = "Array<parse_error.ParseError>")]
pub fn errors(&self) -> Vec<napi_parse_error::ParseError> {
self.0.errors().iter().map(|x| x.clone().into()).collect()
}
Expand All @@ -28,4 +28,10 @@ impl ParseOutput {
pub fn is_valid(&self) -> bool {
self.0.is_valid()
}

/// Creates a cursor that starts at the root of the parse tree.
#[napi(ts_return_type = "cursor.Cursor")]
pub fn create_tree_cursor(&self) -> napi_cursor::Cursor {
return self.0.create_tree_cursor().into();
}
}
10 changes: 10 additions & 0 deletions crates/codegen/parser/runtime/src/napi/napi_text_index.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,16 @@ impl From<&RustTextIndex> for TextIndex {
}
}

impl From<&TextIndex> for RustTextIndex {
fn from(value: &TextIndex) -> Self {
Self {
utf8: value.utf8 as usize,
utf16: value.utf16 as usize,
char: value.char as usize,
}
}
}

#[napi(object, namespace = "text_index")]
#[derive(Copy, Clone)]
pub struct TextRange {
Expand Down
4 changes: 2 additions & 2 deletions crates/codegen/parser/runtime/src/parse_error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ impl ParseError {
.collect();
}

pub fn to_error_report(&self, source_id: &str, source: &str, with_colour: bool) -> String {
return render_error_report(self, source_id, source, with_colour);
pub fn to_error_report(&self, source_id: &str, source: &str, with_color: bool) -> String {
return render_error_report(self, source_id, source, with_color);
}
}

Expand Down
9 changes: 7 additions & 2 deletions crates/codegen/parser/runtime/src/parse_output.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use super::{cst, parse_error::ParseError};
use crate::{cst, cursor::Cursor, parse_error::ParseError};

#[derive(Debug, PartialEq)]
pub struct ParseOutput {
Expand All @@ -7,7 +7,7 @@ pub struct ParseOutput {
}

impl ParseOutput {
pub fn parse_tree(&self) -> cst::Node {
pub fn tree(&self) -> cst::Node {
return self.parse_tree.clone();
}

Expand All @@ -18,4 +18,9 @@ impl ParseOutput {
pub fn is_valid(&self) -> bool {
return self.errors.is_empty();
}

/// Creates a cursor that starts at the root of the parse tree.
pub fn create_tree_cursor(&self) -> Cursor {
return self.parse_tree.create_cursor(Default::default());
Copy link
Member

Choose a reason for hiding this comment

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

If exposing the API is important for us now, I would add more documentation to the public cursor functions that the user might use

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure!

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ pub fn total_not_skipped_span(result: &ParserResult) -> usize {

nodes
.iter()
.flat_map(cst::Node::cursor)
.flat_map(|node| cst::Node::create_cursor(node, Default::default()))
.filter_map(|node| match node {
cst::Node::Token(token) if token.kind != TokenKind::SKIPPED => Some(token.text.len()),
_ => None,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ where
debug_assert_eq!(
errors.len() > 0,
parse_tree
.cursor()
.create_cursor(Default::default())
.any(|x| x.as_token_with_kind(&[TokenKind::SKIPPED]).is_some())
);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ impl Match {
pub fn is_full_recursive(&self) -> bool {
self.nodes
.iter()
.flat_map(cst::Node::cursor)
.flat_map(|node| cst::Node::create_cursor(node, Default::default()))
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a fan of using the Default as parameter and think it'd be clearer if we have a separate cursor_without_offset (without the argument) function cursor_with_offset (or something along the lines).

This is fine for now, but I'd rethink how we can design the API to better convey that only the root node cursor has absolute offsets and other created ones are relative.

Copy link
Collaborator Author

@OmarTawfik OmarTawfik Nov 10, 2023

Choose a reason for hiding this comment

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

I think cursor_without_offset() should create a cursor that doesn't track offsets at all (for perf, and also to not expose an invalid .offset property).

Also I thought about introducing a TextIndex::ZERO helper.

But I think we should consider this as part of future updates to the API. I want to keep this PR minimal to address existing feedback.

.all(|node| node.as_token_with_kind(&[TokenKind::SKIPPED]).is_none())
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ impl Language {
];

pub fn new(version: Version) -> std::result::Result<Self, Error> {
if Self::SUPPORTED_VERSIONS.contains(&version) {
if Self::SUPPORTED_VERSIONS.binary_search(&version).is_ok() {
Ok(Self {
{%- for version in code.referenced_versions %}
version_is_at_least_{{ version | replace(from=".", to="_") }}: Version::new({{ version | split(pat=".") | join(sep=", ") }}) <= version,
Expand Down
4 changes: 2 additions & 2 deletions crates/solidity/outputs/cargo/crate/src/generated/cst.rs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions crates/solidity/outputs/cargo/crate/src/generated/cursor.rs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading