Better typed api #105

oberblastmeister · 2022-07-19T14:03:09Z

Continuation of #36

src/ast.rs

src/ast/nodes.rs

oberblastmeister · 2022-07-19T17:06:42Z

That was a lot of work but I think I finished.

oberblastmeister · 2022-07-19T17:16:45Z

I'm going to remove Value, as it is replaced by LiteralKind. The library is focused on syntax and not semantics, so Value is not appropriate here. The current Value enum is also incorrect, as a value should also contain lambdas and records which are not present here. Also, it looks like you can't construct a string value, which is weird. Instead, I will add methods on the inner types of LiteralKind to return the "even more typed" types of the literals.

oberblastmeister · 2022-07-19T17:17:30Z

@oxalica @zseri @Ma27 What are your thoughts?

examples/list-fns.rs

src/ast.rs

src/ast/expr_ext.rs

Co-authored-by: Alain Zscheile <[email protected]>

src/ast/expr_ext.rs

Co-authored-by: Alain Zscheile <[email protected]>

fogti · 2022-07-19T19:51:59Z

Tests should be ported before this should/can be merged.

oberblastmeister · 2022-07-19T19:54:33Z

The tests should be ported already, hence the massive diff

oberblastmeister · 2022-07-19T19:56:30Z

Also rowan already has a pretty printer for the syntax tree, so we can remove our TextDump type. I was going to update the tests with the rowan pretty printer but the diff got too large. I will probably do that in another pr.

- Try to make the macro syntax more rust like. - impl blocks should be written - Implementing a trait that has all defaults should be done by hand

examples/list-fns.rs

Co-authored-by: Alain Zscheile <[email protected]>

fogti · 2022-07-22T04:54:18Z

examples/list-fns.rs

@@ -1,6 +1,20 @@
 use std::{env, error::Error, fs};

-use rnix::{types::*, NodeOrToken, SyntaxKind::*, SyntaxNode};
+use rnix::ast::AstToken;


Suggested change

use rnix::ast::AstToken;

fogti · 2022-07-22T04:56:10Z

src/ast.rs

+    pub(super) fn children_tokens<N: AstNode>(parent: &N) -> impl Iterator<Item = SyntaxToken> {
+        parent.syntax().children_with_tokens().filter_map(SyntaxElement::into_token)
+    }
+
+    pub(super) fn children_tokens_u<N: AstNode>(parent: &N) -> impl Iterator<Item = SyntaxToken> {
+        parent.syntax().children_with_tokens().filter_map(SyntaxElement::into_token)
+    }


what is the intended difference between these two functions? The first one should ne ,pre typed, right? But it currently isn't?

I guess this is fixed now.

src/ast.rs

fogti · 2022-07-22T05:05:08Z

src/ast/operators.rs

+    pub fn from_kind(token: SyntaxKind) -> Option<Self> {
+        match token {
+            TOKEN_CONCAT => Some(BinOpKind::Concat),
+            TOKEN_QUESTION => Some(BinOpKind::IsSet),
+            TOKEN_UPDATE => Some(BinOpKind::Update),
+
+            TOKEN_ADD => Some(BinOpKind::Add),
+            TOKEN_SUB => Some(BinOpKind::Sub),
+            TOKEN_MUL => Some(BinOpKind::Mul),
+            TOKEN_DIV => Some(BinOpKind::Div),
+
+            TOKEN_AND => Some(BinOpKind::And),
+            TOKEN_EQUAL => Some(BinOpKind::Equal),
+            TOKEN_IMPLICATION => Some(BinOpKind::Implication),
+            TOKEN_LESS => Some(BinOpKind::Less),
+            TOKEN_LESS_OR_EQ => Some(BinOpKind::LessOrEq),
+            TOKEN_MORE => Some(BinOpKind::More),
+            TOKEN_MORE_OR_EQ => Some(BinOpKind::MoreOrEq),
+            TOKEN_NOT_EQUAL => Some(BinOpKind::NotEqual),
+            TOKEN_OR => Some(BinOpKind::Or),
+
+            _ => None,
+        }
+    }


imo this should be a core::convert::TryFrom impl

fogti · 2022-07-22T05:05:36Z

src/ast/operators.rs

+    pub fn from_kind(kind: SyntaxKind) -> Option<Self> {
+        match kind {
+            TOKEN_INVERT => Some(UnaryOpKind::Invert),
+            TOKEN_SUB => Some(UnaryOpKind::Negate),
+            _ => None,
+        }
+    }


this should also be a TryFrom impl

src/ast/str_util.rs

src/ast/nodes.rs

src/ast/tokens.rs

darichey · 2022-07-22T05:27:56Z

I think any problems with str_util.rs should be ignored in this PR, since it's just moved from the now-removed value.rs. Those can be a focus of a future PR.

fogti · 2022-07-22T05:30:37Z

ok

fogti · 2022-07-22T05:36:28Z

we do have indeed a problem with str_util.rs tho, because any changes in other PRs need to be manually ported/integrated into this one, without much assistance from git (because it doesn't recognize the file as "moved"), which is unfortunate.

examples/list-fns.rs

src/lib.rs

oberblastmeister · 2022-07-22T13:26:43Z

I wonder if we should be using TryFrom, our own trait, or just normal methods. All of our uses of TryFrom basically have type Error = () which makes it isomorphic to Option.

fogti · 2022-07-22T18:43:51Z

We could also try From<ExtType> for Option<OurType>, but it might not work due to orphan rules.

oberblastmeister · 2022-07-22T19:07:12Z

I think this is basically ready. This pr is really big and I don't want it to drag on forever. It doesn't have to be perfect, as we can make followup prs. I think currently it is good enough. I want to get this in so I can merge other prs. So @oxalica @darichey @zseri final round of comments before I merge?

Ma27 · 2022-07-23T13:21:40Z

Would you mind summarizing the changes in here, especially the breaking things? I actually wanted to review it as well, however I didn't have the time for it last week, next week perhaps.

The thing is, this may be fine implementation-wise, but to me it seems as if this change was punched through way too fast. Talking about the API changes etc. first might've been preferable.

oberblastmeister · 2022-07-23T13:37:28Z

I understand your concern, but we talked about the api a lot before merging this, hence the massive comment length. This pr was here for more than a year, and I didn't want to wait any longer, so I don't think it was too fast.

Summary

removes the types module and replaces it with ast
- the ast modules has better types and sum types
- biggest change is removes ParsedType in favor of Expr
removes the value module. methods on Value go on the individual types. For example, string related methods go on ast::Str
parse changed to Root::parse

oberblastmeister added 2 commits July 11, 2022 23:53

more types; add match_ast; literalkind

9f56e12

use rowan AstNode

61211a3

oberblastmeister mentioned this pull request Jul 19, 2022

better typed api, update rowan, remove SmolStr #36

Closed

oberblastmeister added 3 commits July 19, 2022 10:17

transition AST type to Parse type

3743c07

implement From for enum AstNodes

6706b12

start literal node in string and add comment

6590dea

oxalica suggested changes Jul 19, 2022

View reviewed changes

oberblastmeister added 6 commits July 19, 2022 11:11

distinguish between keys and attrs

ae449c1

use rowan AstChildren

acabaa3

fix macro indentation

46148ba

api improvements

57bb3f1

use Root::parse function

8a3ad43

update tests to make string a literal also

f9d4281

oberblastmeister force-pushed the typed branch from adae4bf to f9d4281 Compare July 19, 2022 16:49

separate string related stuff from value.rs

91717e2