Skip to content

Fix up duplicate key warning #4190

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.

Sign up for GitHub

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

Merged
kddnewton merged 1 commit into from
Jul 31, 2026
+119 −26
Merged

Fix up duplicate key warning #4190

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 4 additions & 2 deletions include/prism/internal/static_literals.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
#ifndef PRISM_INTERNAL_STATIC_LITERALS_H
#define PRISM_INTERNAL_STATIC_LITERALS_H

#include "prism/internal/encoding.h"

#include "prism/ast.h"
#include "prism/buffer.h"
#include "prism/line_offset_list.h"
Expand Down Expand Up @@ -83,7 +85,7 @@ typedef struct {
/*
* Add a node to the set of static literals.
*/
pm_node_t * pm_static_literals_add(const pm_line_offset_list_t *line_offsets, const uint8_t *start, int32_t start_line, pm_static_literals_t *literals, pm_node_t *node, bool replace);
pm_node_t * pm_static_literals_add(const pm_line_offset_list_t *line_offsets, const uint8_t *start, int32_t start_line, const pm_encoding_t *encoding, pm_static_literals_t *literals, pm_node_t *node, bool replace);

/*
* Free the internal memory associated with the given static literals set.
Expand All @@ -93,6 +95,6 @@ void pm_static_literals_free(pm_static_literals_t *literals);
/*
* Create a string-based representation of the given static literal.
*/
void pm_static_literal_inspect(pm_buffer_t *buffer, const pm_line_offset_list_t *line_offsets, const uint8_t *start, int32_t start_line, const char *encoding_name, const pm_node_t *node);
void pm_static_literal_inspect(pm_buffer_t *buffer, const pm_line_offset_list_t *line_offsets, const uint8_t *start, int32_t start_line, const pm_encoding_t *encoding, const pm_node_t *node);

#endif
8 changes: 4 additions & 4 deletions src/prism.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13685,11 +13685,11 @@ parse_statements(pm_parser_t *parser, pm_context_t context, uint16_t depth) {
*/
static void
pm_hash_key_static_literals_add(pm_parser_t *parser, pm_static_literals_t *literals, pm_node_t *node) {
const pm_node_t *duplicated = pm_static_literals_add(&parser->line_offsets, parser->start, parser->start_line, literals, node, true);
const pm_node_t *duplicated = pm_static_literals_add(&parser->line_offsets, parser->start, parser->start_line, parser->encoding, literals, node, true);

if (duplicated != NULL) {
pm_buffer_t buffer = { 0 };
pm_static_literal_inspect(&buffer, &parser->line_offsets, parser->start, parser->start_line, parser->encoding->name, duplicated);
pm_static_literal_inspect(&buffer, &parser->line_offsets, parser->start, parser->start_line, parser->encoding, duplicated);

pm_diagnostic_list_append_format(
&parser->metadata_arena,
Expand All @@ -13714,7 +13714,7 @@ static void
pm_when_clause_static_literals_add(pm_parser_t *parser, pm_static_literals_t *literals, pm_node_t *node) {
pm_node_t *previous;

if ((previous = pm_static_literals_add(&parser->line_offsets, parser->start, parser->start_line, literals, node, false)) != NULL) {
if ((previous = pm_static_literals_add(&parser->line_offsets, parser->start, parser->start_line, parser->encoding, literals, node, false)) != NULL) {
pm_diagnostic_list_append_format(
&parser->metadata_arena,
&parser->warning_list,
Expand Down Expand Up @@ -17095,7 +17095,7 @@ parse_pattern_hash_implicit_value(pm_parser_t *parser, pm_constant_id_list_t *ca
*/
static void
parse_pattern_hash_key(pm_parser_t *parser, pm_static_literals_t *keys, pm_node_t *node) {
if (pm_static_literals_add(&parser->line_offsets, parser->start, parser->start_line, keys, node, true) != NULL) {
if (pm_static_literals_add(&parser->line_offsets, parser->start, parser->start_line, parser->encoding, keys, node, true) != NULL) {
pm_parser_err_node(parser, node, PM_ERR_PATTERN_HASH_KEY_DUPLICATE);
}
}
Expand Down
80 changes: 60 additions & 20 deletions src/static_literals.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@ typedef struct {
/** The line number that the parser starts on. */
int32_t start_line;

/** The name of the encoding that the parser is using. */
const char *encoding_name;
/** The encoding that the parser is using. */
const pm_encoding_t *encoding;
} pm_static_literals_metadata_t;

static PRISM_INLINE uint32_t
Expand Down Expand Up @@ -94,6 +94,39 @@ integer_hash(const pm_integer_t *integer) {
return hash;
}

/**
* Return the flags of the given node that determine the encoding of its string.
*
* The FORCED_* flags record how a literal was written. An escape that locks the
* encoding to UTF-8 (a `\u` escape above U+007F, or any `\u` in a character
* literal) sets FORCED_UTF8 whether or not the file is already UTF-8. So in a
* UTF-8 file a `\u00E9` escape and a literal e-acute carry different flags
* while being the same string, and the flag has to be dropped for them to
* compare equally.
*/
static pm_node_flags_t
node_encoding_flags(const pm_static_literals_metadata_t *metadata, const pm_node_t *node) {
switch (PM_NODE_TYPE(node)) {
case PM_STRING_NODE: {
pm_node_flags_t mask = PM_STRING_FLAGS_FORCED_BINARY_ENCODING;
if (metadata->encoding != PM_ENCODING_UTF_8_ENTRY) mask |= PM_STRING_FLAGS_FORCED_UTF8_ENCODING;
return node->flags & mask;
}
case PM_SYMBOL_NODE: {
pm_node_flags_t mask = PM_SYMBOL_FLAGS_FORCED_BINARY_ENCODING | PM_SYMBOL_FLAGS_FORCED_US_ASCII_ENCODING;
if (metadata->encoding != PM_ENCODING_UTF_8_ENTRY) mask |= PM_SYMBOL_FLAGS_FORCED_UTF8_ENCODING;
return node->flags & mask;
}
case PM_SOURCE_FILE_NODE:
/* __FILE__ takes the encoding of the filepath, and every instance
* of it in a parse resolves the same way. */
return 0;
default:
assert(false && "unreachable");
return 0;
}
}

/**
* Return the hash of the given node. It is important that nodes that have
* equivalent static literal values have the same hash. This is because we use
Expand Down Expand Up @@ -134,11 +167,7 @@ node_hash(const pm_static_literals_metadata_t *metadata, const pm_node_t *node)
// Strings hash their value and mix in their flags so that different
// encodings are not considered equal.
const pm_string_t *value = &((const pm_string_node_t *) node)->unescaped;

pm_node_flags_t flags = node->flags;
flags &= (PM_STRING_FLAGS_FORCED_BINARY_ENCODING | PM_STRING_FLAGS_FORCED_UTF8_ENCODING);

return murmur_hash(pm_string_source(value), pm_string_length(value) * sizeof(uint8_t)) ^ murmur_scramble((uint32_t) flags);
return murmur_hash(pm_string_source(value), pm_string_length(value) * sizeof(uint8_t)) ^ murmur_scramble((uint32_t) node_encoding_flags(metadata, node));
}
case PM_SOURCE_FILE_NODE: {
// Source files hash their value and mix in their flags so that
Expand All @@ -156,7 +185,7 @@ node_hash(const pm_static_literals_metadata_t *metadata, const pm_node_t *node)
// Symbols hash their value and mix in their flags so that different
// encodings are not considered equal.
const pm_string_t *value = &((const pm_symbol_node_t *) node)->unescaped;
return murmur_hash(pm_string_source(value), pm_string_length(value) * sizeof(uint8_t)) ^ murmur_scramble((uint32_t) node->flags);
return murmur_hash(pm_string_source(value), pm_string_length(value) * sizeof(uint8_t)) ^ murmur_scramble((uint32_t) node_encoding_flags(metadata, node));
}
default:
assert(false && "unreachable");
Expand Down Expand Up @@ -344,10 +373,21 @@ pm_string_value(const pm_node_t *node) {
* A comparison function for comparing two nodes that have attached strings.
*/
static int
pm_compare_string_nodes(PRISM_UNUSED const pm_static_literals_metadata_t *metadata, const pm_node_t *left, const pm_node_t *right) {
pm_compare_string_nodes(const pm_static_literals_metadata_t *metadata, const pm_node_t *left, const pm_node_t *right) {
const pm_string_t *left_string = pm_string_value(left);
const pm_string_t *right_string = pm_string_value(right);
return pm_string_compare(left_string, right_string);

int result = pm_string_compare(left_string, right_string);
if (result != 0) return result;

/*
* Equal bytes are not enough. In a binary source file the two bytes written
* as `"\xC3\xA9"` stay BINARY while a `"\u00E9"` escape is forced to UTF-8,
* so those are distinct keys even though the bytes match.
*/
pm_node_flags_t left_flags = node_encoding_flags(metadata, left);
pm_node_flags_t right_flags = node_encoding_flags(metadata, right);
return PM_NUMERIC_COMPARISON(left_flags, right_flags);
}

/**
Expand All @@ -370,7 +410,7 @@ pm_compare_regular_expression_nodes(PRISM_UNUSED const pm_static_literals_metada
* Add a node to the set of static literals.
*/
pm_node_t *
pm_static_literals_add(const pm_line_offset_list_t *line_offsets, const uint8_t *start, int32_t start_line, pm_static_literals_t *literals, pm_node_t *node, bool replace) {
pm_static_literals_add(const pm_line_offset_list_t *line_offsets, const uint8_t *start, int32_t start_line, const pm_encoding_t *encoding, pm_static_literals_t *literals, pm_node_t *node, bool replace) {
switch (PM_NODE_TYPE(node)) {
case PM_INTEGER_NODE:
case PM_SOURCE_LINE_NODE:
Expand All @@ -380,7 +420,7 @@ pm_static_literals_add(const pm_line_offset_list_t *line_offsets, const uint8_t
.line_offsets = line_offsets,
.start = start,
.start_line = start_line,
.encoding_name = NULL
.encoding = encoding
},
node,
replace,
Expand All @@ -393,7 +433,7 @@ pm_static_literals_add(const pm_line_offset_list_t *line_offsets, const uint8_t
.line_offsets = line_offsets,
.start = start,
.start_line = start_line,
.encoding_name = NULL
.encoding = encoding
},
node,
replace,
Expand All @@ -407,7 +447,7 @@ pm_static_literals_add(const pm_line_offset_list_t *line_offsets, const uint8_t
.line_offsets = line_offsets,
.start = start,
.start_line = start_line,
.encoding_name = NULL
.encoding = encoding
},
node,
replace,
Expand All @@ -421,7 +461,7 @@ pm_static_literals_add(const pm_line_offset_list_t *line_offsets, const uint8_t
.line_offsets = line_offsets,
.start = start,
.start_line = start_line,
.encoding_name = NULL
.encoding = encoding
},
node,
replace,
Expand All @@ -434,7 +474,7 @@ pm_static_literals_add(const pm_line_offset_list_t *line_offsets, const uint8_t
.line_offsets = line_offsets,
.start = start,
.start_line = start_line,
.encoding_name = NULL
.encoding = encoding
},
node,
replace,
Expand All @@ -447,7 +487,7 @@ pm_static_literals_add(const pm_line_offset_list_t *line_offsets, const uint8_t
.line_offsets = line_offsets,
.start = start,
.start_line = start_line,
.encoding_name = NULL
.encoding = encoding
},
node,
replace,
Expand Down Expand Up @@ -589,7 +629,7 @@ pm_static_literal_inspect_node(pm_buffer_t *buffer, const pm_static_literals_met
break;
}
case PM_SOURCE_ENCODING_NODE:
pm_buffer_append_format(buffer, "#<Encoding:%s>", metadata->encoding_name);
pm_buffer_append_format(buffer, "#<Encoding:%s>", metadata->encoding->name);
break;
case PM_SOURCE_FILE_NODE: {
const pm_string_t *filepath = &((const pm_source_file_node_t *) node)->filepath;
Expand Down Expand Up @@ -627,14 +667,14 @@ pm_static_literal_inspect_node(pm_buffer_t *buffer, const pm_static_literals_met
* Create a string-based representation of the given static literal.
*/
void
pm_static_literal_inspect(pm_buffer_t *buffer, const pm_line_offset_list_t *line_offsets, const uint8_t *start, int32_t start_line, const char *encoding_name, const pm_node_t *node) {
pm_static_literal_inspect(pm_buffer_t *buffer, const pm_line_offset_list_t *line_offsets, const uint8_t *start, int32_t start_line, const pm_encoding_t *encoding, const pm_node_t *node) {
pm_static_literal_inspect_node(
buffer,
&(pm_static_literals_metadata_t) {
.line_offsets = line_offsets,
.start = start,
.start_line = start_line,
.encoding_name = encoding_name
.encoding = encoding
},
node
);
Expand Down
51 changes: 51 additions & 0 deletions test/prism/result/warnings_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,57 @@ def test_duplicated_when_clause
assert_warning("case 1; when 1, 1; end", "when' clause")
end

# Two literals with the same bytes are the same key only when they also end
# up in the same encoding. An escape that locks the encoding to UTF-8 sets
# FORCED_UTF8 whether or not the file is already UTF-8, so the flag has to
# be resolved against the source encoding before literals are compared.
#
# In a binary file the raw bytes stay BINARY while the escape resolves to
# UTF-8, so these are genuinely different keys and must not warn.
def test_duplicated_literals_distinct_encodings
binary = "# -*- encoding: ascii-8bit -*-\n"

refute_warning("#{binary}{\"\xC3\xA9\" => 1, \"\\u00E9\" => 2}".b)
refute_warning("#{binary}{:\"\xC3\xA9\" => 1, :\"\\u00E9\" => 2}".b)
refute_warning("#{binary}case foo\nwhen \"\xC3\xA9\"\nwhen \"\\u00E9\"\nend".b)

# Written as two escapes rather than raw bytes, the pair still resolves to
# BINARY and UTF-8 respectively, and the hash really does hold two keys.
refute_warning("#{binary}{\"\\xC3\\xA9\" => 1, \"\\u00E9\" => 2}".b)
refute_warning("#{binary}{:\"\\xC3\\xA9\" => 1, :\"\\u00E9\" => 2}".b)

# A US-ASCII source is the only one that emits FORCED_BINARY, and BINARY
# is not the source encoding there, so the pair stays distinct.
us_ascii = "# -*- encoding: us-ascii -*-\n"

refute_warning("#{us_ascii}{\"\\xC3\\xA9\" => 1, \"\\u00E9\" => 2}")
refute_warning("#{us_ascii}{:\"\\xC3\\xA9\" => 1, :\"\\u00E9\" => 2}")
end

# The mirror image: in a UTF-8 file the escape resolves to the very same
# string, so the pair is a duplicate and still has to be reported.
def test_duplicated_literals_matching_encodings
assert_warning("{\"\xC3\xA9\" => 1, \"\\u00E9\" => 2}", "duplicated and overwritten")
assert_warning("{:\"\xC3\xA9\" => 1, :\"\\u00E9\" => 2}", "duplicated and overwritten")
assert_warning("case foo\nwhen \"\xC3\xA9\"\nwhen \"\\u00E9\"\nend", "when' clause")

# Same bytes and same encoding are duplicates in a binary file too.
binary = "# -*- encoding: ascii-8bit -*-\n"

assert_warning("#{binary}{\"\xC3\xA9\" => 1, \"\xC3\xA9\" => 2}".b, "duplicated and overwritten")
assert_warning("#{binary}{\"\\u00E9\" => 1, \"\\u00E9\" => 2}".b, "duplicated and overwritten")
end

# Hash patterns raise rather than warn, and resolve encodings the same way.
def test_duplicated_pattern_keys_resolve_encoding
binary = "# -*- encoding: ascii-8bit -*-\n"
distinct = "#{binary}case x\nin {\"\xC3\xA9\": a, \"\\u00E9\": b}\nend".b
duplicate = "case x\nin {\"\xC3\xA9\": a, \"\\u00E9\": b}\nend"

assert_empty Prism.parse(distinct).errors.map(&:message)
assert_equal ["duplicated key name"], Prism.parse(duplicate).errors.map(&:message)
end

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