Skip to content

Commit

Permalink
[ruby/prism] Implement the void statement warning
Browse files Browse the repository at this point in the history 
ruby/prism@802ff71cd4
  • Loading branch information
@kddnewton @matzbot
kddnewton authored and matzbot committed Apr 12, 2024
1 parent 0924ff2 commit 4fc457e
Show file tree
Hide file tree
Showing 4 changed files with 234 additions and 8 deletions.
1 change: 1 addition & 0 deletions prism/config.yml
View file
Expand Up @@ -270,6 +270,7 @@ warnings:
- SHEBANG_CARRIAGE_RETURN
- UNEXPECTED_CARRIAGE_RETURN
- UNUSED_LOCAL_VARIABLE
- VOID_STATEMENT
tokens:
- name: EOF
value: 1
Expand Down
166 changes: 164 additions & 2 deletions prism/prism.c
View file
Expand Up @@ -1182,6 +1182,158 @@ pm_assert_value_expression(pm_parser_t *parser, pm_node_t *node) {
}
}

/**
* Warn if the given node is a "void" statement.
*/
static void
pm_void_statement_check(pm_parser_t *parser, const pm_node_t *node) {
const char *type = NULL;
int length = 0;

switch (PM_NODE_TYPE(node)) {
case PM_BACK_REFERENCE_READ_NODE:
case PM_CLASS_VARIABLE_READ_NODE:
case PM_GLOBAL_VARIABLE_READ_NODE:
case PM_INSTANCE_VARIABLE_READ_NODE:
case PM_LOCAL_VARIABLE_READ_NODE:
case PM_NUMBERED_REFERENCE_READ_NODE:
type = "a variable";
length = 10;
break;
case PM_CALL_NODE: {
const pm_call_node_t *cast = (const pm_call_node_t *) node;
if (cast->call_operator_loc.start != NULL || cast->message_loc.start == NULL) break;

const pm_constant_t *message = pm_constant_pool_id_to_constant(&parser->constant_pool, cast->name);
switch (message->length) {
case 1:
switch (message->start[0]) {
case '+':
case '-':
case '*':
case '/':
case '%':
case '|':
case '^':
case '&':
case '>':
case '<':
type = (const char *) message->start;
length = 1;
break;
}
break;
case 2:
switch (message->start[1]) {
case '=':
if (message->start[0] == '<' || message->start[0] == '>' || message->start[0] == '!' || message->start[0] == '=') {
type = (const char *) message->start;
length = 2;
}
break;
case '@':
if (message->start[0] == '+' || message->start[0] == '-') {
type = (const char *) message->start;
length = 2;
}
break;
case '*':
if (message->start[0] == '*') {
type = (const char *) message->start;
length = 2;
}
break;
}
break;
case 3:
if (memcmp(message->start, "<=>", 3) == 0) {
type = "<=>";
length = 3;
}
break;
}

break;
}
case PM_CONSTANT_PATH_NODE:
type = "::";
length = 2;
break;
case PM_CONSTANT_READ_NODE:
type = "a constant";
length = 10;
break;
case PM_DEFINED_NODE:
type = "defined?";
length = 8;
break;
case PM_FALSE_NODE:
type = "false";
length = 5;
break;
case PM_FLOAT_NODE:
case PM_IMAGINARY_NODE:
case PM_INTEGER_NODE:
case PM_INTERPOLATED_REGULAR_EXPRESSION_NODE:
case PM_INTERPOLATED_STRING_NODE:
case PM_RATIONAL_NODE:
case PM_REGULAR_EXPRESSION_NODE:
case PM_SOURCE_ENCODING_NODE:
case PM_SOURCE_FILE_NODE:
case PM_SOURCE_LINE_NODE:
case PM_STRING_NODE:
case PM_SYMBOL_NODE:
type = "a literal";
length = 9;
break;
case PM_NIL_NODE:
type = "nil";
length = 3;
break;
case PM_RANGE_NODE: {
const pm_range_node_t *cast = (const pm_range_node_t *) node;

if (PM_NODE_FLAG_P(cast, PM_RANGE_FLAGS_EXCLUDE_END)) {
type = "...";
length = 3;
} else {
type = "..";
length = 2;
}

break;
}
case PM_SELF_NODE:
type = "self";
length = 4;
break;
case PM_TRUE_NODE:
type = "true";
length = 4;
break;
default:
break;
}

if (type != NULL) {
PM_PARSER_WARN_NODE_FORMAT(parser, node, PM_WARN_VOID_STATEMENT, length, type);
}
}

/**
* Warn if any of the statements that are not the last statement in the list are
* a "void" statement.
*/
static void
pm_void_statements_check(pm_parser_t *parser, const pm_statements_node_t *node) {
if (parser->parsing_eval) return;

assert(node->body.size > 0);
for (size_t index = 0; index < node->body.size - 1; index++) {
pm_void_statement_check(parser, node->body.nodes[index]);
}
}

/**
* When we're handling the predicate of a conditional, we need to know our
* context in order to determine the kind of warning we should deliver to the
Expand Down Expand Up @@ -12911,6 +13063,8 @@ parse_statements(pm_parser_t *parser, pm_context_t context) {
}

context_pop(parser);
pm_void_statements_check(parser, statements);

return statements;
}

Expand Down Expand Up @@ -16666,6 +16820,7 @@ parse_expression_prefix(pm_parser_t *parser, pm_binding_power_t binding_power, b
pop_block_exits(parser, previous_block_exits);
pm_node_list_free(&current_block_exits);

pm_void_statements_check(parser, statements);
return (pm_node_t *) pm_parentheses_node_create(parser, &opening, (pm_node_t *) statements, &parser->previous);
}
case PM_TOKEN_BRACE_LEFT: {
Expand Down Expand Up @@ -20137,8 +20292,15 @@ parse_program(pm_parser_t *parser) {

parser_lex(parser);
pm_statements_node_t *statements = parse_statements(parser, PM_CONTEXT_MAIN);
if (!statements) {

if (statements == NULL) {
statements = pm_statements_node_create(parser);
} else if (!parser->parsing_eval) {
// If we have statements, then the top-level statement should be
// explicitly checked as well. We have to do this here because
// everywhere else we check all but the last statement.
assert(statements->body.size > 0);
pm_void_statement_check(parser, statements->body.nodes[statements->body.size - 1]);
}

pm_constant_id_list_t locals;
Expand Down Expand Up @@ -20589,7 +20751,7 @@ pm_parse_success_p(const uint8_t *source, size_t size, const char *data) {
pm_node_t *node = pm_parse(&parser);
pm_node_destroy(&parser, node);

bool result = parser.error_list.size == 0 && parser.warning_list.size == 0;
bool result = parser.error_list.size == 0;
pm_parser_free(&parser);
pm_options_free(&options);

Expand Down
3 changes: 2 additions & 1 deletion prism/templates/src/diagnostic.c.erb
View file
Expand Up @@ -352,7 +352,8 @@ static const pm_diagnostic_data_t diagnostic_messages[PM_DIAGNOSTIC_ID_MAX] = {
[PM_WARN_LITERAL_IN_CONDITION_VERBOSE] = { "%sliteral in %s", PM_WARNING_LEVEL_VERBOSE },
[PM_WARN_SHEBANG_CARRIAGE_RETURN] = { "shebang line ending with \\r may cause problems", PM_WARNING_LEVEL_DEFAULT },
[PM_WARN_UNEXPECTED_CARRIAGE_RETURN] = { "encountered \\r in middle of line, treated as a mere space", PM_WARNING_LEVEL_DEFAULT },
[PM_WARN_UNUSED_LOCAL_VARIABLE] = { "assigned but unused variable - %.*s", PM_WARNING_LEVEL_VERBOSE }
[PM_WARN_UNUSED_LOCAL_VARIABLE] = { "assigned but unused variable - %.*s", PM_WARNING_LEVEL_VERBOSE },
[PM_WARN_VOID_STATEMENT] = { "possibly useless use of %.*s in void context", PM_WARNING_LEVEL_VERBOSE }
};

/**
Expand Down
72 changes: 67 additions & 5 deletions test/prism/warnings_test.rb
View file
Expand Up @@ -24,15 +24,15 @@ def test_ambiguous_regexp
end

def test_equal_in_conditional
assert_warning("if a = 1; end; a", "should be ==")
assert_warning("if a = 1; end; a = a", "should be ==")
end

def test_dot_dot_dot_eol
assert_warning("foo...", "... at EOL")
assert_warning("_ = foo...", "... at EOL")
assert_warning("def foo(...) = bar ...", "... at EOL")

assert_warning("foo... #", "... at EOL")
assert_warning("foo... \t\v\f\n", "... at EOL")
assert_warning("_ = foo... #", "... at EOL")
assert_warning("_ = foo... \t\v\f\n", "... at EOL")

refute_warning("p foo...bar")
refute_warning("p foo... bar")
Expand All @@ -51,7 +51,7 @@ def test_duplicated_when_clause
end

def test_float_out_of_range
assert_warning("1.0e100000", "out of range")
assert_warning("_ = 1.0e100000", "out of range")
end

def test_integer_in_flip_flop
Expand Down Expand Up @@ -125,6 +125,68 @@ def test_unused_local_variables
refute_warning("def foo; bar = 1; tap { baz = bar; baz }; end")
end

def test_void_statements
assert_warning("foo = 1; foo", "a variable in void")
assert_warning("@foo", "a variable in void")
assert_warning("@@foo", "a variable in void")
assert_warning("$foo", "a variable in void")
assert_warning("$+", "a variable in void")
assert_warning("$1", "a variable in void")

assert_warning("self", "self in void")
assert_warning("nil", "nil in void")
assert_warning("true", "true in void")
assert_warning("false", "false in void")

assert_warning("1", "literal in void")
assert_warning("1.0", "literal in void")
assert_warning("1r", "literal in void")
assert_warning("1i", "literal in void")
assert_warning(":foo", "literal in void")
assert_warning("\"foo\"", "literal in void")
assert_warning("\"foo\#{1}\"", "literal in void")
assert_warning("/foo/", "literal in void")
assert_warning("/foo\#{1}/", "literal in void")

assert_warning("Foo", "constant in void")
assert_warning("::Foo", ":: in void")
assert_warning("Foo::Bar", ":: in void")

assert_warning("1..2", ".. in void")
assert_warning("1..", ".. in void")
assert_warning("..2", ".. in void")
assert_warning("1...2", "... in void")
assert_warning("1...;", "... in void")
assert_warning("...2", "... in void")

assert_warning("defined?(foo)", "defined? in void")

assert_warning("1 + 1", "+ in void")
assert_warning("1 - 1", "- in void")
assert_warning("1 * 1", "* in void")
assert_warning("1 / 1", "/ in void")
assert_warning("1 % 1", "% in void")
assert_warning("1 | 1", "| in void")
assert_warning("1 ^ 1", "^ in void")
assert_warning("1 & 1", "& in void")
assert_warning("1 > 1", "> in void")
assert_warning("1 < 1", "< in void")

assert_warning("1 ** 1", "** in void")
assert_warning("1 <= 1", "<= in void")
assert_warning("1 >= 1", ">= in void")
assert_warning("1 != 1", "!= in void")
assert_warning("1 == 1", "== in void")
assert_warning("1 <=> 1", "<=> in void")

assert_warning("+foo", "+@ in void")
assert_warning("-foo", "-@ in void")

assert_warning("def foo; @bar; @baz; end", "variable in void")
refute_warning("def foo; @bar; end")
refute_warning("@foo", compare: false, scopes: [[]])
end

private

def assert_warning(source, message)
Expand Down

0 comments on commit 4fc457e

Please sign in to comment.