Curly braces in configuration options are a source for unintentional misconfiguration

[MartinSpiessl]

This came up here:

https://gitlab.com/sosy-lab/software/cpachecker/-/issues/578#note_412659072

Some users use some.config.option={} in their benchmark xml and config files. However it seems to be unclear whether this (while apparently working) is actually disallowed or not. According to @PhilippWendler this will be interpreted as the literal string {} and not as an empty collection.

I guess the problem comes probably from the fact that the documentation that is generated for the configuration options lists collection-type default values in curly braces, while users are actually never supposed to add those braces (?). I created issue #31 for this.

We should probably either support curly braces in config values or emit a warning in such circumstances.

[PhilippWendler]

I do not understand what this issue is about? Can you please elaborate what we should do or what your question is?

[MartinSpiessl]

We should probably either support curly braces in config values or emit a warning in such circumstances.

I guess it could be doable to have support for parsing {} as empty list and not as string, and parsing {a,b,c} as list of these three elements. If not, then users are currently sometimes inserting curly braces anyway while expecting a different behavior. In those cases we could at least emit a warning.

[PhilippWendler]

Hm. I would prefer to not treat { as somewhat special, because it never was treated in a special way and it was also never documented that it would. Changing this now seems like a change without benefit and backwards incompatibility.

From the point of view of the configuration system, {} is a perfectly valid value (depending on the type of the option, of course), so I would also prefer to not warn about it. If {} is an invalid value from the point of view of the receiver of that option, it should be caught later on together with all other invalid values (the configuration system even provides utilities for this, e.g., with the possibility to provide a regexp of allowed values).

[MartinSpiessl]

Hmm so if I understand you correctly you say this should be implemented by the user of java-common-lib, e.g. in CPAchecker's configuration checks and not be part of java-common-lib itself. I could very much live with that. Should also be easy to add there.

So we could add the warning there (maybe only temporarily?) and hint to the solution of #33 in case we have found one.

(the configuration system even provides utilities for this, e.g., with the possibility to provide a regexp of allowed values).

Do you have an example of this at hand? I cannot recall to ever have seen that feature in action. Maybe we should use it more often.

[PhilippWendler]

Hmm so if I understand you correctly you say this should be implemented by the user of java-common-lib, e.g. in CPAchecker's configuration checks and not be part of java-common-lib itself. I could very much live with that. Should also be easy to add there.

No, I suggest to not add a central warning, neither in this project nor in any using project. This configuration system is highly decentralized on purpose, and no code except the class where an option is declared can know whether {} is a legal value.

(the configuration system even provides utilities for this, e.g., with the possibility to provide a regexp of allowed values).

Do you have an example of this at hand? I cannot recall to ever have seen that feature in action. Maybe we should use it more often.

https://github.com/sosy-lab/cpachecker/blob/29246e46e7473d7f2e7431c90b0798e5e602f882/src/org/sosy_lab/cpachecker/cfa/CFACreator.java#L124

Of course, in many cases you do not need it, because the option is declared to have a different type, not String, which has the same effect of limiting the allowed values.

[MartinSpiessl]

I see, thanks!

Of course, in many cases you do not need it, because the option is declared to have a different type, not String, which has the same effect of limiting the allowed values.

So there are also options that are lists of C function names, so there it would be nice to check for valid names I guess. But I guess that could/should then be done in the class that actually uses that option.

[PhilippWendler]

Btw: Yet another way to restrict the set of allowed values, even dynamically, is to simply add a respective TypeConverter instance. This can implement arbitrary checks.

To use an own TypeConverter, one can either change the type of the option to a custom class and associate that type with a certain converter, or create an annotation, label the option with it, and associate that annotation with the converter. Using the latter it is even possible to use custom converters for String options. (Example of this is @IntegerOption and IntegerTypeConverter, which is used by default.)

To me it seems there is no action item left in this issue so I would suggest to close it?

[MartinSpiessl]

Looks like it. Just to summarize:

We should probably either support curly braces in config values or emit a warning in such circumstances.

For the first part, the conclusion is that tools can support this if they use a custom TypeConverter, so there is nothing to do at java-common-lib

For the second part, we do not want to print such warnings in general (because we cannot even do this, because for some tools {} might be perfectly fine). This should be done e.g. by regex field in the @Option annotation or in the TypeConverter in the tool itself. So nothing needs / can be done in java-common-lib and the issue can be closed.