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

make EdgeLabel required in the CST #815

Open
OmarTawfik opened this issue Feb 15, 2024 · 6 comments
Open

make EdgeLabel required in the CST #815

OmarTawfik opened this issue Feb 15, 2024 · 6 comments
Assignees

Comments

@OmarTawfik
Copy link
Contributor

OmarTawfik commented Feb 15, 2024

Previously, some nodes didn't produce labels, so Edge::label was Option<>. This is no longer the case.
Making this a required property for all edges (replacing the Option) makes it much easier to handle/match in client code.

@OmarTawfik OmarTawfik converted this from a draft issue Feb 15, 2024
@AntonyBlakey
Copy link
Contributor

The ability to have un-named nodes is critical to the query system!

@Xanewok
Copy link
Contributor

Xanewok commented Feb 20, 2024

I'm also in the camp of having the names optional. If it's all about Option<T> handling, we can surely inline the None variant directly in the FieldName but it makes sense not to name every node there is.

@OmarTawfik OmarTawfik self-assigned this Feb 20, 2024
@OmarTawfik
Copy link
Contributor Author

I really think we should make it required everywhere. From an API perspective, if i'm iterating on children, or even filtering/inspecting a specific child, I should be able to read/know its name, and it should be strongly typed, and finite/matchable. All such is provided by the FieldName enum. But using Option<FieldName> and None force me to then rely on my own knowledge of how the parser works to know what to do next.

Re: the query system: I think the fact that something has a name or not shouldn't interfere with the querying system, as they are independent, and can actually be confusing in some cases, like the separator: Comma that separates TupleDeconstructionElements with empty members. I think it should derive its contentful-ness from other existing properties accordingly.

But we also discussed the cost of adding a side channel to record such information, and it is not clear whether it is worth doing now.

@OmarTawfik
Copy link
Contributor Author

Another idea: we might not need a side channel after all, if we can derive this info from existing node kinds (example below). We would need to use #808 to see if this affects query engine performance:

fn is_node_contentful(parent_kind: &RuleKind, child_kind: &TokenKind) -> bool {
    match (parent_kind, child_kind) {
        // for `StructItem`, add children that appear in `terminated_by`/`delimited_by`:
        (RuleKind::Statement, TokenKind::Semicolon) => false,
        (RuleKind::Block, TokenKind::LeftCurly | TokenKind::RightCurly) => false,
        // .....

        // For `Separated`, add the `separator`:
        (RuleKind::Arguments, TokenKind::Comma) => false,
        (RuleKind::Parameters, TokenKind::Comma) => false,
        // .....

        // Also trivia:
        (_, TokenKind::Whitespace | TokenKind::Newline | TokenKind::Comment) => false,

        // otherwise, contentful:
        _ => true,
    }
}

@OmarTawfik
Copy link
Contributor Author

Let's defer this until we have more experience/feedback on the API.

@OmarTawfik OmarTawfik removed their assignment Feb 21, 2024
@OmarTawfik OmarTawfik moved this to Todo in Slang - Backlog Feb 21, 2024
@github-project-automation github-project-automation bot moved this to ⏳ Todo in Slang - 2024 H2 Dec 6, 2024
@OmarTawfik OmarTawfik changed the title make FieldName required in the CST make EdgeLabel required in the CST Dec 11, 2024
@OmarTawfik
Copy link
Contributor Author

@AntonyBlakey Currently we already produce labels for all (?) children nodes in the tree. The remaining work is about changing the type of the Edge::label field from Option<EdgeLabel> to EdgeLabel.
It shouldn’t impact the query engine or the parser, as users should still be able to author queries with no edge filters. But it will make it much easier to use our CST and other APIs, and eliminate a lot of redundant code. Please let me know if you have concerns on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ⏳ Todo
Development

No branches or pull requests

5 participants