Skip to content

Commit

Permalink
fix: Don't include whitespace in the span with empty nonterminals
Browse files Browse the repository at this point in the history
Empty non-terminals used to be spanned by the whitespace between the
previous and next token. If the non-terminal was the first thing in
another production this span would leak into that production, causing it
to have a to large span assigned.

This makes empty productions instead only point to the start of the
lookahead to avoid this problem.
  • Loading branch information
Marwes committed May 10, 2020
1 parent 672716e commit 11a50e7
Show file tree
Hide file tree
Showing 7 changed files with 224 additions and 228 deletions.
70 changes: 35 additions & 35 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 12 additions & 0 deletions lalrpop-test/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,8 @@ lalrpop_mod!(

lalrpop_mod!(comments);

lalrpop_mod!(sp_from_optional);

pub fn use_cfg_created_parser() {
cfg::CreatedParser::new();
}
Expand Down Expand Up @@ -1017,3 +1019,13 @@ fn comments() {
vec!["22", "3", "5", "13"]
);
}

#[test]
fn sp_from_optional() {
assert_eq!(
sp_from_optional::TestParser::new()
.parse("before let")
.unwrap(),
(9, "let", 12)
);
}
6 changes: 3 additions & 3 deletions lalrpop-test/src/loc_issue_90_lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ Ok(
Upgrade(
0,
Wonky(
5,
6,
6
),
6
Expand Down Expand Up @@ -125,8 +125,8 @@ Ok(
Ref(
0,
Maybe(
1,
1
2,
2
),
Ident(
2,
Expand Down
13 changes: 13 additions & 0 deletions lalrpop-test/src/sp_from_optional.lalrpop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
grammar;

Sp<T> = @L T @R;

Meta: () = {
"comment" => (),
=> (),
};

// An empty `Meta` production here should not cause the span of `Let` to include the preceding whitespace
Let: &'input str = Meta <"let">;

pub Test: (usize, &'input str, usize) = "before" <Sp<Let>>;
46 changes: 15 additions & 31 deletions lalrpop/src/lr1/codegen/ascent.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,9 @@ use crate::lr1::core::*;
use crate::lr1::lookahead::Token;
use crate::lr1::state_graph::StateGraph;
use crate::rust::RustWrite;
use std::io::{self, Write};
use crate::tls::Tls;
use crate::util::{Escape, Sep};
use std::io::{self, Write};

use super::base::CodeGenerator;

Expand Down Expand Up @@ -843,39 +843,39 @@ impl<'ascent, 'grammar, W: Write>
};
let transfer_syms = self.pop_syms(optional, fixed, production_inputs)?;

// identify the "start" location for this production; this
// is typically the start of the first symbol we are
// reducing; but in the case of an empty production, it
// will be the last symbol pushed, or at worst `default`.
if let Some(first_sym) = transfer_syms.first() {
// identify the "start" and "end" location for this production; this
// is typically the start of the first symbol and end of the last symbol we are
// reducing; but in the case of an empty production, it will come from the
// lookahead or the end of the last symbol pushed
if let (Some(first_sym), Some(last_sym)) = (transfer_syms.first(), transfer_syms.last()) {
rust!(
self.out,
"let {}start = {}.0.clone();",
self.prefix,
first_sym
);
rust!(self.out, "let {}end = {}.2.clone();", self.prefix, last_sym);
} else if stack_suffix.len() > 0 {
// we pop no symbols, so grab from the top of the stack
// (unless we are in the start state)
let top = stack_suffix.len() - 1;
if !stack_suffix.fixed().is_empty() {
rust!(
self.out,
"let {}start = {}sym{}.2.clone();",
self.prefix,
self.prefix,
top
"let {p}start = {p}lookahead.as_ref().map(|o| o.0.clone()).unwrap_or_else(|| {p}sym{top}.2.clone());",
p = self.prefix,
top = top
);
} else {
// top of stack is optional; should not have been popped yet tho
rust!(
self.out,
"let {}start = {}sym{}.as_ref().unwrap().2.clone();",
self.prefix,
self.prefix,
top
"let {p}start = {p}lookahead.as_ref().map(|o| o.0.clone()).unwrap_or_else(|| {p}sym{top}.as_ref().unwrap().2.clone());",
p = self.prefix,
top = top
);
}
rust!(self.out, "let {p}end = {p}start;", p = self.prefix);
} else {
// this only occurs in the start state
rust!(
Expand All @@ -884,23 +884,7 @@ impl<'ascent, 'grammar, W: Write>
self.prefix,
loc_type
);
}

// identify the "end" location for this production;
// this is typically the end of the last symbol we are reducing,
// but in the case of an empty production it will come from the
// lookahead
if let Some(last_sym) = transfer_syms.last() {
rust!(self.out, "let {}end = {}.2.clone();", self.prefix, last_sym);
} else {
rust!(
self.out,
"let {}end = {}lookahead.as_ref().map(|o| o.0.clone()).unwrap_or_else(|| \
{}start.clone());",
self.prefix,
self.prefix,
self.prefix
);
rust!(self.out, "let {p}end = {p}start;", p = self.prefix);
}

let transfered_syms = transfer_syms.len();
Expand Down

0 comments on commit 11a50e7

Please sign in to comment.