nix-community / rnix-parser

A Nix parser written in Rust [maintainer=@oberblastmeister]
MIT License
366 stars 44 forks source link

better typed api, update rowan, remove `SmolStr` #36

Closed oberblastmeister closed 2 years ago

oberblastmeister commented 3 years ago

I have made the typed api better by creating more types and returning our types from functions. For example the lambda function of the Apply type now returns an Expr instead of just any SyntaxNode. This makes the functions much more useful and allows for less explicit casting. There are also new types for select tokens, such as Whitespace and Comment. ParsedType has been renamed to Expr because having every possible parsed type in an enum is not very useful. Only allowing Expr increases the 'tightness' of the type. Most of the stuff is based on rust-analyzer. I imagine these changes would make the experience of @aaronjanse better when implementing the evaluator. I also updated rowan and removed SmolStr which fixes #29 .

Ma27 commented 3 years ago

@aaronjanse would you mind taking a look? :)

lovesegfault commented 3 years ago

This also needs an update since rowan 0.15 is now available.

aaronjanse commented 3 years ago

I'm hesitant to change the API so heavily. Although the semvar of rnix-parser isn't 1.0, the API is still depended upon by many projects.

I'm still in favor of the changes. I just think we should look into what the migration process would be for existing codebases and how much they would benefit from the new design

oberblastmeister commented 3 years ago

I think @nerdypepper could provide some input, as I think he expressed some interest for a better typed api.

oberblastmeister commented 3 years ago

To migrate to then new changes would just require some changing of names and less casting. I think the breaking change is okay, as like you said we aren't at 1.0 yet, and I imagine the API wouldn't change much after this. It is better to get this api in before 1.0 rather than be stuck with a less typed one that is harder to change. It seems like about 5 projects depend on this crate, which doesn't seem like that much. For rnix-lsp and nixpkgs-fmt, I can provide prs to update them.

oppiliappan commented 3 years ago

I would love to see these changes land before 1.0. I am perfectly fine with breaking changes in rnix-parser, it is often hard to keep a stable ABI for these kind of projects.

It would be nice to get input from other rnix-parser users however.

Ma27 commented 3 years ago

I'll re-assign it to 0.11.0. I fully agree, before we have improved the error handling #34/#35 and have taken care of this, we shouldn't consider a 1.0. Also, I'd like to publish 0.10 ~nowish as it's needed to support Nix 2.4 syntax features.

I'll try take a closer look during the next weeks, then we can resume evaluating how/what to merge :)

Sorry that this takes so long, @oberblastmeister please don't forget that I highly appreciate the work you've put in here! :heart:

fogti commented 2 years ago

I think it might be a good idea to split this PR into multiple parts if possible, especially because I suppose that refining the typed API takes longer to be ready-to-merge than the "update rowan, remove SmolStr" part.

Ma27 commented 2 years ago

Pinging @oberblastmeister @zseri do you want to split this up and get it ready? :)

fogti commented 2 years ago

ok, this branch can now be rebased upon master, as the two partial PRs by me have been merged. @oberblastmeister If you want me to help with this (because it's basically just some kind of churn / "boilerplate work"), just ask, I have some large amounts of free time after the next two weeks...

oberblastmeister commented 2 years ago

@Ma27 @zseri The rowan pr looks good, but the remove SmolStr pr seems to create unnecessary Strings which will create lots of heap allocations. Should I create another pr to use string references?

fogti commented 2 years ago

I recognize stuff like https://github.com/nix-community/rnix-parser/commit/5686c73cbd954fcc33c91fe1e289b37cd740d3ea#diff-4a04259da480a6b794a2e947e4cc03eff4d1aa9330836f5b91cac68c5398193fR231-R234 as problematic, is there something else which I missed?

oberblastmeister commented 2 years ago

I don't think you missed other stuff.

fogti commented 2 years ago
fogti commented 2 years ago

it might be a good idea to squash this branch before rebasing; and rename the PR title because it would then reference stuff which wouldn't be part of it anymore...

fogti commented 2 years ago

@oberblastmeister ok, take a look at https://github.com/zseri/rnix-parser/tree/better-typed-api

oxalica commented 2 years ago

Exciting to see this PR but the code here seems quite obsolete.

rowan 0.15 now defines trait AstNode and mod support with auxiliary functions in rowan::ast. They could reduce lots of code in our side.

@zseri

ok, take a look at https://github.com/zseri/rnix-parser/tree/better-typed-api

I'd think you'd take over this PR, right? Could you please open a new PR to make review easier?

oberblastmeister commented 2 years ago

@oxalica I'm still working on this. I'll try to get it done today.

oberblastmeister commented 2 years ago

The new pull request is at #105