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

Added support for the experimental pipe operators #167

Merged
merged 1 commit into from
Dec 17, 2024

Conversation

aftix
Copy link
Contributor

@aftix aftix commented Dec 13, 2024

Summary & Motivation

Adds support for the pipe operator as described in NixOS/rfcs#148 . It is available with on Nix 2.24 with the pipe-operators experimental feature and on Lix with the pipe-operator experimental feature. The operator precedence is between function application and attribute selection.

Backwards-incompatible changes

This should not effect parsing of any input text that already parsed without error before this change: it only adds more valid input.

Further context

This library is used downstream in places like statix and alejandra. Currently, using pipe operator syntax in your nix project makes any tools downstream from this library completely unusable. Issues oppiliappan/statix#88 and kamadorueda/alejandra#436 are blocked on this library not supporting the operators.

Since this doesn't change parsing of nix code that doesn't use the pipe operator, it would be a much better user experience to have the downstream tools error with "experimental feature pipe-operator not enabled" or something when the feature is off (if they wish to) than to have the tools plain not work with any codebase using pipe operators.

@oberblastmeister
Copy link
Contributor

Hi, thanks for the pr! Could you rewrite the tests as snapshot tests and add them to the test_data directory? Thanks

src/parser.rs Outdated
@@ -583,8 +583,11 @@ where

checkpoint
}
fn parse_pipe(&mut self) -> Checkpoint {
self.parse_left_assoc(Self::parse_simple, T!["|>"] | T!["<|"])
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't these have different associativity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right - it wasn't explicitly mentioned in the RFC but using nix repl i see that <| is right associative

@aftix aftix force-pushed the push-pwrnxztoylos branch from 020fe8d to 414209c Compare December 16, 2024 23:49
@aftix
Copy link
Contributor Author

aftix commented Dec 16, 2024

Hi, thanks for the pr! Could you rewrite the tests as snapshot tests and add them to the test_data directory? Thanks

I've moved the tests to test_data with tests for associativity / the correct precedence between |> and <|

@oberblastmeister
Copy link
Contributor

I think the precedence is wrong https://nix.dev/manual/nix/2.25/language/operators#pipe-operators. It should be the operator with the lowest binding power. You can replace parse_math with a new function like parse_pipe. parse_math is the outermost parsing function and thus has the lowest binding power. Then can you add more tests combining pipes with other operators like +?

@aftix aftix force-pushed the push-pwrnxztoylos branch from 414209c to 4918f37 Compare December 17, 2024 00:20
@aftix
Copy link
Contributor Author

aftix commented Dec 17, 2024

yeah you're right, i've changed the precedence and added tests for the precedence in relation to other operators

@oberblastmeister
Copy link
Contributor

looks good to me

@oberblastmeister oberblastmeister merged commit 9001e02 into nix-community:master Dec 17, 2024
3 checks passed
@mightyiam
Copy link

Thank you!

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