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

Move !include processing into the parser #1618

Merged
merged 67 commits into from
Nov 21, 2023
Merged

Conversation

neunenak
Copy link
Contributor

@neunenak neunenak commented Jun 15, 2023

Handle !include directives as an ordinary parsed AST node rather than as a special-case text include in the loader. This requires a number of changes to the lexer, parser, loader, analyzer, and in-memory representation of Justfiles.

With this change, the loader will load the root justfile given on the command line and pass the text to the parser to lex and parse it. This file may have !include directives in it, which will be parsed as a new type of Item. After parsing, an analyze step will search the AST for any nodes corresponding to !includes. If any are found, the loader will then load those external justfiles, parse them into separate ASTs, and recursively search them for further !includes until all recurisvely-included justfiles are processed. At this point the analyzer will handle building the Justfile data structure out of the combined set of ASTs, as if they corresponded to one large AST built from the concatenated text of all the included files. Once the Justfile data structure is built, just can run as normal.

This change will make it possible to put !include directives anywhere in a justfile, make it easier to provide error messages that reference their actual source justfile, and make it easier to implement features that need to be aware of source justfiles like #1583 or #1580 .

@neunenak neunenak force-pushed the parse-before-load branch 3 times, most recently from b485885 to b8e1ba2 Compare June 27, 2023 07:37
@neunenak neunenak marked this pull request as ready for review June 27, 2023 07:49
@neunenak
Copy link
Contributor Author

@casey any chance you could take a look at this PR? It's a lengthy one, but I think doing an architectural change along these lines is a good idea in the long run.

Compiler::compile() now outputs a struct `Compilation` instead of a
tuple of `Ast` and `Justfile`. Right now this `Compilation` struct
contains the same ast and justfile, but abstracting this output into a
new type will make it easier to handle justfiles with includes, which
will necessarily have multiple Ast's (from different included files) as
part of a coherent structure.
Still need to thread debug info through
@casey
Copy link
Owner

casey commented Nov 21, 2023

I was looking at lex_bang_expression, and I noticed that it checked for {{ while lexing the include path. I removed it and no tests failed, which allowed me to replace the check with the Lexer::at_eol_or_eof helper function, and I wanted to make sure I wasn't missing something.

@neunenak
Copy link
Contributor Author

Sorry I've been so delinquent on this! I'm reviewing it now.

Are there any breaking behavior changes between this and !include? I'm hoping not, since otherwise we should think about leaving the old !include functionality intact, so that people can migrate, since although !include is unstable, it has a lot of users.

What's the best way to approach reviewing this PR? Is there a natural entry point and order to the review?

No worries thanks for taking a look.

There shouldn't be any breaking changes to how !include works in this PR. These changes should make it easier to make possibly-breaking changes to how !include syntax works in the future though, although I agree that it would be good to minimize those as much as possible despite the instability of the feature.

I think the best natural entry point is the load_and_compile function in loader.rs, which replaces the old load function. That's the first place in the code path from starting just to executing a recipe where this PR's code does something meaningfully different.

@neunenak
Copy link
Contributor Author

I was looking at lex_bang_expression, and I noticed that it checked for {{ while lexing the include path. I removed it and no tests failed, which allowed me to replace the check with the Lexer::at_eol_or_eof helper function, and I wanted to make sure I wasn't missing something.

I don't quite remember what I had in mind here, and it's possible I just copy-pasted that code from something else. I don't think we want to treat {{ }} as syntactically-special within an !include directive for now (and maybe not ever), so that change should be fine.

@casey casey merged commit f745316 into casey:master Nov 21, 2023
5 checks passed
@casey
Copy link
Owner

casey commented Nov 21, 2023

I did a big refactor before merging. The main thing I wanted to do was reduce the size of the diff for review, the diff in the analyzer in particular was hard for me to read, so I refactored it to leave the analyzer mostly untouched. I moved the code to lex, parse, and load includes into the compiler, since that seems like a better place than the loader, which now just loads files into the arena. The Compiler creates adds canonical path to Item::Imports, maps of canonical paths to source code, and canonical paths to asts, along with the root path, and passes them to the analyzer, which can now be mostly unchanged. The maps can later be used to improve error reporting, and make dump and format dump/format all justfiles.

@casey
Copy link
Owner

casey commented Nov 21, 2023

I created this #1735 to track actually stabilizing the feature.

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.

2 participants