Remove different behaviour when single input

[olorin37]

Addresses #1901. Proposition.

It could be so simple, but it seems that cli tests rely on specific behaviour for single file input. I'll think about it later, cause it requires grasping the way tests are made... What do you think? Is it good direction?

[vkleen]

Personally, I like the approach. The extra indirection through an import should be entirely inconsequential. I don't see a good reason why tests couldn't be made to work with this either, but I haven't given it much thought.

[olorin37]

Surely tests can be aligned, but I had to lest the issue for other duties, for a while.

[olorin37]

The main problem still remains. Snapshots for cli tests now contains import "FILE" instead actual content...

Some explicit occurences of new_from_file has been removed.

[yannham]

Thanks for tackling this, @olorin37. While it sounded like a quick and efficient solution initially, given the tests and the small annoyances coming from having this additional import indirection, I wonder if we shouldn't take a different path.

Here is the crux: when importing a file, the import resolver (the cache) parse the file according to its extension. So it selects a format based on the file name. While it's a bit fragile, it's currently still very useful and does the job. However, when loading a file without an import, the cache always parses it as Nickel.

I think it doesn't make a lot of sense: if we automatically use the file extension to drive how imported files are parsed, we should probably do the same for the main file as well. I believe it mostly amounts to get rid of all the xxx_multi methods in cache::Cache, or rather, to get rid of the non-multi variants and rename all xxx_multi to just xxx. As an example: get rid of parse and rename parse_multi to just parse. Then fix the callsites that will missing a new additional ImportFormat parameter by providing InputFormat::Nickel.

In a second step, maybe we can add a parse_auto as well, that will have the same signature as the current parse but will determine the format based on the file extension, and default to Nickel. This is exactly what Cache::resolve (used for import resolution) is doing, so this just amounts to extract a small part of it and putting in this new parse_auto function.

The last step is to use this new parse_auto inside cache::Prepare instead of parse, here precisely:

nickel/core/src/cache.rs

Line 905 in 3b01e62

if let CacheOp::Done(_) = self.parse(file_id)? {

Overall this change doesn't require any new logic, but just re-organizing the cache::Cache methods. @olorin37, do you feel like tackling this? Of course I understand that you might not have the time or the additional motivation 🙂

[olorin37]

Surly I'll try to grasp it, but I am not sure if I'll manage, cause of time:) If not I'll inform you... but first I'll try to do it.

[yannham]

Great! If you think that would help at some point, we can even schedule a little pair programming session, as it seems we're in the same timezone. Let me know (e.g. on Discord)!

[olorin37]

After the re-organization I got exactly the same problems:

━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ Snapshot Summary ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Snapshot file: cli/tests/snapshot/snapshots/snapshot__pprint-ast_stdout_simple_record.ncl.snap
Snapshot: pprint-ast_stdout_simple_record.ncl
Source: cli/tests/snapshot/main.rs:59
──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
Expression: out
──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
-old snapshot
+new results
────────────┬─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
    0       │-{
    1       │-  aaaaa | Number | default = 1,
    2       │-  bbbbb
    3       │-    : String
    4       │-    | force
    5       │-    = "some long string that goes past the 80 character line limit for pretty printing",
    6       │-  ccccc : { x : Number, y : Number } = { x = 999.8979, y = 500, },
    7       │-  ddddd
    8       │-    | Array std.string.NonEmpty
    9       │-    = [ "a", "list", "of", "non", "empty", "strings" ],
   10       │-}
          0 │+import "[INPUTS_PATH]/pretty/simple_record.ncl"
────────────┴─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────

It seems, first layer of imports should be somehow resolved.

[yannham]

@olorin37 I think you're missing one step, but you're not far. Sorry if that wasn't clear, but my suggestion of going the "get rid of xxx_multi" road is a different direction that your initial take. The idea would thus be to restore the new_from_file specific case for single files, that you originally got rid of. Finally, we need to determine the format from the path automatically in cache::Cache::prepare. More concretely:

1. Revert 59e6c28

2. Add this helper method to cache::InputFormat, next to from_path:

   /// Returns an [InputFormat] based on the file extension of a source path.
   pub fn from_source_path(source_path: &SourcePath) -> Option<InputFormat> {
     if let SourcePath::Path(p) = source_path {
       Self::from_path(p)
     }
     else {
       None
     }
   }

   (I didn't check this code, so it might need a few tweaks to make the compiler happy, but to get the idea)

3. Then, we can finally use that from within cache::Cache::prepare: replace the following line

   nickel/core/src/cache.rs

   Line 905 in 3b01e62

   if let CacheOp::Done(_) = self.parse(file_id)? {

   by something like:

   let input_format = self.file_paths.get(file_id).and_then(|source_path InputFormat::from_source_path(source_path)).unwrap_or_default();
   if let CacheOp::Done(_) = self.parse(file_id, input_format)? { 

This should let the cache select the right format automatically now, even for a single program.

I think in the long term, we might want to also do your initial simplification - the two aren't mutually exclusive - but we then need to fix the pretty printer, which is making your tests fail. Some tests are checking that nickel pprint-ast - which parse a file and pretty-print the parsed ast - behave correctly, but now it only sees the root import instead of the actual program. In the meantime, this approach - which is also simplifying the code and make the input format parameter passing more explicit - is probably the fastest way to get there.

[olorin37]

Right, I missed that you proposal is an alternative to mine. And, yes I actually do what you proposed without understanding how it would help those snapshot tests (with pretty print). So, I'll follow your idea and tips.

"Less is more" removing this one unnecessary case from my initial proposition should be done some day I think, unless it does not complicates things even more 🤔

[olorin37]

@yannham Thanks for guidance, now it is working. Please, make an review, anything to improve (with this "fast-path" approach)?

[yannham]

The parse_auto part isn't mandatory, and we can do that in a subsequent PR. However I think we should fix the import issue in ncl_bench_group before proceeding with this PR.

[yannham]

I think you should either:

1. Use a fully qualified name for InputFormat, that is nickel_lang_core::cache::InputFormat
2. Import it a few line above, inside the macro, and keep it as InputFormat

But in any case, the consumers/callers of ncl_bench_group should (Edit: + NOT) have to import it themselves (as you had to do in various bench files). This leads to code duplication, but more importantly, callers of nlc_bench_group have to magically guess that they need to do this import (or wait for a compilation error), while the macro should - and can - be self-contained.

[yannham]

Edit: I meant should NOT have to import it themselves.

[olorin37]

Yep, right now I see. I missed that the code I edited was inside a macro, sure it's a mistake. So, I'll import it in macro, and get rid of those imports from benches:)

[yannham]

(See comment in ncl_bench_group, but example of import that we shouldn't have to do)

[yannham]

I wonder if those shouldn't guess the format as well, instead of InputFormat::Nickel. That's why it could be handy to have a parse_auto (the name isn't great, so feel free to use another if you find some!). However, this one can also be done in a subsequent PR, as the current code at least fixes the original issue.

[olorin37]

parse_infer_format? less concise, but more descriptive ...

[yannham]

@olorin37 right, it's a reasonable suggestion and better than parse_auto, so go for it 🙂

[yannham]

Same, I think any parse from within Program methods should probably try to guess the format, to have the same behavior as prepare.

[yannham]

Looks great, thank you! We've settled (in chat conversations) on implementing this new parse_guess_format or whatever in a follow-up PR, and not in this one, to get the ball rolling.