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

parser: shrink the size of WithSpan to one register #257

Merged
merged 2 commits into from
Nov 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
161 changes: 82 additions & 79 deletions rinja_derive/src/generator.rs

Large diffs are not rendered by default.

24 changes: 10 additions & 14 deletions rinja_derive/src/heritage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use std::path::Path;
use std::sync::Arc;

use parser::node::{BlockDef, Macro};
use parser::{Node, Parsed, WithSpan};
use parser::{Node, Parsed, Span};
use rustc_hash::FxBuildHasher;

use crate::config::Config;
Expand Down Expand Up @@ -80,29 +80,29 @@ impl Context<'_> {
for n in nodes {
match n {
Node::Extends(e) => {
ensure_top(top, e, path, parsed, "extends")?;
ensure_top(top, e.span(), path, parsed, "extends")?;
if extends.is_some() {
return Err(CompileError::new(
"multiple extend blocks found",
Some(FileInfo::of(e, path, parsed)),
Some(FileInfo::of(e.span(), path, parsed)),
));
}
extends = Some(config.find_template(
e.path,
Some(path),
Some(FileInfo::of(e, path, parsed)),
Some(FileInfo::of(e.span(), path, parsed)),
)?);
}
Node::Macro(m) => {
ensure_top(top, m, path, parsed, "macro")?;
ensure_top(top, m.span(), path, parsed, "macro")?;
macros.insert(m.name, &**m);
}
Node::Import(import) => {
ensure_top(top, import, path, parsed, "import")?;
ensure_top(top, import.span(), path, parsed, "import")?;
let path = config.find_template(
import.path,
Some(path),
Some(FileInfo::of(import, path, parsed)),
Some(FileInfo::of(import.span(), path, parsed)),
)?;
imports.insert(import.scope, path);
}
Expand Down Expand Up @@ -141,21 +141,17 @@ impl Context<'_> {
})
}

pub(crate) fn generate_error<T>(
&self,
msg: impl fmt::Display,
node: &WithSpan<'_, T>,
) -> CompileError {
pub(crate) fn generate_error(&self, msg: impl fmt::Display, node: Span<'_>) -> CompileError {
CompileError::new(
msg,
self.path.map(|path| FileInfo::of(node, path, self.parsed)),
)
}
}

fn ensure_top<T>(
fn ensure_top(
top: bool,
node: &WithSpan<'_, T>,
node: Span<'_>,
path: &Path,
parsed: &Parsed,
kind: &str,
Expand Down
13 changes: 9 additions & 4 deletions rinja_derive/src/input.rs
Original file line number Diff line number Diff line change
Expand Up @@ -185,9 +185,14 @@ impl TemplateInput<'_> {
// Add a dummy entry to `map` in order to prevent adding `path`
// multiple times to `check`.
let new_path = e.key();
let source = parsed.source();
let source = get_template_source(
new_path,
Some((&path, parsed.source(), n.span())),
Some((
&path,
source,
n.span().as_suffix_of(source).unwrap_or_default(),
)),
)?;
check.push((new_path.clone(), source, Some(new_path.clone())));
e.insert(Arc::default());
Expand All @@ -200,7 +205,7 @@ impl TemplateInput<'_> {
let extends = self.config.find_template(
extends.path,
Some(&path),
Some(FileInfo::of(extends, &path, &parsed)),
Some(FileInfo::of(extends.span(), &path, &parsed)),
)?;
let dependency_path = (path.clone(), extends.clone());
if path == extends {
Expand All @@ -220,7 +225,7 @@ impl TemplateInput<'_> {
let import = self.config.find_template(
import.path,
Some(&path),
Some(FileInfo::of(import, &path, &parsed)),
Some(FileInfo::of(import.span(), &path, &parsed)),
)?;
add_to_check(import)?;
}
Expand All @@ -231,7 +236,7 @@ impl TemplateInput<'_> {
let include = self.config.find_template(
include.path,
Some(&path),
Some(FileInfo::of(include, &path, &parsed)),
Some(FileInfo::of(include.span(), &path, &parsed)),
)?;
add_to_check(include)?;
}
Expand Down
9 changes: 5 additions & 4 deletions rinja_derive/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ use generator::template_to_string;
use heritage::{Context, Heritage};
use input::{Print, TemplateArgs, TemplateInput};
use integration::Buffer;
use parser::{Parsed, WithSpan, strip_common};
use parser::{Parsed, strip_common};
#[cfg(not(feature = "__standalone"))]
use proc_macro::TokenStream as TokenStream12;
#[cfg(feature = "__standalone")]
Expand Down Expand Up @@ -294,11 +294,12 @@ impl<'a> FileInfo<'a> {
}
}

fn of<T>(node: &WithSpan<'a, T>, path: &'a Path, parsed: &'a Parsed) -> Self {
fn of(node: parser::Span<'a>, path: &'a Path, parsed: &'a Parsed) -> Self {
let source = parsed.source();
Self {
path,
source: Some(parsed.source()),
node_source: Some(node.span()),
source: Some(source),
node_source: node.as_suffix_of(source),
}
}
}
Expand Down
12 changes: 6 additions & 6 deletions rinja_parser/src/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,9 @@ use winnow::combinator::{
use winnow::error::{ErrorKind, ParserError as _};

use crate::{
CharLit, ErrorContext, Level, Num, ParseErr, ParseResult, PathOrIdentifier, StrLit, WithSpan,
char_lit, filter, identifier, keyword, num_lit, path_or_identifier, skip_ws0, skip_ws1,
str_lit, ws,
CharLit, ErrorContext, Level, Num, ParseErr, ParseResult, PathOrIdentifier, Span, StrLit,
WithSpan, char_lit, filter, identifier, keyword, num_lit, path_or_identifier, skip_ws0,
skip_ws1, str_lit, ws,
};

macro_rules! expr_prec_layer {
Expand Down Expand Up @@ -217,14 +217,14 @@ impl<'a> Expr<'a> {
allow_underscore: bool,
) -> ParseResult<'a, WithSpan<'a, Self>> {
let (_, level) = level.nest(i)?;
let start = i;
let start = Span::from(i);
let range_right =
move |i| (ws(alt(("..=", ".."))), opt(move |i| Self::or(i, level))).parse_next(i);
let (i, expr) = alt((
range_right.map(|(op, right)| {
range_right.map(move |(op, right)| {
WithSpan::new(Self::Range(op, None, right.map(Box::new)), start)
}),
(move |i| Self::or(i, level), opt(range_right)).map(|(left, right)| match right {
(move |i| Self::or(i, level), opt(range_right)).map(move |(left, right)| match right {
Some((op, right)) => WithSpan::new(
Self::Range(op, Some(Box::new(left)), right.map(Box::new)),
start,
Expand Down
95 changes: 80 additions & 15 deletions rinja_parser/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -114,11 +114,11 @@ impl<'a> Ast<'a> {
Ok(("", nodes)) => Ok(Self { nodes }),
Ok(_) | Err(winnow::error::ErrMode::Incomplete(_)) => unreachable!(),
Err(
winnow::error::ErrMode::Backtrack(ErrorContext { input, message, .. })
| winnow::error::ErrMode::Cut(ErrorContext { input, message, .. }),
winnow::error::ErrMode::Backtrack(ErrorContext { span, message, .. })
| winnow::error::ErrMode::Cut(ErrorContext { span, message, .. }),
) => Err(ParseError {
message,
offset: src.len() - input.len(),
offset: span.offset_from(src).unwrap_or_default(),
file_path,
}),
}
Expand All @@ -134,19 +134,76 @@ impl<'a> Ast<'a> {
/// in the code generation.
pub struct WithSpan<'a, T> {
inner: T,
span: &'a str,
span: Span<'a>,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

To ensure the size doesn't grow up, might be worth adding a compile-time size_of check of WithSpan and of Span.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think it matters too much, as the PR fixes another problem, too:

If the input span is shrunk at the end of the input, e.g. let i = i.trim(), then the error messages will be broken afterward, because only the lengths are compared. This PR fixes that, too. This was actually my original intention with, because I had to refactor some code a few times in order not to break the error messages. That the Span is smaller afterwards is just a happy co-incidence. :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added a check, anyway. :)


/// An location in `&'a str`
#[derive(Debug, Clone, Copy)]
pub struct Span<'a>(&'a [u8; 0]);

impl Default for Span<'static> {
#[inline]
fn default() -> Self {
Self::empty()
}
}

impl<'a> Span<'a> {
#[inline]
pub const fn empty() -> Self {
Self(&[])
}

pub fn offset_from(self, start: &'a str) -> Option<usize> {
let start_range = start.as_bytes().as_ptr_range();
let this_ptr = self.0.as_slice().as_ptr();
match start_range.contains(&this_ptr) {
// SAFETY: we just checked that `this_ptr` is inside `start_range`
true => Some(unsafe { this_ptr.offset_from(start_range.start) as usize }),
false => None,
}
}

pub fn as_suffix_of(self, start: &'a str) -> Option<&'a str> {
let offset = self.offset_from(start)?;
match start.is_char_boundary(offset) {
true => Some(&start[offset..]),
false => None,
}
}
}

impl<'a> From<&'a str> for Span<'a> {
#[inline]
fn from(value: &'a str) -> Self {
Self(value[..0].as_bytes().try_into().unwrap())
}
}

impl<'a, T> WithSpan<'a, T> {
pub const fn new(inner: T, span: &'a str) -> Self {
Self { inner, span }
#[inline]
pub fn new(inner: T, span: impl Into<Span<'a>>) -> Self {
Self {
inner,
span: span.into(),
}
}

#[inline]
pub const fn new_without_span(inner: T) -> Self {
Self {
inner,
span: Span::empty(),
}
}

pub fn span(&self) -> &'a str {
#[inline]
pub fn span(&self) -> Span<'a> {
self.span
}

pub fn deconstruct(self) -> (T, &'a str) {
#[inline]
pub fn deconstruct(self) -> (T, Span<'a>) {
let Self { inner, span } = self;
(inner, span)
}
Expand Down Expand Up @@ -229,18 +286,18 @@ pub(crate) type ParseResult<'a, T = &'a str> = Result<(&'a str, T), ParseErr<'a>
/// `rinja`'s users experience less good (since this generic is only needed for `nom`).
#[derive(Debug)]
pub(crate) struct ErrorContext<'a> {
pub(crate) input: &'a str,
pub(crate) span: Span<'a>,
pub(crate) message: Option<Cow<'static, str>>,
}

impl<'a> ErrorContext<'a> {
fn unclosed(kind: &str, tag: &str, i: &'a str) -> Self {
Self::new(format!("unclosed {kind}, missing {tag:?}"), i)
fn unclosed(kind: &str, tag: &str, span: impl Into<Span<'a>>) -> Self {
Self::new(format!("unclosed {kind}, missing {tag:?}"), span)
}

fn new(message: impl Into<Cow<'static, str>>, input: &'a str) -> Self {
fn new(message: impl Into<Cow<'static, str>>, span: impl Into<Span<'a>>) -> Self {
Self {
input,
span: span.into(),
message: Some(message.into()),
}
}
Expand All @@ -249,7 +306,7 @@ impl<'a> ErrorContext<'a> {
impl<'a> winnow::error::ParserError<&'a str> for ErrorContext<'a> {
fn from_error_kind(input: &'a str, _code: ErrorKind) -> Self {
Self {
input,
span: input.into(),
message: None,
}
}
Expand All @@ -262,7 +319,7 @@ impl<'a> winnow::error::ParserError<&'a str> for ErrorContext<'a> {
impl<'a, E: std::fmt::Display> FromExternalError<&'a str, E> for ErrorContext<'a> {
fn from_external_error(input: &'a str, _kind: ErrorKind, e: E) -> Self {
Self {
input,
span: input.into(),
message: Some(Cow::Owned(e.to_string())),
}
}
Expand Down Expand Up @@ -1213,3 +1270,11 @@ mod test {
assert!(str_lit.parse_next(r#"d"hello""#).is_err());
}
}

#[test]
fn assert_span_size() {
assert_eq!(
std::mem::size_of::<Span<'static>>(),
std::mem::size_of::<*const ()>()
);
}
14 changes: 9 additions & 5 deletions rinja_parser/src/node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ use winnow::token::{any, tag};

use crate::memchr_splitter::{Splitter1, Splitter2, Splitter3};
use crate::{
ErrorContext, Expr, Filter, ParseResult, State, Target, WithSpan, filter, identifier, keyword,
skip_till, skip_ws0, str_lit_without_prefix, ws,
ErrorContext, Expr, Filter, ParseResult, Span, State, Target, WithSpan, filter, identifier,
keyword, skip_till, skip_ws0, str_lit_without_prefix, ws,
};

#[derive(Debug, PartialEq)]
Expand Down Expand Up @@ -43,7 +43,9 @@ impl<'a> Node<'a> {
&err
{
if err.message.is_none() {
opt(|i| unexpected_tag(i, s)).parse_next(err.input)?;
if let Some(span) = err.span.as_suffix_of(i) {
opt(|i| unexpected_tag(i, s)).parse_next(span)?;
}
}
}
return Err(err);
Expand Down Expand Up @@ -184,7 +186,7 @@ impl<'a> Node<'a> {
}

#[must_use]
pub fn span(&self) -> &str {
pub fn span(&self) -> Span<'a> {
match self {
Self::Lit(span) => span.span,
Self::Comment(span) => span.span,
Expand Down Expand Up @@ -218,7 +220,9 @@ fn cut_node<'a, O>(
&result
{
if err.message.is_none() {
opt(|i| unexpected_raw_tag(kind, i)).parse_next(err.input)?;
if let Some(span) = err.span.as_suffix_of(i) {
opt(|i| unexpected_raw_tag(kind, i)).parse_next(span)?;
}
}
}
result
Expand Down
18 changes: 16 additions & 2 deletions rinja_parser/src/tests.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,14 @@
use crate::node::{Lit, Whitespace, Ws};
use crate::{Ast, Expr, Filter, InnerSyntax, Node, Num, StrLit, Syntax, SyntaxBuilder, WithSpan};
use crate::{
Ast, Expr, Filter, InnerSyntax, Node, Num, Span, StrLit, Syntax, SyntaxBuilder, WithSpan,
};

impl<T> WithSpan<'static, T> {
fn no_span(inner: T) -> Self {
Self { inner, span: "" }
Self {
inner,
span: Span::default(),
}
}
}

Expand Down Expand Up @@ -1122,3 +1127,12 @@ fn extends_with_whitespace_control() {
}
}
}

#[test]
fn fuzzed_span_is_not_substring_of_source() {
let _: Result<Ast<'_>, crate::ParseError> = Ast::from_str(
include_str!("../tests/fuzzed_span_is_not_substring_of_source.bin"),
None,
&Syntax::default(),
);
}
Binary file not shown.