Avoid compilation for justfile formatting

[terror]

--fmt only uses the root AST's Display impl, but previously went through full compilation, including import and module resolution. This meant --fmt would fail with MissingImportFile or MissingModuleFile errors for files it never needed.

This diff moves Format handling to before the Self::compile() call, similar to Edit, so it loads, lexes, and parses only the root file. The unstable check inspects the AST directly for set unstable rather than going through Config::require_unstable, which requires a fully compiled Justfile.

[terror]

Related: #3088

[casey]

This is definitely a good feature, although I'm not sure I like the duplication that it introduces. Lexing and parsing was previously only done in Compiler::compile, but now it's done both there and in Subcommand::format. Can this be de-duplicated? Perhaps by creating a helper function on Compiler which does lexing and parsing for a Source, which can be called in both places?

[casey]

Is this ready for another look? I ignore notifications from pushed changes to a branch, since they're often just noise.

[terror]

Is this ready for another look? I ignore notifications from pushed changes to a branch, since they're often just noise.

No worries, yeah this should be ready. I lifted out a Compiler::parse to avoid duplicating lexing/parsing. We could take it a step further and also load in this function, but 🤷

I also don't know if this should live on Compiler. Seems better fitted for Parser?

[casey]

I agree that putting it on Parser is better. Ideally, we could replace Parser::parse with something that would take a Source, but if that doesn't work Parser::parse_source that takes a Source is fine.

[casey]

Why did the include_justfile test go away?

[terror]

Why did the include_justfile test go away?

Compilation::root_src is now unused outside of tests, but I guess it could be a non-panic test, or make root_src test only 🤔

[casey]

What was it testing? Are there other tests testing the same thing, or did we lose coverage by deleting it?

[terror]

What was it testing? Are there other tests testing the same thing, or did we lose coverage by deleting it?

It was testing that Compiler::compile successfully handles a chain of transitive imports across multiple files. From what I can tell, there are integration tests that cover the same thing, i.e. nested_import_paths_are_relative_to_containing_submodule, recipes_in_nested_imports_run_in_parent_module, and nested_multiply_imported_items_do_not_conflict.

[casey]

Dope, LGTM!