Unset config properties: use default value or fail with a meaningful exception #10013
Conversation
| @@ -76,7 +77,14 @@ public void endVisit(JPermutationDependentValue x, Context ctx) { | |||
| private JExpression propertyValueExpression(JPermutationDependentValue x) { | |||
| List<String> propertyValues = props.getConfigurationProperties().getStrings(x.getRequestedValue()); | |||
|
|
|||
| String propertyValue = propertyValues.isEmpty() ? null : Joiner.on(",").join(propertyValues); | |||
| if (!propertyValues.isEmpty() && propertyValues.stream().allMatch(Objects::isNull)) { | |||
There was a problem hiding this comment.
I struggle with this distinction every time I try to get back into this code and clean it up - could you add a comment here explaining why we sometimes get [] and sometimes [null]? I think it is signalling (badly, incompletely) that this config property is only permitted to have a single value, but there is no distinction here between "multi-valued with one value" and "single valued and set", so I'm not sure why it matters... Also there is no distinction between "multivalued and unset" and "undefined key", which seems even more egregious.
At least if we keep that confusing behavior, a comment would help to clarify as to why we even do it this way.
| if (x.getDefaultValueExpression() != null) { | ||
| return x.getDefaultValueExpression(); | ||
| } | ||
| throw new InternalCompilerException("No default set for configuration property '" |
There was a problem hiding this comment.
Do we want to throw here? What about letting the value be null - that's what would happen for java if a system property was unset?
That said, this could also be a follow-up feature after the bug is fixed.
There was a problem hiding this comment.
From #9806 (comment) I assumed that defining a config property without a default is bad and should result in a build failure. That's also why I filed gwtproject/gwt-safehtml#31 . I'm OK with changing the behavior and returning null. @jnehlmeier thoughts?
There was a problem hiding this comment.
If it is an internal compiler error I guess I just don't like it to happen under normal circumstances for a user who either
a) was expecting System.getProperty(name) to return null
b) forgot to pass a default as the second arg to Sysmte.getProperty
c) forgot to set the config property value
Any/all of those seem like a reasonable expectations to have - and you shouldn't be rewarded with an InternalCompilerException if you fail that. Plus, if you hit one ICE, that's the end of compilation, but we in theory could have several errors we could pick up here. ICE does have the advantage that it gives you very clear context about where the error happened - but we have the SourceInfo instance here and can log something helpful?
JsInteropRestrictionChecker is a pretty good example of "I need to log a fatal error for the developer to consume, and a stack trace will never help them figure it out".
If the user did make mistake A or B above, then logging an error will help them figure it out. C wouldn't be as helpful, but the text of the message can be clear that they just need to define a value in the .gwt.xml (or command line, -setProperty foo=bar).
| throw new InternalCompilerException("No default set for configuration property '" | ||
| + x.getRequestedValue() + "'"); | ||
| } | ||
| String propertyValue = propertyValues.isEmpty() ? null : Joiner.on(",").skipNulls().join(propertyValues); |
There was a problem hiding this comment.
/home/runner/work/gwt/gwt/gwt/dev/core/src/com/google/gwt/dev/jjs/impl/ResolvePermutationDependentValues.java:78:0: warning: Line is longer than 100 characters (found 105). (com.puppycrawl.tools.checkstyle.checks.sizes.LineLengthCheck)
/home/runner/work/gwt/gwt/gwt/dev/core/src/com/google/gwt/dev/jjs/impl/ResolvePermutationDependentValues.java:87:0: warning: Line is longer than 100 characters (found 111). (com.puppycrawl.tools.checkstyle.checks.sizes.LineLengthCheck)
If I'm reading right the first is outside of your patch so ignore.
Fixes #9885
Not quite sure about the usecase where the Joiner is actually used.