From d6de2b582860c3e533c4420ce6249c7f9b753e87 Mon Sep 17 00:00:00 2001 From: AnotherTest Date: Sun, 28 Jun 2020 18:42:57 +0430 Subject: [PATCH] Shell: Show descriptions about syntax errors The description contains an error message and where in the source the error happened. --- Shell/AST.cpp | 78 ++++++++++++++++++++++++++++++++---------------- Shell/AST.h | 19 ++++++++++-- Shell/Forward.h | 1 + Shell/Parser.cpp | 29 ++++++++++-------- Shell/Shell.cpp | 6 ++-- 5 files changed, 90 insertions(+), 43 deletions(-) diff --git a/Shell/AST.cpp b/Shell/AST.cpp index 24c7ae3bef..cbb17463a7 100644 --- a/Shell/AST.cpp +++ b/Shell/AST.cpp @@ -174,8 +174,10 @@ And::And(Position position, RefPtr left, RefPtr right) , m_left(move(left)) , m_right(move(right)) { - if (m_left->is_syntax_error() || m_right->is_syntax_error()) - set_is_syntax_error(); + if (m_left->is_syntax_error()) + set_is_syntax_error(m_left->syntax_error_node()); + else if (m_right->is_syntax_error()) + set_is_syntax_error(m_right->syntax_error_node()); } And::~And() @@ -233,8 +235,10 @@ ListConcatenate::ListConcatenate(Position position, RefPtr element, RefPtr , m_element(move(element)) , m_list(move(list)) { - if (m_element->is_syntax_error() || m_list->is_syntax_error()) - set_is_syntax_error(); + if (m_element->is_syntax_error()) + set_is_syntax_error(m_element->syntax_error_node()); + else if (m_list->is_syntax_error()) + set_is_syntax_error(m_list->syntax_error_node()); } ListConcatenate::~ListConcatenate() @@ -274,7 +278,7 @@ Background::Background(Position position, RefPtr command) , m_command(move(command)) { if (m_command->is_syntax_error()) - set_is_syntax_error(); + set_is_syntax_error(m_command->syntax_error_node()); } Background::~Background() @@ -386,7 +390,7 @@ CastToCommand::CastToCommand(Position position, RefPtr inner) , m_inner(move(inner)) { if (m_inner->is_syntax_error()) - set_is_syntax_error(); + set_is_syntax_error(m_inner->syntax_error_node()); } CastToCommand::~CastToCommand() @@ -442,7 +446,7 @@ CastToList::CastToList(Position position, RefPtr inner) , m_inner(move(inner)) { if (m_inner && m_inner->is_syntax_error()) - set_is_syntax_error(); + set_is_syntax_error(m_inner->syntax_error_node()); } CastToList::~CastToList() @@ -565,7 +569,7 @@ DoubleQuotedString::DoubleQuotedString(Position position, RefPtr inner) , m_inner(move(inner)) { if (m_inner->is_syntax_error()) - set_is_syntax_error(); + set_is_syntax_error(m_inner->syntax_error_node()); } DoubleQuotedString::~DoubleQuotedString() @@ -613,7 +617,7 @@ DynamicEvaluate::DynamicEvaluate(Position position, RefPtr inner) , m_inner(move(inner)) { if (m_inner->is_syntax_error()) - set_is_syntax_error(); + set_is_syntax_error(m_inner->syntax_error_node()); } DynamicEvaluate::~DynamicEvaluate() @@ -865,7 +869,7 @@ Execute::Execute(Position position, RefPtr command, bool capture_stdout) , m_capture_stdout(capture_stdout) { if (m_command->is_syntax_error()) - set_is_syntax_error(); + set_is_syntax_error(m_command->syntax_error_node()); } Execute::~Execute() @@ -911,8 +915,10 @@ Join::Join(Position position, RefPtr left, RefPtr right) , m_left(move(left)) , m_right(move(right)) { - if (m_left->is_syntax_error() || m_right->is_syntax_error()) - set_is_syntax_error(); + if (m_left->is_syntax_error()) + set_is_syntax_error(m_left->syntax_error_node()); + else if (m_right->is_syntax_error()) + set_is_syntax_error(m_right->syntax_error_node()); } Join::~Join() @@ -969,8 +975,10 @@ Or::Or(Position position, RefPtr left, RefPtr right) , m_left(move(left)) , m_right(move(right)) { - if (m_left->is_syntax_error() || m_right->is_syntax_error()) - set_is_syntax_error(); + if (m_left->is_syntax_error()) + set_is_syntax_error(m_left->syntax_error_node()); + else if (m_right->is_syntax_error()) + set_is_syntax_error(m_right->syntax_error_node()); } Or::~Or() @@ -1030,8 +1038,10 @@ Pipe::Pipe(Position position, RefPtr left, RefPtr right) , m_left(move(left)) , m_right(move(right)) { - if (m_left->is_syntax_error() || m_right->is_syntax_error()) - set_is_syntax_error(); + if (m_left->is_syntax_error()) + set_is_syntax_error(m_left->syntax_error_node()); + else if (m_right->is_syntax_error()) + set_is_syntax_error(m_right->syntax_error_node()); } Pipe::~Pipe() @@ -1189,8 +1199,10 @@ Sequence::Sequence(Position position, RefPtr left, RefPtr right) , m_left(move(left)) , m_right(move(right)) { - if (m_left->is_syntax_error() || m_right->is_syntax_error()) - set_is_syntax_error(); + if (m_left->is_syntax_error()) + set_is_syntax_error(m_left->syntax_error_node()); + else if (m_right->is_syntax_error()) + set_is_syntax_error(m_right->syntax_error_node()); } Sequence::~Sequence() @@ -1408,8 +1420,10 @@ Juxtaposition::Juxtaposition(Position position, RefPtr left, RefPtr , m_left(move(left)) , m_right(move(right)) { - if (m_left->is_syntax_error() || m_right->is_syntax_error()) - set_is_syntax_error(); + if (m_left->is_syntax_error()) + set_is_syntax_error(m_left->syntax_error_node()); + else if (m_right->is_syntax_error()) + set_is_syntax_error(m_right->syntax_error_node()); } Juxtaposition::~Juxtaposition() @@ -1486,8 +1500,10 @@ StringPartCompose::StringPartCompose(Position position, RefPtr left, RefPt , m_left(move(left)) , m_right(move(right)) { - if (m_left->is_syntax_error() || m_right->is_syntax_error()) - set_is_syntax_error(); + if (m_left->is_syntax_error()) + set_is_syntax_error(m_left->syntax_error_node()); + else if (m_right->is_syntax_error()) + set_is_syntax_error(m_right->syntax_error_node()); } StringPartCompose::~StringPartCompose() @@ -1510,10 +1526,16 @@ void SyntaxError::highlight_in_editor(Line::Editor& editor, Shell&, HighlightMet editor.stylize({ m_position.start_offset, m_position.end_offset }, { Line::Style::Foreground(Line::Style::XtermColor::Red), Line::Style::Bold }); } -SyntaxError::SyntaxError(Position position) +SyntaxError::SyntaxError(Position position, String error) : Node(move(position)) + , m_syntax_error_text(move(error)) { - set_is_syntax_error(); + m_is_syntax_error = true; +} + +const SyntaxError& SyntaxError::syntax_error_node() const +{ + return *this; } SyntaxError::~SyntaxError() @@ -1692,8 +1714,12 @@ VariableDeclarations::VariableDeclarations(Position position, Vector v , m_variables(move(variables)) { for (auto& decl : m_variables) { - if (decl.name->is_syntax_error() || decl.value->is_syntax_error()) { - set_is_syntax_error(); + if (decl.name->is_syntax_error()) { + set_is_syntax_error(decl.name->syntax_error_node()); + break; + } + if (decl.value->is_syntax_error()) { + set_is_syntax_error(decl.value->syntax_error_node()); break; } } diff --git a/Shell/AST.h b/Shell/AST.h index 6484ed49cb..9426bd02ef 100644 --- a/Shell/AST.h +++ b/Shell/AST.h @@ -333,11 +333,21 @@ public: virtual bool would_execute() const { return false; } const Position& position() const { return m_position; } - void set_is_syntax_error() { m_is_syntax_error = true; } + void set_is_syntax_error(const SyntaxError& error_node) + { + m_is_syntax_error = true; + m_syntax_error_node = error_node; + } + virtual const SyntaxError& syntax_error_node() const + { + ASSERT(is_syntax_error()); + return *m_syntax_error_node; + } protected: Position m_position; bool m_is_syntax_error { false }; + RefPtr m_syntax_error_node; }; class PathRedirectionNode : public Node { @@ -766,9 +776,11 @@ private: class SyntaxError final : public Node { public: - SyntaxError(Position); + SyntaxError(Position, String); virtual ~SyntaxError(); + const String& error_text() const { return m_syntax_error_text; } + private: virtual void dump(int level) const override; virtual RefPtr run(RefPtr) override; @@ -776,6 +788,9 @@ private: virtual HitTestResult hit_test_position(size_t) override { return { nullptr, nullptr }; } virtual String class_name() const override { return "SyntaxError"; } virtual bool is_syntax_error() const override { return true; } + virtual const SyntaxError& syntax_error_node() const override; + + String m_syntax_error_text; }; class Tilde final : public Node { diff --git a/Shell/Forward.h b/Shell/Forward.h index 86a0ff0858..f54829b191 100644 --- a/Shell/Forward.h +++ b/Shell/Forward.h @@ -31,5 +31,6 @@ namespace AST { class Node; class Value; +class SyntaxError; } diff --git a/Shell/Parser.cpp b/Shell/Parser.cpp index 9d018f0cef..7e6f985e17 100644 --- a/Shell/Parser.cpp +++ b/Shell/Parser.cpp @@ -123,7 +123,7 @@ RefPtr Parser::parse() // Parsing stopped midway, this is a syntax error. auto error_start = push_start(); m_offset = m_input.length(); - return create(move(toplevel), create()); + return create(move(toplevel), create("Unexpected tokens past the end")); } return toplevel; @@ -234,7 +234,7 @@ RefPtr Parser::parse_variable_decls() if (!command) m_offset = start->offset; else if (!expect(')')) - command->set_is_syntax_error(); + command->set_is_syntax_error(*create("Expected a terminating close paren")); expression = command; } } @@ -341,7 +341,7 @@ RefPtr Parser::parse_redirection() // Eat a character and hope the problem goes away consume(); } - return create(); + return create("Expected a path"); } return create(pipe_fd, move(path)); // Redirection WriteAppend } @@ -363,7 +363,10 @@ RefPtr Parser::parse_redirection() ASSERT(fd.has_value()); dest_pipe_fd = fd.value(); } - return create(pipe_fd, dest_pipe_fd); // Redirection Fd2Fd + auto redir = create(pipe_fd, dest_pipe_fd); // Redirection Fd2Fd + if (dest_pipe_fd == -1) + redir->set_is_syntax_error(*create("Expected a file descriptor")); + return redir; } consume_while(is_whitespace); pipe_fd = pipe_fd >= 0 ? pipe_fd : STDOUT_FILENO; @@ -373,7 +376,7 @@ RefPtr Parser::parse_redirection() // Eat a character and hope the problem goes away consume(); } - return create(); + return create("Expected a path"); } return create(pipe_fd, move(path)); // Redirection Write } @@ -397,7 +400,7 @@ RefPtr Parser::parse_redirection() // Eat a character and hope the problem goes away consume(); } - return create(); + return create("Expected a path"); } if (mode == Read) return create(pipe_fd, move(path)); // Redirection Read @@ -530,10 +533,10 @@ RefPtr Parser::parse_string() consume(); auto inner = parse_doublequoted_string_inner(); if (!inner) - inner = create(); + inner = create("Unexpected EOF in string"); if (!expect('"')) { inner = create(move(inner)); - inner->set_is_syntax_error(); + inner->set_is_syntax_error(*create("Expected a terminating double quote")); return inner; } return create(move(inner)); // Double Quoted String @@ -547,7 +550,7 @@ RefPtr Parser::parse_string() is_error = true; auto result = create(move(text)); // String Literal if (is_error) - result->set_is_syntax_error(); + result->set_is_syntax_error(*create("Expected a terminating single quote")); return move(result); } @@ -682,15 +685,15 @@ RefPtr Parser::parse_evaluate() consume(); auto inner = parse_pipe_sequence(); if (!inner) - inner = create(); + inner = create("Unexpected EOF in list"); if (!expect(')')) - inner->set_is_syntax_error(); + inner->set_is_syntax_error(*create("Expected a terminating close paren")); return create(move(inner), true); } auto inner = parse_expression(); if (!inner) { - inner = create(); + inner = create("Expected a command"); } else { if (inner->is_list()) { auto execute_inner = create(move(inner), true); @@ -814,7 +817,7 @@ RefPtr Parser::parse_glob() } else { // FIXME: Allow composition of tilde+bareword with globs: '~/foo/bar/baz*' putback(); - bareword_part->set_is_syntax_error(); + bareword_part->set_is_syntax_error(*create(String::format("Unexpected %s inside a glob", bareword_part->class_name().characters()))); return bareword_part; } textbuilder.append(text); diff --git a/Shell/Shell.cpp b/Shell/Shell.cpp index 3bfbb0c84e..b04d134774 100644 --- a/Shell/Shell.cpp +++ b/Shell/Shell.cpp @@ -325,8 +325,10 @@ int Shell::run_command(const StringView& cmd) return 0; if (command->is_syntax_error()) { - // FIXME: Provide descriptive messages for syntax errors. - fprintf(stderr, "Shell: Syntax error in command\n"); + auto& error_node = command->syntax_error_node(); + auto& position = error_node.position(); + fprintf(stderr, "Shell: Syntax error in command: %s\n", error_node.error_text().characters()); + fprintf(stderr, "Around '%.*s'\n", (int)min(position.end_offset - position.start_offset, (size_t)10), cmd.characters_without_null_termination() + position.start_offset); return 1; }