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.

Sign up for GitHub

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

Prevent duplication of include_bytes! statements #85

Closed
wants to merge 1 commit into from
Closed

Prevent duplication of include_bytes! statements #85

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 21 additions & 17 deletions rinja_derive/src/generator.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,10 @@ use std::collections::hash_map::{Entry, HashMap};
use std::fmt::{Arguments, Display, Write};
use std::ops::Deref;
use std::path::Path;
use std::sync::Arc;
use std::sync::{Arc, OnceLock};
use std::{cmp, hash, mem, str};

use once_map::OnceMap;
use parser::node::{
Call, Comment, CondTest, FilterBlock, If, Include, Let, Lit, Loop, Match, Whitespace, Ws,
};
Expand Down Expand Up @@ -88,6 +89,23 @@ impl<'a> Generator<'a> {
Ok(buf.buf)
}

fn generate_include_bytes<S: AsRef<str>>(&mut self, buf: &mut Buffer, path: S) {
static CACHE: OnceLock<OnceMap<String, &'static ()>> = OnceLock::new();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about it, I don't think using a cache this why is sane:

  • Given the file layout.html is used in compiling units a.rs and b.rs, then the include might wind up only in one of the generated objects, say, a.rs.
  • Afterwards, we remove a.rs and any mentions to it.
  • Then we alter layout.html: b.rs does not mention the dependency, so the change won't cause a recompilation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we remove a.rs, the crate will be recompiled and layout.html will be used in b.rs. Or did I miss something?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You cannot assume that the whole crate is recompiled when incremental compilation is enabled, can you?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hum... It's a good question. But since you remove a.rs, it also means you remove mod a somewhere else, so in this case, I wonder if the whole crate isn't simply rebuilt...

Copy link
Collaborator

@Kijewski Kijewski Jul 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But what if the add more indirection? (Assume the usual folder structure):

mod x1 {
    mod x2 {
        mod a;
    }
    mod x3 {
        mob b;
    }
}

Then removing mod a will cause a recompilation of mod x2, but not x1. If the need to rebuild would bubble up, then there could be no incremental compilation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fact that we're debating over this is already a pretty good sign that we should not do it. Closing then! :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe once upon a time rust-lang/rust#99515 gets implemented. :) If I understand the proposal correctly, then this could be a game changer, and we could even use spans inside the template file to show error messages, etc.


let path = path.as_ref();
let cache = CACHE.get_or_init(OnceMap::new);
if cache.contains_key(path) {
return;
}
buf.writeln(
quote! {
const _: &[::core::primitive::u8] = ::core::include_bytes!(#path);
}
.to_string(),
);
cache.insert(path.to_string(), |_| &());
}

// Implement `Template` for the given context struct.
fn impl_template(&mut self, ctx: &Context<'a>, buf: &mut Buffer) -> Result<(), CompileError> {
self.write_header(buf, format_args!("{CRATE}::Template"), None);
Expand All @@ -109,13 +127,7 @@ impl<'a> Generator<'a> {
Source::Source(_) => **path != self.input.path,
};
if path_is_valid {
let path = path.to_str().unwrap();
buf.writeln(
quote! {
const _: &[::core::primitive::u8] = ::core::include_bytes!(#path);
}
.to_string(),
);
self.generate_include_bytes(buf, path.to_str().unwrap());
}
}

Expand Down Expand Up @@ -789,15 +801,7 @@ impl<'a> Generator<'a> {
.find_template(i.path, Some(&self.input.path))?;

// Make sure the compiler understands that the generated code depends on the template file.
{
let path = path.to_str().unwrap();
buf.writeln(
quote! {
const _: &[::core::primitive::u8] = ::core::include_bytes!(#path);
}
.to_string(),
);
}
self.generate_include_bytes(buf, path.to_str().unwrap());

// We clone the context of the child in order to preserve their macros and imports.
// But also add all the imports and macros from this template that don't override the
Expand Down
Loading