h/fish-shell
1
Fork 0
mirror of https://github.com/fish-shell/fish-shell.git synced 2026-06-06 08:51:14 -03:00
Code Issues Packages Projects Releases Wiki Activity

ast: remove Node parent pointers

Browse Source 
tl;dr reduce memory usage and ast complexity. No functional change expected.

This concerns how our ast is represented. Prior to this commit, and ever
since the new ast was introduced, each Node had a reference to its parent -
the so-called "parent pointer". This made it convenient to "walk around"
the AST - for example we could get a command and then walk up the AST to
determine if that command had a decoration (`time`, etc).

This parent pointer concept was natural in C++ for a few reasons:

1. Pointers are idiomatic in C++.
2. Parent pointers were "thin": just regular pointer-sized, e.g.
   8 bytes in x86-64.
3. C++'s inheritance means that the pointer can just be stored as a field
   in the base Node class. Super easy and efficient.

But these proved to have significant drawbacks when expressed in Rust:

1. Parent pointers form a cyclic data structure which is very awkward in
   Rust. We had to use raw pointers and unsafe code.
2. Parent pointers were a `&dyn Node` which is necessarily "fat" (base
   pointer + vtable pointer), taking up 16 bytes on a 64 bit machine. This
   greatly bloated the size of the AST because our AST is quite fine (many
   detailed node types).
3. The lack of inheritance means that parent pointers had to be repeated
   for every node and exposed through the Node trait, which was awkward and
   verbose.

In fact storing parent pointers is rather uncommon among AST
implementations. For example, LLVM does not do this; instead it dynamically
constructs a map of parent pointers on demand (see `ParentMapContext`).
fish could do this or something like it, but in fact we can do better: we
can tweak Traversal to provide parent pointers.

As a reminder, Traversal is a way of naturally iterating over all AST nodes
in a for loop:

    for node in ast.walk() {...}

This is in-order ("top-down"). A parent node is yielded before its children.

Prior to this commit, this worked as follows: the Traversal maintained a
stack of next-to-be-yielded nodes. The `next()` function would pop off the
next node, push all of its children onto the stack, and then yield that
popped node.

We can easily make Traversal remember the sequence of parents of a given
Node by tweaking the Traversal to remember each Node after popping it. That
is, mark each Node in the stack as "NeedsVisit" or "Visited". Within
`next()`, check the status of the top node:

  - NeedsVisit => mark it as Visited, push its children, and return that Node
  - Visited => pop it, discard it, and try again

This means that, for any Node returned by `next()`, we can get the sequence
of parents as those Nodes that are marked as Visited on the stack.

The net effect of all of this is a large decrease in ast complexity and
memory usage. For example, __fish_complete_gpg.fish goes from 508 KB to 198 KB,
as measured by `fish_indent --dump-parse-tree`.

There's somewhat higher memory usage for Traversal, but these are transient
allocations, not permanent.
This commit is contained in:
Peter Ammon
2025-04-20 17:53:48 -07:00
parent 22d2dd6c90 e6bc8ffa13
commit 489d5d1733
13 changed files with 783 additions and 609 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
CHANGELOG.rst
View File

@@ -71,6 +71,7 @@ Other improvements
------------------
- ``fish_indent`` and ``fish_key_reader`` are now available as builtins, and if fish is called with that name it will act like the given tool (as a multi-call binary).
This allows truly distributing fish as a single file. (:issue:`10876`)
- ``fish_indent --dump-parse-tree`` now emits simple metrics about the tree including its memory consumption.
For distributors
----------------

586
src/ast.rs
View File

File diff suppressed because it is too large Load Diff

189
src/builtins/fish_indent.rs
View File

@@ -48,17 +48,20 @@
/// certain runs, weight line breaks, have a cost model, etc.
struct PrettyPrinter<'source, 'ast> {
/// The parsed ast.
ast: Ast,
ast: &'ast Ast,
state: PrettyPrinterState<'source, 'ast>,
}
struct PrettyPrinterState<'source, 'ast> {
/// Original source.
// Original source.
source: &'source wstr,
/// The indents of our string.
/// This has the same length as 'source' and describes the indentation level.
// The traversal of the ast.
traversal: Traversal<'ast>,
// The indents of our string.
// This has the same length as 'source' and describes the indentation level.
indents: Vec<i32>,
/// The prettifier output.
@@ -85,6 +88,60 @@ struct PrettyPrinterState<'source, 'ast> {
errors: Option<&'ast SourceRangeList>,
}
#[derive(Copy, Clone, Default, Debug)]
struct AstSizeMetrics {
/// The total number of nodes.
node_count: usize,
/// The number of branches, leaves, and lists, tokens, and keywords.
/// Note tokens and keywords are also counted as leaves.
branch_count: usize,
leaf_count: usize,
list_count: usize,
token_count: usize,
keyword_count: usize,
// An estimate of the total allocated size of the ast in bytes.
memory_size: usize,
}
impl std::fmt::Display for AstSizeMetrics {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
writeln!(f, "AstSizeMetrics:")?;
writeln!(f, " nodes: {}", self.node_count)?;
writeln!(f, " branches: {}", self.branch_count)?;
writeln!(f, " leaves: {}", self.leaf_count)?;
writeln!(f, " lists: {}", self.list_count)?;
writeln!(f, " tokens: {}", self.token_count)?;
writeln!(f, " keywords: {}", self.keyword_count)?;
let memsize = self.memory_size;
let (val, unit) = if memsize >= 1024 * 1024 {
(memsize as f64 / (1024.0 * 1024.0), "MB")
} else {
(memsize as f64 / 1024.0, "KB")
};
writeln!(f, " memory: {} bytes ({:.2} {})", memsize, val, unit)
}
}
impl<'a> NodeVisitor<'a> for AstSizeMetrics {
fn visit(&mut self, node: &'a dyn Node) {
self.node_count += 1;
self.memory_size += node.self_memory_size();
match node.category() {
Category::branch => self.branch_count += 1,
Category::leaf => self.leaf_count += 1,
Category::list => self.list_count += 1,
}
if node.as_token().is_some() {
self.token_count += 1;
}
if node.as_keyword().is_some() {
self.keyword_count += 1;
}
node.accept(self, false);
}
}
/// Flags we support.
#[derive(Copy, Clone, Default)]
struct GapFlags {
@@ -105,11 +162,13 @@ struct GapFlags {
}
impl<'source, 'ast> PrettyPrinter<'source, 'ast> {
fn new(source: &'source wstr, do_indent: bool) -> Self {
fn new(source: &'source wstr, ast: &'ast Ast, do_indent: bool) -> Self {
let traversal = Traversal::new(ast.top());
let mut zelf = Self {
ast: Ast::parse(source, parse_flags(), None),
ast,
state: PrettyPrinterState {
source,
traversal,
indents: if do_indent
/* Whether to indent, or just insert spaces. */
{
@@ -135,10 +194,10 @@ fn new(source: &'source wstr, do_indent: bool) -> Self {
}
// Entry point. Prettify our source code and return it.
fn prettify(&'ast mut self) -> WString {
fn prettify(&mut self) -> WString {
self.state.output.clear();
self.state.errors = Some(&self.ast.extras.errors);
self.state.visit(self.ast.top());
self.state.prettify_traversal();
// Trailing gap text.
self.state.emit_gap_text_before(
@@ -340,13 +399,13 @@ fn gap_text_flags_before_node(&self, node: &dyn Node) -> GapFlags {
ParseTokenType::string => {
// Allow escaped newlines before commands that follow a variable assignment
// since both can be long (#7955).
let p = node.parent().unwrap();
let p = self.traversal.parent(node);
if p.typ() != Type::decorated_statement {
return result;
}
let p = p.parent().unwrap();
let p = self.traversal.parent(p);
assert_eq!(p.typ(), Type::statement);
let p = p.parent().unwrap();
let p = self.traversal.parent(p);
if let Some(job) = p.as_job_pipeline() {
if !job.variables.is_empty() {
result.allow_escaped_newlines = true;
@@ -672,8 +731,8 @@ fn visit_semi_nl(&mut self, node: &dyn ast::Token) {
}
fn is_multi_line_brace(&self, node: &dyn ast::Token) -> bool {
node.parent()
.unwrap()
self.traversal
.parent(node.as_node())
.as_brace_statement()
.is_some_and(|brace_statement| {
self.multi_line_brace_statement_locations
@@ -750,11 +809,61 @@ fn visit_maybe_newlines(&mut self, node: &ast::MaybeNewlines) {
self.emit_gap_text(gap_range, flags);
}
fn visit_begin_header(&mut self) {
fn visit_begin_header(&mut self, node: &ast::BeginHeader) {
self.emit_node_text(&node.kw_begin);
if let Some(semi_nl) = &node.semi_nl {
self.visit_semi_nl(semi_nl);
}
// 'begin' does not require a newline after it, but we insert one.
if !self.at_line_start() {
self.emit_newline();
}
}
// Prettify our ast traversal, populating the output.
fn prettify_traversal(&mut self) {
while let Some(node) = self.traversal.next() {
// Leaf nodes we just visit their text.
if node.as_keyword().is_some() {
self.emit_node_text(node);
continue;
}
if let Some(token) = node.as_token() {
match token.token_type() {
ParseTokenType::end => self.visit_semi_nl(token),
ParseTokenType::left_brace => self.visit_left_brace(token),
ParseTokenType::right_brace => self.visit_right_brace(token),
_ => self.emit_node_text(node),
}
continue;
}
match node.typ() {
Type::argument | Type::variable_assignment => {
self.emit_node_text(node);
self.traversal.skip_children(node);
}
Type::redirection => {
self.visit_redirection(node.as_redirection().unwrap());
self.traversal.skip_children(node);
}
Type::maybe_newlines => {
self.visit_maybe_newlines(node.as_maybe_newlines().unwrap());
self.traversal.skip_children(node);
}
Type::begin_header => {
self.visit_begin_header(node.as_begin_header().unwrap());
self.traversal.skip_children(node);
}
_ => {
// For branch and list nodes, default is to visit their children.
if [Category::branch, Category::list].contains(&node.category()) {
continue;
}
panic!("unexpected node type");
}
}
}
}
}
// The flags we use to parse.
@@ -765,50 +874,6 @@ fn parse_flags() -> ParseTreeFlags {
| ParseTreeFlags::SHOW_BLANK_LINES
}
impl<'source, 'ast> NodeVisitor<'_> for PrettyPrinterState<'source, 'ast> {
// Default implementation is to just visit children.
fn visit(&mut self, node: &'_ dyn Node) {
// Leaf nodes we just visit their text.
if node.as_keyword().is_some() {
self.emit_node_text(node);
return;
}
if let Some(token) = node.as_token() {
match token.token_type() {
ParseTokenType::end => self.visit_semi_nl(token),
ParseTokenType::left_brace => self.visit_left_brace(token),
ParseTokenType::right_brace => self.visit_right_brace(token),
_ => self.emit_node_text(node),
}
return;
}
match node.typ() {
Type::argument | Type::variable_assignment => {
self.emit_node_text(node);
}
Type::redirection => {
self.visit_redirection(node.as_redirection().unwrap());
}
Type::maybe_newlines => {
self.visit_maybe_newlines(node.as_maybe_newlines().unwrap());
}
Type::begin_header => {
// 'begin' does not require a newline after it, but we insert one.
node.accept(self, false);
self.visit_begin_header();
}
_ => {
// For branch and list nodes, default is to visit their children.
if [Category::branch, Category::list].contains(&node.category()) {
node.accept(self, false);
return;
}
panic!("unexpected node type");
}
}
}
}
/// Return whether a character at a given index is escaped.
/// A character is escaped if it has an odd number of backslashes.
fn char_is_escaped(text: &wstr, idx: usize) -> bool {
@@ -1190,8 +1255,14 @@ fn prettify(streams: &mut IoStreams, src: &wstr, do_indent: bool) -> WString {
);
let ast_dump = ast.dump(src);
streams.err.appendln(ast_dump);
// Output metrics too.
let mut metrics = AstSizeMetrics::default();
metrics.visit(ast.top());
streams.err.appendln(format!("{}", metrics));
}
let mut printer = PrettyPrinter::new(src, do_indent);
let ast = Ast::parse(src, parse_flags(), None);
let mut printer = PrettyPrinter::new(src, &ast, do_indent);
printer.prettify()
}

57
src/highlight/highlight.rs
View File

@@ -2,7 +2,7 @@
use crate::abbrs::{self, with_abbrs};
use crate::ast::{
self, Argument, Ast, BlockStatement, BlockStatementHeaderVariant, BraceStatement,
DecoratedStatement, Keyword, Leaf, List, Node, NodeVisitor, Redirection, Token, Type,
DecoratedStatement, Keyword, List, Node, NodeVisitor, Redirection, Token, Type,
VariableAssignment,
};
use crate::builtins::shared::builtin_exists;
@@ -844,27 +844,27 @@ fn visit_children(&mut self, node: &dyn Node) {
fn visit_keyword(&mut self, node: &dyn Keyword) {
let mut role = HighlightRole::normal;
match node.keyword() {
ParseKeyword::kw_begin
| ParseKeyword::kw_builtin
| ParseKeyword::kw_case
| ParseKeyword::kw_command
| ParseKeyword::kw_else
| ParseKeyword::kw_end
| ParseKeyword::kw_exec
| ParseKeyword::kw_for
| ParseKeyword::kw_function
| ParseKeyword::kw_if
| ParseKeyword::kw_in
| ParseKeyword::kw_switch
| ParseKeyword::kw_while => role = HighlightRole::keyword,
ParseKeyword::kw_and
| ParseKeyword::kw_or
| ParseKeyword::kw_not
| ParseKeyword::kw_exclam
| ParseKeyword::kw_time => role = HighlightRole::operat,
ParseKeyword::none => (),
ParseKeyword::Begin
| ParseKeyword::Builtin
| ParseKeyword::Case
| ParseKeyword::Command
| ParseKeyword::Else
| ParseKeyword::End
| ParseKeyword::Exec
| ParseKeyword::For
| ParseKeyword::Function
| ParseKeyword::If
| ParseKeyword::In
| ParseKeyword::Switch
| ParseKeyword::While => role = HighlightRole::keyword,
ParseKeyword::And
| ParseKeyword::Or
| ParseKeyword::Not
| ParseKeyword::Exclam
| ParseKeyword::Time => role = HighlightRole::operat,
ParseKeyword::None => (),
};
self.color_node(node.leaf_as_node(), HighlightSpec::with_fg(role));
self.color_node(node.as_node(), HighlightSpec::with_fg(role));
}
fn visit_token(&mut self, tok: &dyn Token) {
let mut role = HighlightRole::normal;
@@ -884,11 +884,11 @@ fn visit_token(&mut self, tok: &dyn Token) {
}
_ => (),
}
self.color_node(tok.leaf_as_node(), HighlightSpec::with_fg(role));
self.color_node(tok.as_node(), HighlightSpec::with_fg(role));
}
// Visit an argument, perhaps knowing that our command is cd.
fn visit_argument(&mut self, arg: &Argument, cmd_is_cd: bool, options_allowed: bool) {
self.color_as_argument(arg.as_node(), options_allowed);
self.color_as_argument(arg, options_allowed);
if !self.io_still_ok() {
return;
}
@@ -912,7 +912,7 @@ fn visit_argument(&mut self, arg: &Argument, cmd_is_cd: bool, options_allowed: b
self.color_array[i].valid_path = true;
}
}
Err(..) => self.color_node(arg.as_node(), HighlightSpec::with_fg(HighlightRole::error)),
Err(..) => self.color_node(arg, HighlightSpec::with_fg(HighlightRole::error)),
}
}
@@ -926,10 +926,7 @@ fn visit_redirection(&mut self, redir: &Redirection) {
// It may have parsed successfully yet still be invalid (e.g. 9999999999999>&1)
// If so, color the whole thing invalid and stop.
if !oper.is_valid() {
self.color_node(
redir.as_node(),
HighlightSpec::with_fg(HighlightRole::error),
);
self.color_node(redir, HighlightSpec::with_fg(HighlightRole::error));
return;
}
@@ -943,7 +940,7 @@ fn visit_redirection(&mut self, redir: &Redirection) {
// Check if the argument contains a command substitution. If so, highlight it as a param
// even though it's a command redirection, and don't try to do any other validation.
if has_cmdsub(&target) {
self.color_as_argument(redir.target.leaf_as_node(), true);
self.color_as_argument(&redir.target, true);
return;
}
// No command substitution, so we can highlight the target file or fd. For example,
@@ -959,7 +956,7 @@ fn visit_redirection(&mut self, redir: &Redirection) {
self.file_tester.test_redirection_target(&target, oper.mode)
};
self.color_node(
redir.target.leaf_as_node(),
&redir.target,
HighlightSpec::with_fg(if target_is_valid {
HighlightRole::redirection
} else {

125
src/parse_constants.rs
View File

@@ -81,30 +81,29 @@ pub enum ParseTokenType {
comment,
}
#[repr(u8)]
#[derive(Clone, Copy, Debug, Eq, PartialEq)]
pub enum ParseKeyword {
// 'none' is not a keyword, it is a sentinel indicating nothing.
none,
kw_and,
kw_begin,
kw_builtin,
kw_case,
kw_command,
kw_else,
kw_end,
kw_exclam,
kw_exec,
kw_for,
kw_function,
kw_if,
kw_in,
kw_not,
kw_or,
kw_switch,
kw_time,
kw_while,
// 'None' is not a keyword, it is a sentinel indicating nothing.
// Note it proves convenient to keep this as a value rather than using Option.
None,
And,
Begin,
Builtin,
Case,
Command,
Else,
End,
Exclam,
Exec,
For,
Function,
If,
In,
Not,
Or,
Switch,
Time,
While,
}
// Statement decorations like 'command' or 'exec'.
@@ -224,7 +223,7 @@ pub fn to_wstr(self) -> &'static wstr {
impl Default for ParseKeyword {
fn default() -> Self {
ParseKeyword::none
ParseKeyword::None
}
}
@@ -232,24 +231,24 @@ impl ParseKeyword {
/// Return the keyword as a string.
pub fn to_wstr(self) -> &'static wstr {
match self {
ParseKeyword::kw_and => L!("and"),
ParseKeyword::kw_begin => L!("begin"),
ParseKeyword::kw_builtin => L!("builtin"),
ParseKeyword::kw_case => L!("case"),
ParseKeyword::kw_command => L!("command"),
ParseKeyword::kw_else => L!("else"),
ParseKeyword::kw_end => L!("end"),
ParseKeyword::kw_exclam => L!("!"),
ParseKeyword::kw_exec => L!("exec"),
ParseKeyword::kw_for => L!("for"),
ParseKeyword::kw_function => L!("function"),
ParseKeyword::kw_if => L!("if"),
ParseKeyword::kw_in => L!("in"),
ParseKeyword::kw_not => L!("not"),
ParseKeyword::kw_or => L!("or"),
ParseKeyword::kw_switch => L!("switch"),
ParseKeyword::kw_time => L!("time"),
ParseKeyword::kw_while => L!("while"),
ParseKeyword::And => L!("and"),
ParseKeyword::Begin => L!("begin"),
ParseKeyword::Builtin => L!("builtin"),
ParseKeyword::Case => L!("case"),
ParseKeyword::Command => L!("command"),
ParseKeyword::Else => L!("else"),
ParseKeyword::End => L!("end"),
ParseKeyword::Exclam => L!("!"),
ParseKeyword::Exec => L!("exec"),
ParseKeyword::For => L!("for"),
ParseKeyword::Function => L!("function"),
ParseKeyword::If => L!("if"),
ParseKeyword::In => L!("in"),
ParseKeyword::Not => L!("not"),
ParseKeyword::Or => L!("or"),
ParseKeyword::Switch => L!("switch"),
ParseKeyword::Time => L!("time"),
ParseKeyword::While => L!("while"),
_ => L!("unknown_keyword"),
}
}
@@ -263,26 +262,28 @@ fn to_arg(self) -> fish_printf::Arg<'static> {
impl From<&wstr> for ParseKeyword {
fn from(s: &wstr) -> Self {
match s {
_ if s == "!" => ParseKeyword::kw_exclam,
_ if s == "and" => ParseKeyword::kw_and,
_ if s == "begin" => ParseKeyword::kw_begin,
_ if s == "builtin" => ParseKeyword::kw_builtin,
_ if s == "case" => ParseKeyword::kw_case,
_ if s == "command" => ParseKeyword::kw_command,
_ if s == "else" => ParseKeyword::kw_else,
_ if s == "end" => ParseKeyword::kw_end,
_ if s == "exec" => ParseKeyword::kw_exec,
_ if s == "for" => ParseKeyword::kw_for,
_ if s == "function" => ParseKeyword::kw_function,
_ if s == "if" => ParseKeyword::kw_if,
_ if s == "in" => ParseKeyword::kw_in,
_ if s == "not" => ParseKeyword::kw_not,
_ if s == "or" => ParseKeyword::kw_or,
_ if s == "switch" => ParseKeyword::kw_switch,
_ if s == "time" => ParseKeyword::kw_time,
_ if s == "while" => ParseKeyword::kw_while,
_ => ParseKeyword::none,
// Note this is called in hot loops.
let c0 = s.as_char_slice().get(0).copied().unwrap_or('\0');
match c0 {
'!' if s == L!("!") => ParseKeyword::Exclam,
'a' if s == L!("and") => ParseKeyword::And,
'b' if s == L!("begin") => ParseKeyword::Begin,
'b' if s == L!("builtin") => ParseKeyword::Builtin,
'c' if s == L!("case") => ParseKeyword::Case,
'c' if s == L!("command") => ParseKeyword::Command,
'e' if s == L!("else") => ParseKeyword::Else,
'e' if s == L!("end") => ParseKeyword::End,
'e' if s == L!("exec") => ParseKeyword::Exec,
'f' if s == L!("for") => ParseKeyword::For,
'f' if s == L!("function") => ParseKeyword::Function,
'i' if s == L!("if") => ParseKeyword::If,
'i' if s == L!("in") => ParseKeyword::In,
'n' if s == L!("not") => ParseKeyword::Not,
'o' if s == L!("or") => ParseKeyword::Or,
's' if s == L!("switch") => ParseKeyword::Switch,
't' if s == L!("time") => ParseKeyword::Time,
'w' if s == L!("while") => ParseKeyword::While,
_ => ParseKeyword::None,
}
}
}
@@ -423,7 +424,7 @@ pub fn token_type_user_presentable_description(
type_: ParseTokenType,
keyword: ParseKeyword,
) -> WString {
if keyword != ParseKeyword::none {
if keyword != ParseKeyword::None {
return sprintf!("keyword: '%ls'", keyword.to_wstr());
}
match type_ {

8
src/parse_execution.rs
View File

@@ -611,7 +611,7 @@ fn apply_variable_assignments(
}
*block = Some(ctx.parser().push_block(Block::variable_assignment_block()));
for variable_assignment in variable_assignment_list {
let source = self.node_source(&**variable_assignment);
let source = self.node_source(variable_assignment);
let equals_pos = variable_assignment_equals_pos(source).unwrap();
let variable_name = &source[..equals_pos];
let expression = &source[equals_pos + 1..];
@@ -1347,7 +1347,7 @@ fn run_begin_statement(
fn get_argument_nodes(args: &ast::ArgumentList) -> AstArgsList<'_> {
let mut result = AstArgsList::new();
for arg in args {
result.push(&**arg);
result.push(arg);
}
result
}
@@ -1725,11 +1725,11 @@ fn test_and_run_1_job_conjunction(
if let Some(deco) = &jc.decorator {
let last_status = ctx.parser().get_last_status();
match deco.keyword() {
ParseKeyword::kw_and => {
ParseKeyword::And => {
// AND. Skip if the last job failed.
skip = last_status != 0;
}
ParseKeyword::kw_or => {
ParseKeyword::Or => {
// OR. Skip if the last job succeeded.
skip = last_status == 0;
}

4
src/parse_tree.rs
View File

@@ -39,7 +39,7 @@ impl ParseToken {
pub fn new(typ: ParseTokenType) -> Self {
ParseToken {
typ,
keyword: ParseKeyword::none,
keyword: ParseKeyword::None,
has_dash_prefix: false,
is_help_argument: false,
is_newline: false,
@@ -73,7 +73,7 @@ pub fn is_dash_prefix_string(&self) -> bool {
/// Returns a string description of the given parse token.
pub fn describe(&self) -> WString {
let mut result = self.typ.to_wstr().to_owned();
if self.keyword != ParseKeyword::none {
if self.keyword != ParseKeyword::None {
sprintf!(=> &mut result, " <%ls>", self.keyword.to_wstr())
}
result

133
src/parse_util.rs
View File

@@ -1,5 +1,7 @@
//! Various mostly unrelated utility functions related to parsing, loading and evaluating fish code.
use crate::ast::{self, Ast, Keyword, Leaf, List, Node, NodeVisitor, Token};
use crate::ast::{
self, is_same_node, Ast, Keyword, Leaf, List, Node, NodeVisitor, Token, Traversal,
};
use crate::builtins::shared::builtin_exists;
use crate::common::{
escape_string, unescape_string, valid_var_name, valid_var_name_char, EscapeFlags,
@@ -825,7 +827,10 @@ pub fn apply_indents(src: &wstr, indents: &[i32]) -> WString {
// Visit all of our nodes. When we get a job_list or case_item_list, increment indent while
// visiting its children.
struct IndentVisitor<'a> {
// companion: Pin<&'a mut indent_visitor_t>,
// The parent node of the node we are currently visiting, or None if we are the root.
parent: Option<&'a dyn ast::Node>,
// companion: Pin<&'a mut IndentVisitor>,
// The one-past-the-last index of the most recently encountered leaf node.
// We use this to populate the indents even if there's no tokens in the range.
last_leaf_end: usize,
@@ -849,9 +854,11 @@ struct IndentVisitor<'a> {
// List of locations of escaped newline characters.
line_continuations: Vec<usize>,
}
impl<'a> IndentVisitor<'a> {
fn new(src: &'a wstr, indents: &'a mut Vec<i32>, initial_indent: i32) -> Self {
Self {
parent: None,
last_leaf_end: 0,
last_indent: initial_indent - 1,
unclosed: false,
@@ -972,6 +979,7 @@ fn indent_string_part(&mut self, range: Range<usize>, is_double_quoted: bool) {
}
}
}
impl<'a> NodeVisitor<'a> for IndentVisitor<'a> {
// Default implementation is to just visit children.
fn visit(&mut self, node: &'a dyn Node) {
@@ -987,13 +995,14 @@ fn visit(&mut self, node: &'a dyn Node) {
// Increment indents for conditions in headers (#1665).
Type::job_conjunction => {
if [Type::while_header, Type::if_clause].contains(&node.parent().unwrap().typ()) {
let typ = self.parent.unwrap().typ();
if matches!(typ, Type::if_clause | Type::while_header) {
inc = 1;
dec = 1;
}
}
// Increment indents for job_continuation_t if it contains a newline.
// Increment indents for JobContinuation if it contains a newline.
// This is a bit of a hack - it indents cases like:
// cmd1 |
// ....cmd2
@@ -1036,12 +1045,12 @@ fn visit(&mut self, node: &'a dyn Node) {
// To address this, if we see that the switch statement was not closed, do not
// decrement the indent afterwards.
inc = 1;
let switchs = node.parent().unwrap().as_switch_statement().unwrap();
let switchs = self.parent.unwrap().as_switch_statement().unwrap();
dec = if switchs.end.has_source() { 1 } else { 0 };
}
Type::token_base => {
let token_type = node.as_token().unwrap().token_type();
let parent_type = node.parent().unwrap().typ();
let parent_type = self.parent.unwrap().typ();
if parent_type == Type::begin_header && token_type == ParseTokenType::end {
// The newline after "begin" is optional, so it is part of the header.
// The header is not in the indented block, so indent the newline here.
@@ -1091,7 +1100,9 @@ fn visit(&mut self, node: &'a dyn Node) {
self.last_indent = self.indent;
}
let saved = self.parent.replace(node);
node.accept(self, false);
self.parent = saved;
self.indent -= dec;
}
}
@@ -1185,7 +1196,8 @@ pub fn parse_util_detect_errors_in_ast(
// Verify return only within a function.
// Verify no variable expansions.
for node in ast::Traversal::new(ast.top()) {
let mut traversal = ast::Traversal::new(ast.top());
while let Some(node) = traversal.next() {
if let Some(jc) = node.as_job_continuation() {
// Somewhat clumsy way of checking for a statement without source in a pipeline.
// See if our pipe has source but our statement does not.
@@ -1214,23 +1226,28 @@ pub fn parse_util_detect_errors_in_ast(
// while foo & ; end
// If it's not a background job, nothing to do.
if job.bg.is_some() {
errored |= detect_errors_in_backgrounded_job(job, &mut out_errors);
errored |= detect_errors_in_backgrounded_job(&traversal, job, &mut out_errors);
}
} else if let Some(stmt) = node.as_decorated_statement() {
errored |= detect_errors_in_decorated_statement(buff_src, stmt, &mut out_errors);
errored |=
detect_errors_in_decorated_statement(buff_src, &traversal, stmt, &mut out_errors);
} else if let Some(block) = node.as_block_statement() {
// If our 'end' had no source, we are unsourced.
if !block.end.has_source() {
has_unclosed_block = true;
}
errored |=
detect_errors_in_block_redirection_list(&block.args_or_redirs, &mut out_errors);
errored |= detect_errors_in_block_redirection_list(
node,
&block.args_or_redirs,
&mut out_errors,
);
} else if let Some(brace_statement) = node.as_brace_statement() {
// If our closing brace had no source, we are unsourced.
if !brace_statement.right_brace.has_source() {
has_unclosed_block = true;
}
errored |= detect_errors_in_block_redirection_list(
node,
&brace_statement.args_or_redirs,
&mut out_errors,
);
@@ -1240,14 +1257,17 @@ pub fn parse_util_detect_errors_in_ast(
has_unclosed_block = true;
}
errored |=
detect_errors_in_block_redirection_list(&ifs.args_or_redirs, &mut out_errors);
detect_errors_in_block_redirection_list(node, &ifs.args_or_redirs, &mut out_errors);
} else if let Some(switchs) = node.as_switch_statement() {
// If our 'end' had no source, we are unsourced.
if !switchs.end.has_source() {
has_unclosed_block = true;
}
errored |=
detect_errors_in_block_redirection_list(&switchs.args_or_redirs, &mut out_errors);
errored |= detect_errors_in_block_redirection_list(
node,
&switchs.args_or_redirs,
&mut out_errors,
);
}
}
@@ -1505,8 +1525,9 @@ fn detect_errors_in_job_conjunction(
false
}
/// Given that the job given by node should be backgrounded, return true if we detect any errors.
/// Given that the job should be backgrounded, return true if we detect any errors.
fn detect_errors_in_backgrounded_job(
traversal: &Traversal,
job: &ast::JobPipeline,
parse_errors: &mut Option<&mut ParseErrorList>,
) -> bool {
@@ -1520,35 +1541,34 @@ fn detect_errors_in_backgrounded_job(
// foo & ; or bar
// if foo & ; end
// while foo & ; end
let Some(job_conj) = job.parent().unwrap().as_job_conjunction() else {
let Some(job_conj) = traversal.parent(job).as_job_conjunction() else {
return false;
};
if job_conj.parent().unwrap().as_if_clause().is_some()
|| job_conj.parent().unwrap().as_while_header().is_some()
{
let job_conj_parent = traversal.parent(job_conj);
if job_conj_parent.as_if_clause().is_some() || job_conj_parent.as_while_header().is_some() {
errored = append_syntax_error!(
parse_errors,
source_range.start(),
source_range.length(),
BACKGROUND_IN_CONDITIONAL_ERROR_MSG
);
} else if let Some(jlist) = job_conj.parent().unwrap().as_job_list() {
} else if let Some(jlist) = job_conj_parent.as_job_list() {
// This isn't very complete, e.g. we don't catch 'foo & ; not and bar'.
// Find the index of ourselves in the job list.
let index = jlist
.iter()
.position(|job| job.pointer_eq(job_conj))
.position(|job| is_same_node(job, job_conj))
.expect("Should have found the job in the list");
// Try getting the next job and check its decorator.
if let Some(next) = jlist.get(index + 1) {
if let Some(deco) = &next.decorator {
assert!(
[ParseKeyword::kw_and, ParseKeyword::kw_or].contains(&deco.keyword()),
[ParseKeyword::And, ParseKeyword::Or].contains(&deco.keyword()),
"Unexpected decorator keyword"
);
let deco_name = if deco.keyword() == ParseKeyword::kw_and {
let deco_name = if deco.keyword() == ParseKeyword::And {
L!("and")
} else {
L!("or")
@@ -1567,9 +1587,10 @@ fn detect_errors_in_backgrounded_job(
}
/// Given a source buffer `buff_src` and decorated statement `dst` within it, return true if there
/// is an error and false if not. `storage` may be used to reduce allocations.
/// is an error and false if not.
fn detect_errors_in_decorated_statement(
buff_src: &wstr,
traversal: &ast::Traversal,
dst: &ast::DecoratedStatement,
parse_errors: &mut Option<&mut ParseErrorList>,
) -> bool {
@@ -1586,22 +1607,18 @@ fn detect_errors_in_decorated_statement(
}
// Get the statement we are part of.
let st = dst.parent().unwrap().as_statement().unwrap();
let st = traversal.parent(dst).as_statement().unwrap();
// Walk up to the job.
let mut job = None;
let mut cursor = dst.parent();
while job.is_none() {
let c = cursor.expect("Reached root without finding a job");
job = c.as_job_pipeline();
cursor = c.parent();
}
let job = job.expect("Should have found the job");
let job = traversal
.parent_nodes()
.find_map(|n| n.as_job_pipeline())
.expect("Should have found the job");
// Check our pipeline position.
let pipe_pos = if job.continuation.is_empty() {
PipelinePosition::none
} else if job.statement.pointer_eq(st) {
} else if is_same_node(&job.statement, st) {
PipelinePosition::first
} else {
PipelinePosition::subsequent
@@ -1700,30 +1717,31 @@ fn detect_errors_in_decorated_statement(
}
// Check that we don't break or continue from outside a loop.
if !errored && [L!("break"), L!("continue")].contains(&&command[..]) && !first_arg_is_help {
if !errored && (command == "break" || command == "continue") && !first_arg_is_help {
// Walk up until we hit a 'for' or 'while' loop. If we hit a function first,
// stop the search; we can't break an outer loop from inside a function.
// This is a little funny because we can't tell if it's a 'for' or 'while'
// loop from the ancestor alone; we need the header. That is, we hit a
// block_statement, and have to check its header.
let mut found_loop = false;
let mut ancestor: Option<&dyn Node> = Some(dst);
while let Some(anc) = ancestor {
if let Some(block) = anc.as_block_statement() {
if [ast::Type::for_header, ast::Type::while_header]
.contains(&block.header.typ())
{
for block in traversal
.parent_nodes()
.filter_map(|anc| anc.as_block_statement())
{
match block.header.typ() {
ast::Type::for_header | ast::Type::while_header => {
// This is a loop header, so we can break or continue.
found_loop = true;
break;
} else if block.header.typ() == ast::Type::function_header {
}
ast::Type::function_header => {
// This is a function header, so we cannot break or
// continue. We stop our search here.
found_loop = false;
break;
}
_ => {}
}
ancestor = anc.parent();
}
if !found_loop {
@@ -1779,34 +1797,23 @@ fn detect_errors_in_decorated_statement(
errored
}
// Given we have a trailing argument_or_redirection_list, like `begin; end > /dev/null`, verify that
// there are no arguments in the list.
// Given we have a trailing ArgumentOrRedirectionList, like `begin; end > /dev/null`, verify that
// there are no arguments in the list. The parent of the list is provided.
fn detect_errors_in_block_redirection_list(
parent: &dyn Node,
args_or_redirs: &ast::ArgumentOrRedirectionList,
out_errors: &mut Option<&mut ParseErrorList>,
) -> bool {
let Some(first_arg) = get_first_arg(args_or_redirs) else {
return false;
};
if args_or_redirs
.parent()
.unwrap()
.as_brace_statement()
.is_some()
{
return append_syntax_error!(
out_errors,
first_arg.source_range().start(),
first_arg.source_range().length(),
RIGHT_BRACE_ARG_ERR_MSG
);
let r = first_arg.source_range();
if parent.as_brace_statement().is_some() {
append_syntax_error!(out_errors, r.start(), r.length(), RIGHT_BRACE_ARG_ERR_MSG);
} else {
append_syntax_error!(out_errors, r.start(), r.length(), END_ARG_ERR_MSG);
}
append_syntax_error!(
out_errors,
first_arg.source_range().start(),
first_arg.source_range().length(),
END_ARG_ERR_MSG
)
true
}
/// Given a string containing a variable expansion error, append an appropriate error to the errors

29
src/reader.rs
View File

@@ -42,7 +42,7 @@
use errno::{errno, Errno};
use crate::abbrs::abbrs_match;
use crate::ast::{self, Ast, Category, Traversal};
use crate::ast::{is_same_node, Ast, Category};
use crate::builtins::shared::ErrorCode;
use crate::builtins::shared::STATUS_CMD_ERROR;
use crate::builtins::shared::STATUS_CMD_OK;
@@ -5352,22 +5352,9 @@ fn extract_tokens(s: &wstr) -> Vec<PositionedToken> {
| ParseTreeFlags::LEAVE_UNTERMINATED;
let ast = Ast::parse(s, ast_flags, None);
// Helper to check if a node is the command portion of a decorated statement.
let is_command = |node: &dyn ast::Node| {
let mut cursor = Some(node);
while let Some(cur) = cursor {
if let Some(stmt) = cur.as_decorated_statement() {
if node.pointer_eq(&stmt.command) {
return true;
}
}
cursor = cur.parent();
}
false
};
let mut result = vec![];
for node in Traversal::new(ast.top()) {
let mut traversal = ast.walk();
while let Some(node) = traversal.next() {
// We are only interested in leaf nodes with source.
if node.category() != Category::leaf {
continue;
@@ -5404,10 +5391,12 @@ fn extract_tokens(s: &wstr) -> Vec<PositionedToken> {
if !has_cmd_subs {
// Common case of no command substitutions in this leaf node.
result.push(PositionedToken {
range,
is_cmd: is_command(node),
})
// Check if a node is the command portion of a decorated statement.
let is_cmd = traversal
.parent(node)
.as_decorated_statement()
.is_some_and(|stmt| is_same_node(node, &stmt.command));
result.push(PositionedToken { range, is_cmd })
}
}

46
src/tests/ast.rs Normal file
View File

@@ -0,0 +1,46 @@
use crate::ast::{is_same_node, Ast, Node};
use crate::wchar::prelude::*;
const FISH_FUNC: &str = r#"
function stuff --description 'Stuff'
set -l log "/tmp/chaos_log.(random)"
set -x PATH /custom/bin $PATH
echo "[$USER] Hooray" | tee -a $log 2>/dev/null
time if test (count $argv) -eq 0
echo "No targets specified" >> $log 2>&1
return 1
end
for target in $argv
command bash -c "echo" >> $log 2> /dev/null
switch $status
case 0
echo "Success" | tee -a $log
case '*'
echo "Failure" >> $log
end
end
set_color green
end
"#;
#[test]
fn test_is_same_node() {
// is_same_node is pretty subtle! Let's check it.
let src = WString::from_str(FISH_FUNC);
let ast = Ast::parse(&src, Default::default(), None);
assert!(!ast.errored());
let all_nodes: Vec<&dyn Node> = ast.walk().collect();
for i in 0..all_nodes.len() {
for j in 0..all_nodes.len() {
let same = is_same_node(all_nodes[i], all_nodes[j]);
if i == j {
assert!(same, "Node {} should be the same as itself", i);
} else {
assert!(!same, "Node {} should not be the same as node {}", i, j);
}
}
}
}

45
src/tests/ast_bench.rs Normal file
View File

@@ -0,0 +1,45 @@
// Run with cargo +nightly bench --features=benchmark
#[cfg(feature = "benchmark")]
mod bench {
extern crate test;
use crate::ast::Ast;
use crate::wchar::prelude::*;
use test::Bencher;
// Return a long string suitable for benchmarking.
fn generate_fish_script() -> WString {
let mut buff = WString::new();
let s = &mut buff;
for i in 0..1000 {
// command with args and redirections
sprintf!(=> s,
"echo arg%d arg%d > out%d.txt 2> err%d.txt\n",
i, i + 1, i, i
);
// simple block
sprintf!(=> s, "begin\n echo inside block %d\nend\n", i );
// conditional
sprintf!(=> s, "if test %d\n echo even\nelse\n echo odd\nend\n", i % 2);
// loop
sprintf!(=> s, "for x in a b c\n echo $x %d\nend\n", i);
// pipeline
sprintf!(=> s, "echo foo%d | grep f | wc -l\n", i);
}
buff
}
#[bench]
fn bench_ast_construction(b: &mut Bencher) {
let src = generate_fish_script();
b.bytes = (src.len() * 4) as u64; // 4 bytes per character
b.iter(|| {
let _ast = Ast::parse(&src, Default::default(), None);
});
}
}

2
src/tests/mod.rs
View File

@@ -1,4 +1,6 @@
mod abbrs;
mod ast;
mod ast_bench;
mod common;
mod complete;
mod debounce;

167
src/tests/parser.rs
View File

@@ -1,10 +1,10 @@
use crate::ast::{self, Ast, JobPipeline, List, Node, Traversal};
use crate::ast::{self, is_same_node, Ast, JobPipeline, List, Node, Traversal};
use crate::common::ScopeGuard;
use crate::env::EnvStack;
use crate::expand::ExpandFlags;
use crate::io::{IoBufferfill, IoChain};
use crate::parse_constants::{
ParseErrorCode, ParseTreeFlags, ParserTestErrorBits, StatementDecoration,
ParseErrorCode, ParseTokenType, ParseTreeFlags, ParserTestErrorBits, StatementDecoration,
};
use crate::parse_tree::{parse_source, LineCounter};
use crate::parse_util::{parse_util_detect_errors, parse_util_detect_errors_in_argument};
@@ -818,3 +818,166 @@ fn test_line_counter_empty() {
assert_eq!(line_counter.line_offset_of_node(), None);
assert_eq!(line_counter.source_offset_of_node(), None);
}
// Helper for testing a simple ast traversal.
// The ast is always for the command 'true;'.
struct TrueSemiAstTester<'a> {
// The AST we are testing.
ast: &'a Ast,
// Expected parent-child relationships, in the order we expect to encounter them.
parent_child: Box<[(&'a dyn Node, &'a dyn Node)]>,
}
impl<'a> TrueSemiAstTester<'a> {
const TRUE_SEMI: &'static wstr = L!("true;");
fn new(ast: &'a Ast) -> Self {
let job_list = ast.top().as_job_list().expect("Expected job_list");
let job_conjunction = &job_list[0];
let job_pipeline = &job_conjunction.job;
let variable_assignment_list = &job_pipeline.variables;
let statement = &job_pipeline.statement;
let decorated_statement = statement
.contents
.as_decorated_statement()
.expect("Expected decorated_statement");
let command = &decorated_statement.command;
let args_or_redirs = &decorated_statement.args_or_redirs;
let job_continuation = &job_pipeline.continuation;
let job_conjunction_continuation = &job_conjunction.continuations;
let semi_nl = job_conjunction.semi_nl.as_ref().expect("Expected semi_nl");
// Helpful parent-child map, such that the children are in the order that we expect to encounter them
// in the AST.
let parent_child: &[(&'a dyn Node, &'a dyn Node)] = &[
(job_list, job_conjunction),
(job_conjunction, job_pipeline),
(job_pipeline, variable_assignment_list),
(job_pipeline, statement),
(statement, decorated_statement),
(decorated_statement, command),
(decorated_statement, args_or_redirs),
(job_pipeline, job_continuation),
(job_conjunction, job_conjunction_continuation),
(job_conjunction, semi_nl),
];
Self {
ast,
parent_child: Box::from(parent_child),
}
}
// Expected nodes, in-order.
fn expected_nodes(&self) -> Vec<&'a dyn Node> {
let mut expected: Vec<&dyn Node> = vec![self.ast.top()];
expected.extend(self.parent_child.iter().map(|&(_p, c)| c));
expected
}
// Helper function to construct the parent list of a given node, such at the first entry is
// the node itself, and the last entry is the root node.
fn get_parents<'s>(&'s self, node: &'a dyn Node) -> impl Iterator<Item = &'a dyn Node> + 's {
let mut next = Some(node);
std::iter::from_fn(move || {
let out = next?;
next = self
.parent_child
.iter()
.find_map(|&(p, c)| is_same_node(c, out).then_some(p));
Some(out)
})
}
}
#[test]
fn test_ast() {
// Light testing of the AST and traversals.
let ast = Ast::parse(TrueSemiAstTester::TRUE_SEMI, ParseTreeFlags::empty(), None);
let tester = TrueSemiAstTester::new(&ast);
assert!(ast.top().as_job_list().is_some(), "Expected job_list");
// Walk the AST and collect all nodes.
// See is_same_node comments for why we can't use assert_eq! here.
let found = ast.walk().collect::<Vec<_>>();
let expected = tester.expected_nodes();
assert_eq!(found.len(), expected.len());
for idx in 0..found.len() {
assert!(is_same_node(found[idx], expected[idx]));
}
// Walk and check parents.
let mut traversal = ast.walk();
while let Some(node) = traversal.next() {
let expected_parents = tester.get_parents(node).collect::<Vec<_>>();
let found_parents = traversal.parent_nodes().collect::<Vec<_>>();
assert_eq!(found_parents.len(), expected_parents.len());
for idx in 0..found_parents.len() {
assert!(is_same_node(found_parents[idx], expected_parents[idx]));
}
}
// Find the decorated statement.
let decorated_statement = ast
.walk()
.find(|n| n.typ() == ast::Type::decorated_statement)
.expect("Expected decorated statement");
// Test the skip feature. Don't descend into the decorated_statement.
let expected_skip: Vec<&dyn Node> = expected
.iter()
.copied()
.filter(|&n| {
// Discard nodes who have the decorated_statement as a parent,
// excepting the decorated_statement itself.
tester
.get_parents(n)
.skip(1)
.all(|p| !is_same_node(p, decorated_statement))
})
.collect();
let mut found = vec![];
let mut traversal = ast.walk();
while let Some(node) = traversal.next() {
if is_same_node(node, decorated_statement) {
traversal.skip_children(node);
}
found.push(node);
}
assert_eq!(found.len(), expected_skip.len());
for idx in 0..found.len() {
assert!(is_same_node(found[idx], expected_skip[idx]));
}
}
#[test]
#[should_panic]
fn test_traversal_skip_children_panics() {
// Test that we panic if we try to skip children of a node that is not the current node.
let ast = Ast::parse(L!("true;"), ParseTreeFlags::empty(), None);
let mut traversal = ast.walk();
while let Some(node) = traversal.next() {
if node.typ() == ast::Type::decorated_statement {
// Should panic as we can only skip the current node.
traversal.skip_children(ast.top());
}
}
}
#[test]
#[should_panic]
fn test_traversal_parent_panics() {
// Can only get the parent of nodes still on the stack.
let ast = Ast::parse(L!("true;"), ParseTreeFlags::empty(), None);
let mut traversal = ast.walk();
let mut decorated_statement = None;
while let Some(node) = traversal.next() {
if node.as_decorated_statement().is_some() {
decorated_statement = Some(node);
} else if node.as_token().map(|t| t.token_type()) == Some(ParseTokenType::end) {
// should panic as the decorated_statement is not on the stack.
let _ = traversal.parent(decorated_statement.unwrap());
}
}
}