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

add pipe operator #152

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

add pipe operator #152

wants to merge 1 commit into from

Conversation

q60
Copy link

@q60 q60 commented Sep 14, 2024

it works but help needed, i'm in love/hate relationships with tree-sitter specifically and parsers in general

before

image

after

image

@eclairevoyant
Copy link

eclairevoyant commented Sep 16, 2024

It's a good start, but this misses support for the <| operator

Copy link
Owner

@oxalica oxalica left a comment

Choose a reason for hiding this comment

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

Thanks, I just know that Nix got new experimental operators. It may worth referencing the source PR NixOS/nix#11131

CI failure is unrelated and is already fixed on main. A rebase should fix it.

@@ -160,7 +160,7 @@ pub(crate) fn highlight(
HlTag::Operator(HlOperator::Comparison)
}
T![+] | T![-] | T![*] | T![/] => HlTag::Operator(HlOperator::Arithmetic),
T![++] | T!["//"] => HlTag::Operator(HlOperator::Aggregation),
Copy link
Owner

Choose a reason for hiding this comment

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

It's not a collection operator. It may need another HlOperator variant like Pipe.

@@ -250,7 +250,7 @@ impl<'db> InferCtx<'db> {
self.unify_var(lhs_ty, rhs_ty);
lhs_ty
}
BinaryOpKind::Concat => {
BinaryOpKind::Concat | BinaryOpKind::Pipe => {
Copy link
Owner

Choose a reason for hiding this comment

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

We shouldn't unity arguments of pipe operators to be List.
They should follow the handling of Expr::Apply with two arguments flipped.

Copy link
Author

Choose a reason for hiding this comment

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

i don't quite follow this, need an example... speaking of the other threads, i can make the changes and push them right away
that's quite dumb, but i have basically zero experience in parsers 💀
this PR (plus patches in tree-sitter-nix, rnix-parser and alejandra) emerged from my need of using pipe operator w/o errors from the language server and with working formatter. then i showed all of this to friends and they kinda expected me to make PRs to the base repos 💀 💀 💀
btw i also opened a PR in tree-sitter-nix, but considering the status quo of rnix-parser and it being a dependency of alejandra, i am totally unsure how to work this out 😭
sorry for this silly venting in this thread

@@ -113,6 +113,7 @@ def! {
LT_EQ = [<=],
MINUS_GT = [->],
NOT_EQ = [!=],
PIPE = [|>],
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
PIPE = [|>],
OR_GT = [|>],

Here we name symbols syntactically, not semantically.

@@ -884,6 +884,7 @@ impl SyntaxKind {
T![*] |
T![/] => (17, 18),
T![++] => (20, 19),
T![|>] => (21, 22),
Copy link
Owner

Choose a reason for hiding this comment

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

This precedence is incorrect. According to Nix, it should have a lower precedence than logic-imply ->. So it should goes above to be (-1, 0), which underflows u8... Unfortunately, we need to shift all other BP values up by +10 (for simplicity and more future proof), so |> becomes (9, 10), -> becomes (12, 11), and etc.

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