Add read_file macro method

[Sija]

#2791, again! ;)

/cc @asterite

[bcardiff]

I don't think the discussion will differ. Better to find a way to embed slices.

That been said. I care more about API. So, if the macro would return a slice, even if the string is embedded as a first implementation, that is something I could live with. Until a better implementation without breaking the API will come.

[sdogruyol]

I've wanted to have this for a long time. Let's KISS and start with something easy to iterate on like this 👍

[sdogruyol]

I've wanted to have this for a long time. Let's KISS and start with something easy to iterate on like this 👍

[sdogruyol]

I've wanted to have this for a long time. Let's KISS and start with something easy to iterate on like this 👍

[asterite]

Right now a string can contain any kind of bytes, so I think this is fine. If someone wants to embed a slice of bytes they can just do:

DATA = {{ read_file("whatever") }}.to_slice

[ysbaddaden]

I'd like a kwarg, to have a String (default) or Bytes (on demand), but maybe it would be confusing, and always having a String is better?

{{ read_file("LICENSE.md") }}             # => String
{{ read_file("logo.png", binary: true) }} # => Bytes

[jhass]

I don't see a strong reason to have it for string, worst case it should be a String.new({{read_file...) away, but if we want it I'd vote for having read_file to return Bytes and read_file_to_string or perhaps read_text_file to return String.

[asterite]

The problem is that read_file is a macro method, and, like all macro methods, it returns an AST node. There's a StringLiteral node to represent a string. There's no node to represent a slice of bytes. So if you want that you'll have to:

• introduce such node
• define a syntax to write it (maybe Bytes[...] would be enough)
• provide operations on it

In my mind it's just simpler to make read_file return a string. I don't see any problem with it, and you can manipulate the data at compile-time just fine.

[jhass]

The proper implementation should do something like #2791 (comment), so generate an AST subtree equivalent to the inline assembly call.

As Brian points out, introducing it now to return a string literal would just force a breaking API change in the future.

On a compromise, maybe we can do a text only version for now and call it read_text_file or the like?

[asterite]

If it returns an asm node there's no way to manipulate the data at compile-time. I still don't understand what's the problem with having it be a StringLiteral, could someone explain this to me? :-)

[jhass]

@asterite

Until a better implementation without breaking the API will come.

As Brian points out, introducing it now to return a string literal would just force a breaking API change in the future.

I'm not sure how I can present the argument any more clear than that, sorry.

[asterite]

But read_file returning StringLiteral is perfect and there won't be a need to change it's API. I mean, we are assuming it needs to be changed because it has something wrong with it. I want to know what that is.

[jhass]

Maybe it's the old discussion about what String should be, generic data blob or representation of sequence of codepoints in some encoding and me strongly favoring the latter as I see doing the former one of the biggest mistakes of Ruby. Also if we would go for the former we should remove Slice.

[jhass]

Also if we keep the concerns separate, that is read some text vs just give me the bytes, we can make the former pleasant to use at some point by applying any necessary encoding transformations.

[asterite]

Then I guess a conclusion won't be reached here. I propose to close this then until someone figures out how to lay out everything related to strings and bytes.

[straight-shoota]

@jhass String.new({{ read_file }}) would copy the slice to the new String instance.

Having a binary argument as @ysbaddaden suggested seems like a good idea. If false, it returns a StringLiteral. If binary: true, it could just append a call to #to_slice to the StringLiteral. And later, when there is a proper bytes literal, it could just be upgraded to that without any changes in the API.

[bcardiff]

• I'm ok with read_text_file.
• I'm ok with read_file to expand to a Call(StringLiteral, :to_slice) and whatever is needed to simulate a slice literal.
• I'm ok with not having read_file now, but adding read_text_file now
• I'm not ok with changing the returned type based on an argument. Even in macros. So no for binary: true.

So, if the macro name is changed and the review of @asterite regarding path is solved, this is GTG on my side.

[RX14]

There's clearly two different things wanted here:

• a way to read files inside a macro and then use the contents of the file inside the macro to make decisions on the macro level. This may expose the contents of a file at runtime but doesn't have to. The most reasonable way to do this is returning a StringLiteral. Think VERSION file.
• a way to efficiently embed large, possibly binary, files inside the executable to make exevutables more self-contained. Think compiled image and JS assets for a webapp.

These are orthogonal and want different interfaces. The former can be read_file and would be a macro method. The latter can be achieved using module-level ASM to embed a file in the ELF directly using assembler directives. I'd call it embed_file and it'd be a macro which expanded to a global_asm block and a Slice.new call. (and macros are not macro methods)

[jhass]

Good point, thank you. I still think we should be cautious with introducing the first case while the second doesn't exist, because during that period people will abuse it for the second case. So I stick to preferring read_text_file as a name, I don't think it harms the first case in any way and communicates its contract much more clearly.

[straight-shoota]

@RX14 The embedded data could be binary but also text. For text, it should simple to get a String
at runtime without copying the slice.

[RX14]

@straight-shoota you could just use read_text_file for that.

[straight-shoota]

Yeah, but it feels a bit weird to have read_text_file but embed_file for the same thing with a binary file.

[jhass]

Yeah, but it feels a bit weird to have read_text_file but embed_file for the same thing with a binary file.

That's a discussion we can have once we do embed_file, let's not derail this further than necessary.

[RX14]

Yeah, but it feels a bit weird to have read_text_file but embed_file for the same thing with a binary file.

But it's not the same thing, conceptually or in implementation. That was the whole point of my comment above.

[straight-shoota]

Both fits in the second bullet point. If I want to embed a file into the binary, I can use embed_file for binary data. But if it's text data and it's supposed to be accessible as String at runtime, I need to use read_text_file instead.

[asterite]

Any other language that lets you embed data using asm? It seems like a huge portability pain to do that.

[RX14]

@asterite any language with asm which uses a real assembler lets you do it.

Doing file embedding with global_asm just means that we pass all the work over to LLVM, and don't have to add any code in the crystal compiler to do this. The asm isn't platform-specific because the ASM block consists only of assembler directives, no instructions.

The implementation doesn't matter anyway.

[bcardiff]

NB: We should wait for 1 more core team re review before merging. And it should be merged with a squash.

[straight-shoota]

@Sija could squash it and add a nice commit message =)

[bcardiff]

Merged :-)
The nice commit message was the title :-P

Thanks everybody for the discussion!

[Sija]

It occured to me that perhaps it would be sensible to have nilable read_file? (current read_file) and raising read_file variants, whatcha think?

[bcardiff]

@Sija you mean that read_file will cause a compile time failure if the file does not exist, right?
I think that make sense I would not even add a read_file?.

[Sija]

@bcardiff yep, exactly. Although I would (add read_file? variant), since I can easily imagine use case where you'd like to read file with certain fallback which wouldn't be possible when using raising variant.

[Sija]

@asterite true, yet it simplifies things in case like this:

{% version = read_file("../../VERSION") %}
{{ version ? version.chomp : nil }}

vs

{{ read_file("../../VERSION").chomp }}

[straight-shoota]

{{ read_file("../../VERSION") || raise "Missing VERSION file" }}

[Sija]

@straight-shoota nope, it ain't work since I'd need to use .chomp. Having #try would solve it but sadly, it doesn't exist in macros.

[straight-shoota]

{{ (read_file("../../VERSION") || raise "Missing VERSION file").chomp }}

[woodruffw]

👍 For a version that errors at compile time if the file doesn't exist (or I/O fails in any way). The second || raise workaround suffices, but having the error as a property of the macro itself is DRYer and feels like a natural extension of an I/O operation at compile time.

[Sija]

I agree with @woodruffw, @straight-shoota's workaround is good enough (though it still "hides" the root cause of not returning the file contents - which might be for instance permissions problem, not only missing file), yet having it "baked" into the method itself feels more natural (as long as there's nilable variant too).