diff --git a/news/572.api_change b/news/572.api_change new file mode 100644 index 000000000..4919b8762 --- /dev/null +++ b/news/572.api_change @@ -0,0 +1,2 @@ +Implicitly empty resolver arguments (e.g., `${foo:a,}`) are deprecated in favor of explicit quoted strings (e.g., `${foo:a,""}`) + diff --git a/omegaconf/grammar/OmegaConfGrammarParser.g4 b/omegaconf/grammar/OmegaConfGrammarParser.g4 index eaa496d8f..58000e2b6 100644 --- a/omegaconf/grammar/OmegaConfGrammarParser.g4 +++ b/omegaconf/grammar/OmegaConfGrammarParser.g4 @@ -42,7 +42,7 @@ element: listContainer: BRACKET_OPEN sequence? BRACKET_CLOSE; // [], [1,2,3], [a,b,[1,2]] dictContainer: BRACE_OPEN (dictKeyValuePair (COMMA dictKeyValuePair)*)? BRACE_CLOSE; // {}, {a:10,b:20} dictKeyValuePair: dictKey COLON element; -sequence: element (COMMA element)*; +sequence: (element (COMMA element?)*) | (COMMA element?)+; // Interpolations. diff --git a/omegaconf/grammar_visitor.py b/omegaconf/grammar_visitor.py index debfa7f52..c41721ef4 100644 --- a/omegaconf/grammar_visitor.py +++ b/omegaconf/grammar_visitor.py @@ -1,4 +1,5 @@ import sys +import warnings from typing import ( TYPE_CHECKING, Any, @@ -246,20 +247,42 @@ def visitSequence( ) -> Generator[Any, None, None]: from ._utils import _get_value - assert ctx.getChildCount() >= 1 # element (COMMA element)* - for i, child in enumerate(ctx.getChildren()): - if i % 2 == 0: - assert isinstance(child, OmegaConfGrammarParser.ElementContext) + # (element (COMMA element?)*) | (COMMA element?)+ + assert ctx.getChildCount() >= 1 + + # DEPRECATED: remove in 2.2 (revert #571) + def empty_str_warning() -> None: + txt = ctx.getText() + warnings.warn( + f"In the sequence `{txt}` some elements are missing: please replace " + f"them with empty quoted strings. " + f"See https://github.com/omry/omegaconf/issues/572 for details.", + category=UserWarning, + ) + + is_previous_comma = True # whether previous child was a comma (init to True) + for child in ctx.getChildren(): + if isinstance(child, OmegaConfGrammarParser.ElementContext): # Also preserve the original text representation of `child` so # as to allow backward compatibility with old resolvers (registered # with `legacy_register_resolver()`). Note that we cannot just cast # the value to string later as for instance `null` would become "None". yield _get_value(self.visitElement(child)), child.getText() + is_previous_comma = False else: assert ( isinstance(child, TerminalNode) and child.symbol.type == OmegaConfGrammarLexer.COMMA ) + if is_previous_comma: + empty_str_warning() + yield "", "" + else: + is_previous_comma = True + if is_previous_comma: + # Trailing comma. + empty_str_warning() + yield "", "" def visitSingleElement( self, ctx: OmegaConfGrammarParser.SingleElementContext diff --git a/tests/test_grammar.py b/tests/test_grammar.py index 28f5b07b8..f585e632f 100644 --- a/tests/test_grammar.py +++ b/tests/test_grammar.py @@ -1,8 +1,9 @@ import math +import re from typing import Any, Callable, List, Optional, Tuple import antlr4 -from pytest import mark, param, raises +from pytest import mark, param, raises, warns from omegaconf import ( DictConfig, @@ -369,6 +370,30 @@ def test_config_value( parse_tree, expected_visit = self._parse("configValue", definition, expected) self._visit_with_config(parse_tree, expected_visit) + @parametrize_from( + [ + ("trailing_comma", "${test:a,b,}", ["a", "b", ""]), + ("empty_middle", "${test:a,,b}", ["a", "", "b"]), + ("empty_first", "${test:,a,b}", ["", "a", "b"]), + ("single_comma", "${test:,}", ["", ""]), + ( + "mixed_with_ws", + "${test: ,a,b,\t,,c, \t \t ,d,, \t}", + ["", "a", "b", "", "", "c", "", "d", "", ""], + ), + ] + ) + def test_deprecated_empty_args( + self, restore_resolvers: Any, definition: str, expected: Any + ) -> None: + OmegaConf.register_new_resolver("test", self._resolver_test) + + parse_tree, expected_visit = self._parse("singleElement", definition, expected) + with warns( + UserWarning, match=re.escape("https://github.com/omry/omegaconf/issues/572") + ): + self._visit_with_config(parse_tree, expected_visit) + def _check_is_same_type(self, value: Any, expected: Any) -> None: """ Helper function to validate that types of `value` and `expected are the same.