[Bug] Override grammar: quoted strings ending with a trailing backslash may trigger parsing errors

[odelalleau]

🐛 Bug

Description

The following override examples cause exceptions (note: there is escaping from the shell, so when using \\ below, Hydra only sees \, and escaped quotes are also replaced, e.g. \" -> "):

python my_app.py "+foo=['#\\', 'x']"  # case 1a
python my_app.py "+foo={a: '#\\', b: 'x'}"  # case 2a
python my_app.py "+foo=['#\\\\', 'x']"  # case 3a
python my_app.py "+foo={a: '#\\\\', b: 'x'}"  # case 4a

Note that if the second string is using double quotes then there is no exception:

python my_app.py "+foo=['#\\', \"x\"]"  # case 1b
python my_app.py "+foo={a: '#\\', b: \"x\"}"  # case 2b
python my_app.py "+foo=['#\\\\', \"x\"]"  # case 3b
python my_app.py "+foo={a: '#\\\\', b: \"x\"}"  # case 4b

Expected behavior

The fix to this issue should make it so that:

• Using single or double quotes in the second string ('x' vs "x") yields the same result (i.e., cases Xa and Xb should be similar)
• Cases 1 and 2 trigger an exception
• Cases 3 and 4 run without exception, with the first string containing characters #\ (i.e., with a single backslash), compared to the current behavior where 3b and 4b yield #\\

Migration instructions

If a working string in Hydra 1.0 triggers an exception or behaves unexpectedly in Hydra 1.1, this means some \ need to be properly escaped. Here are two examples (where again, \ are doubled because of shell escaping):

(Hydra 1.0)                           -> (Hydra 1.1)
python my_app.py "+foo='abc\\'"       -> python my_app.py "+foo='abc\\\\'"        # abc\
python my_app.py "+foo='abc\\\\'def'" -> python my_app.py "+foo='abc\\\\\\'def'"  # abc\'def

Are you willing to open a pull request? (See CONTRIBUTING)

Yes I will.

Additional context

This is caused by the definition of QUOTED_VALUE in the grammar, similar to OmegaConf's issue omry/omegaconf#617

[omry]

Looks like defining QUOTED_VALUE like in OmegaConf (in the iteration you are pointing to solves the issues)?

QUOTED_VALUE:
      '"' (('\\\\')* | (.)*? ~[\\] ('\\\\')*) '"'     // double quotes
    | '\'' (('\\\\')* | (.)*? ~[\\] ('\\\\')*) '\'';  // single quotes

Cases 1 and 2 trigger an exception

By this you mean the case where Hydra is getting a single backslash, yes?

+foo=['#\', 'x']
+foo={a: '#\', b: 'x'}

[odelalleau]

Looks like defining QUOTED_VALUE like in OmegaConf (in the iteration you are pointing to solves the issues)?

QUOTED_VALUE:
      '"' (('\\\\')* | (.)*? ~[\\] ('\\\\')*) '"'     // double quotes
    | '\'' (('\\\\')* | (.)*? ~[\\] ('\\\\')*) '\'';  // single quotes

I'm actually planning to use the lexer mode-based approach from omry/omegaconf#695 because later I'll also need to be able to handle nested quoted strings within interpolations (if you think we can avoid it let me know).

Cases 1 and 2 trigger an exception

By this you mean the case where Hydra is getting a single backslash, yes?

+foo=['#\', 'x']
+foo={a: '#\', b: 'x'}

Yes, those would crash because \' would be considered as an escaped quote (like it is now, but the plan is to also make them crash in cases 1b and 2b which are currently accepted)

[omry]

I'm actually planning to use the lexer mode-based approach from omry/omegaconf#695 because later I'll also need to be able to handle nested quoted strings within interpolations (if you think we can avoid it let me know)

If we find a way to make the Hydra grammar agnostic to interpolations we should not need this.