Gradually move `prlqc-ast` code into other crates

[max-sixty]

What's up?

As discussed with @aljazerzen & @m-span (though not necessarily agreed yet, feedback welcome), prqlc-ast isn't quite pulling its weight as a "cross-compiler external interface".

I'm very keen to continue to make the compiler more modular by stage. But this crate doesn't do that — instead it adds context to understanding a particular stage.

So I think it makes sense to:

• At least move into prqlc-parser the items which only have prqlc-parser as a dependent. For example span.rs (or token.rs, which I recently combined with similar code)
  • (technically prqlc also depends on Span — but only because it's used in prqlc-parser, I'm counting that as "only have prqlc-parser as a dependent", hope that's not too confusing...)
• Possibly do more — e.g. we have some code in generic.rs which lots of crates depend on — we could consider also dissolving that. But we can wait to see what's left

Keen not to just continually refactor the library (meme below), but in this case I do think it would make it simpler and is worthwhile.

[m-span]

I have some ideas on how to handle this, but as I understand, @aljazerzen is in the middle of a refactor. I've generally found it bad practice to have more than one refactor being worked on at the same time, as the second to be merged in will have a hell of a bad time with it. Ironically, if the organization @max-sixty proposes is in place, this wouldn't be much of an issue.

@aljazerzen what is the state of your refactor, and how far does it encompass? Can you give me an idea of what you are/are not touching so I can respect the work you've done so far?

[m-span]

Another related question:

            PRQL

   (parser) │ ▲
 prql_to_pl │ │ pl_to_prql
            │ │
            ▼ │      json::from_pl
                   ────────►
           PL AST            PL JSON
                   ◄────────
            │        json::to_pl
            │
 (resolver) │
   pl_to_rq │
            │
            │
            ▼        json::from_rq
                   ────────►
           RQ AST            RQ JSON
                   ◄────────
            │        json::to_rq
            │ 
  rq_to_sql │ (SQL compiler)
            ▼

            SQL

This diagram should be accurate, but isn't quite up to date, since PL and AST aren't exactly the same, and prql_to_pl technically outputs in prqlc-ast format. Part of the confusion is the different terminology. Should the "parser" be returning PL instead of (parser) AST? If so, should the PL objects be moved to the parser as well?

The end result might be that everything called by prql_to_pl (and possibly pl_to_prql too) exists within the "parser"

[m-span]

Right now it looks like the resolver is actually converting from AST to PL, then to RQ.

[aljazerzen]

I'm in favor of this change - a separate crate is unnecessary and tedious to use.

It will be painful to merge this into my branch, but let's not take that as an excuse to put off improvements on the master branch. In the main prqlc crate I've added use prqlc_ast as ast into the main lib.rs, so everywhere else, I (am trying to) have ast imported as use crate::ast, which would make merging this much easier.

One thing I would ask, is to do this in separate commits (or PRs, just that the master branch contains separate commits in the end).

(PS: my branch is now closer to a complete rewrite of semantic module than a refactor. To merge that, we will have to make changes to the language itself)

The diagram in the lib should be updated, I agree, but we need to rethink how we see the compiler stages.

[aljazerzen]

Also, a bit of terminology decision: is PL an Abstract Syntax Tree? is RQ an Abstract Syntax Tree?

When the prql-ast was split into separate crate, it assumed that PL and RQ are IRs, not an AST. I think that the prqlc-ast, PL and RQ should all be named ASTs, because it is nice to have a name that covers all of these representations.

If we go with this, we need to come up with a nice name for the prqlc-ast. We floated a few ideas before, we just need to pick one. I suggest one of "Parser Syntax Tree" / "Parser Tree", with abbreviations PST / PT.

---

Something that would probably help with code readability would be to use module-qualified names of AST nodes:

- use crate::ir::pl::Expr;
-
- fn print(e: Expr) -> ...

+ use crate::ir::pl;
+
+ fn print(e: pl::Expr) -> ...

This is what we do at edgedb, and it is a life saver. It also means that when you add a piece of code that needs pl::InterpolationItem, you probably don't need to add a new import, because pl is probably already imported.

[aljazerzen]

These are stages of the compiler as I see them now:

stage	sub-stage	AST	comment
parse	lexer	string -> tokens
parse	parser	tokens -> prqlc-ast PT
semantic	ast_expand	PL	bad name, this could be called "desugar"
semantic	resolver	PL	this does a lot of stuff, my rewrites splits it up
semantic	flatten	PL
semantic	lowering	PL -> RQ
sql	preprocess	RQ
sql	srq compiler	RQ -> SRQ
sql	postprocess	SRQ
sql	SQL compiler	SRQ -> sqlparser::ast
sql	codegen	sqlparser::ast -> string

[kgutwin]

Something that would probably help with code readability would be to use module-qualified names of AST nodes:

- use crate::ir::pl::Expr;
-
- fn print(e: Expr) -> ...

+ use crate::ir::pl;
+
+ fn print(e: pl::Expr) -> ...

This is what we do at edgedb, and it is a life saver. It also means that when you add a piece of code that needs pl::InterpolationItem, you probably don't need to add a new import, because pl is probably already imported.

This would be very nice... as a relatively new contributor, I spent so much time tracking down "is it this Expr or that Expr?" The duplication of names made searching the codebase much harder.

You can even see this in the GitHub interface - just find any instance of the word "Expr" and click on it, it shows six different places where you might need to check for its implementation!

[m-span]

What is the tradeoff on struct naming conventions for:

ir::pl::Expr

ir::rq::Expr

vs

ir::pl::PlExpr

ir::rq::RqExpr

?

Do we see one as much highly preferred than the other?

[aljazerzen]

I dislike the second option a lot because:

• we'd have to rename a lot of types in the ast modules,
• the qualifier (pl / rq) is repeated; modules exist to solve exactly this problem.

[kgutwin]

I know naming conventions are practically the definition of bikeshedding... but again, as a new contributor -- I favor things like ir::pl::PlExpr, simply because it provides better signposting when you find yourself in the middle of an unfamiliar codebase and need to get your bearings. Frequently I found that it isn't obvious at first glance what compiler phase you're in if you've been given a random line of code that's throwing a compiler panic. To be honest, I still feel that way sometimes when I'm wandering around the codebase. It would make it much easier if it was a bit more explicit that "this code is working on PL".

Unfortunately it appears that GitHub's symbol explorer isn't clever enough to look at the use statements to figure out which Expr is in scope for a given module definition. I mostly mention that because as someone who was considering contributing, it was important for me to try to get my bearings in the codebase before I went to the trouble of firing up a more sophisticated code editor. It might seem like a weird reason, but simple tools like the GitHub web interface or grep would be a lot easier to use if we went with PlExpr.

[aljazerzen]

And what do you think about that using pl::Expr instead of plain Expr?

[m-span]

Unfortunately it appears that GitHub's symbol explorer isn't clever enough to look at the use statements to figure out which Expr is in scope for a given module definition.

This feels like a good moment to mention that searching by exact symbol is a feature of a full-fledged "smart" IDE like RustRover:

[kgutwin]

And what do you think about that using pl::Expr instead of plain Expr?

Definitely a good idea! I don't know if it would help the GitHub interface be smarter (I'm guessing not) but it would still be helpful to newcomers.

[max-sixty]

And what do you think about that using pl::Expr instead of plain Expr?

Definitely — I did a very minor version of this, as an example, in #4398