Skip to content

Commit 1056515

Browse files
HT154HT154
committed
Implement proper error messages
1 parent b971ff8 commit 1056515

File tree

3 files changed

+158
-67
lines changed

3 files changed

+158
-67
lines changed

‎pkl-cli/src/test/kotlin/org/pkl/cli/CliMainTest.kt‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -37,11 +37,6 @@ import org.pkl.core.Release
3737
class CliMainTest {
3838
private val rootCmd = RootCommand()
3939

40-
@Test
41-
fun `do it`() {
42-
rootCmd.main(listOf("run", "../test.pkl", "../test2*.pkl"))
43-
}
44-
4540
@Test
4641
fun `duplicate CLI option produces meaningful error message`(@TempDir tempDir: Path) {
4742
val inputFile = tempDir.resolve("test.pkl").writeString("").toString()

‎pkl-core/src/main/java/org/pkl/core/runtime/CommandSpecParser.java‎

Lines changed: 117 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -33,11 +33,11 @@
3333
import org.pkl.core.CommandSpec;
3434
import org.pkl.core.CommandSpec.OptionType;
3535
import org.pkl.core.CommandSpec.OptionType.Collection;
36+
import org.pkl.core.CommandSpec.OptionType.Enum;
3637
import org.pkl.core.CommandSpec.OptionType.Primitive;
3738
import org.pkl.core.FileOutput;
3839
import org.pkl.core.PNull;
3940
import org.pkl.core.PType;
40-
import org.pkl.core.PklBugException;
4141
import org.pkl.core.SecurityManager;
4242
import org.pkl.core.SecurityManagerException;
4343
import org.pkl.core.ast.ExpressionNode;
@@ -50,6 +50,7 @@
5050
import org.pkl.core.ast.member.DefaultPropertyBodyNode;
5151
import org.pkl.core.ast.member.ModuleNode;
5252
import org.pkl.core.ast.member.ObjectMember;
53+
import org.pkl.core.ast.member.PropertyTypeNode;
5354
import org.pkl.core.ast.type.TypeNode;
5455
import org.pkl.core.ast.type.UnresolvedTypeNode;
5556
import org.pkl.core.externalreader.ExternalReaderProcessException;
@@ -115,43 +116,36 @@ public CommandSpec parse(VmTyped commandModule) {
115116
private VmClass getOptionsClass(VmTyped commandModule) {
116117
var optionsProperty = commandModule.getVmClass().getProperty(Identifier.OPTIONS);
117118
if (optionsProperty == null) {
118-
throw new PklBugException("no property `options` found in pkl:Command sub-module");
119+
throw exceptionBuilder()
120+
.cannotFindMember(commandModule, Identifier.OPTIONS)
121+
.withSourceSection(commandModule.getVmClass().getSourceSection())
122+
.build();
119123
}
120124
var optionsPropertyTypeNode = optionsProperty.getTypeNode();
121125
if (optionsPropertyTypeNode == null) {
122-
throw new RuntimeException(
123-
"no type annotation on `options` property in pkl:Command sub-module"); // TODO
126+
throw exceptionBuilder()
127+
.withSourceSection(optionsProperty.getSourceSection())
128+
.evalError("commandOptionNoTypeAnnotation", "options", " in `pkl:Command` subclass")
129+
.build();
124130
}
125131
var optionsTypeNode = optionsPropertyTypeNode.getTypeNode();
126132
if (!(optionsTypeNode instanceof TypeNode.ClassTypeNode node)) {
127-
throw new RuntimeException(
128-
"type annotation on `options` property in pkl:Command sub-module is not a class type"); // TODO
133+
throw exceptionBuilder()
134+
.withSourceSection(optionsTypeNode.getSourceSection())
135+
.evalError(
136+
"commandOptionsTypeNotClass", optionsTypeNode.getSourceSection().getCharacters())
137+
.build();
129138
}
130139
var clazz = node.getVmClass();
131140
if (clazz.isAbstract()) {
132-
throw new RuntimeException(
133-
"class of `options` property in pkl:Command sub-module must not be abstact"); // TODO
141+
throw exceptionBuilder()
142+
.withSourceSection(clazz.getHeaderSection())
143+
.evalError("commandOptionsTypeAbstractClass", clazz.getQualifiedName())
144+
.build();
134145
}
135146
return clazz;
136147
}
137148

138-
// name from property name
139-
// description from doc comment
140-
// required if not nullable
141-
// parse: when specified, don't validate for known option types
142-
// args:
143-
// * multiple may be specified
144-
// * no List allowed if command has children
145-
// * only last arg may be a List
146-
// types:
147-
// * Primitive:
148-
// * Number, Float, Int, UInt, Int8, Int16, Int32, UInt8, UInt16, UInt32
149-
// * Boolean
150-
// * String, Char
151-
// * Enum: union of string literals
152-
// * Collection: List<Primitive>, Set<Primitive>
153-
// * Map: Map<Primitive, Primitive>
154-
155149
private Pair<List<CommandSpec.Flag>, List<CommandSpec.Argument>> collectOptions(
156150
VmClass optionsClass) {
157151
var flags = new ArrayList<CommandSpec.Flag>();
@@ -183,18 +177,24 @@ private Pair<List<CommandSpec.Flag>, List<CommandSpec.Argument>> collectOptions(
183177

184178
var defaultValue = getDefaultValue(prop);
185179
if (flagAnnotation != null && argAnnotation != null) {
186-
throw new RuntimeException(
187-
"found both @Flag and @Argument annotations for property `XXX`"); // TODO
180+
throw exceptionBuilder()
181+
.withSourceSection(prop.getSourceSection())
182+
.evalError("commandOptionBothFlagAndArgument", prop.getName())
183+
.build();
188184
} else if (argAnnotation != null) {
189185
if (defaultValue != null) {
190-
throw new RuntimeException(
191-
"unexpected default value for @Argument property `XXX`"); // TODO
186+
throw exceptionBuilder()
187+
.withSourceSection(prop.getSourceSection())
188+
.evalError("commandArgumentUnexpectedDefaultValue", prop.getName())
189+
.build();
192190
}
193191
var parseFunction = getParseFunction(argAnnotation);
194192
var type = getOptionType(prop, parseFunction, null);
195193
if (type instanceof OptionType.Map) {
196-
throw new RuntimeException(
197-
"@Argument option cannot have `Map` type for property `XXX`"); // TODO
194+
throw exceptionBuilder()
195+
.withSourceSection(prop.getSourceSection())
196+
.evalError("commandArgumentUnexpectedMapType", prop.getName())
197+
.build();
198198
}
199199
argNames.add(name);
200200
args.add(
@@ -210,7 +210,10 @@ private Pair<List<CommandSpec.Flag>, List<CommandSpec.Argument>> collectOptions(
210210
hide = (Boolean) VmUtils.readMember(flagAnnotation, Identifier.HIDE);
211211
}
212212
if ("help".equals(name) || "h".equals(shortName)) {
213-
throw new RuntimeException("flags may not have name 'help' or short name 'h'"); // TODO
213+
throw exceptionBuilder()
214+
.withSourceSection(prop.getSourceSection())
215+
.evalError("commandFlagHelpCollision", prop.getName())
216+
.build();
214217
}
215218

216219
flagNames.add(name);
@@ -232,7 +235,10 @@ private Pair<List<CommandSpec.Flag>, List<CommandSpec.Argument>> collectOptions(
232235
for (var arg : args) {
233236
if (arg.type() instanceof Collection) {
234237
if (multipleArgName != null) {
235-
throw new RuntimeException("more than one List/Set @Argument found"); // TODO
238+
throw exceptionBuilder()
239+
.withSourceSection(optionsClass.getSourceSection())
240+
.evalError("commandArgumentsMultipleListOrSet")
241+
.build();
236242
}
237243
multipleArgName = arg.name();
238244
}
@@ -249,15 +255,26 @@ private Pair<List<CommandSpec.Flag>, List<CommandSpec.Argument>> collectOptions(
249255
if (bodyNode == null || bodyNode.getBodyNode() instanceof DefaultPropertyBodyNode) {
250256
var typeNode = prop.getTypeNode();
251257
if (typeNode == null) {
252-
throw new RuntimeException("missing type annotation for property `XXX`"); // TODO
258+
throw exceptionBuilder()
259+
.withSourceSection(prop.getSourceSection())
260+
.evalError("commandOptionNoTypeAnnotation", prop.getName(), "")
261+
.withHint("Option properties require type annotations.")
262+
.build();
253263
}
254264

255265
if (stripConstraints(typeNode.export()) instanceof PType.Union union
256266
&& union.getDefaultElement() != null) {
257267
var elements = union.getElementTypes();
258268
for (var element : elements) {
259269
if (!(element instanceof PType.StringLiteral)) {
260-
throw new RuntimeException("Property doesn't have valid option type"); // TODO
270+
throw exceptionBuilder()
271+
.withSourceSection(prop.getSourceSection())
272+
.evalError(
273+
"commandOptionUnsupportedType",
274+
prop.getName(),
275+
"",
276+
typeNode.getSourceSection().getCharacters())
277+
.build();
261278
}
262279
}
263280
return ((PType.StringLiteral) union.getDefaultElement()).getLiteral();
@@ -287,14 +304,19 @@ private OptionType getOptionType(
287304
@Nullable Object defaultValue) {
288305
var typeNode = prop.getTypeNode();
289306
if (typeNode == null) {
290-
throw new RuntimeException("missing type annotation for property `XXX`"); // TODO
307+
throw exceptionBuilder()
308+
.withSourceSection(prop.getSourceSection())
309+
.evalError("commandOptionNoTypeAnnotation", prop.getName(), "")
310+
.build();
291311
}
292312
var type = stripConstraints(typeNode.export());
293313
var required = defaultValue == null;
294314
if (type instanceof PType.Nullable nullable) {
295315
if (!required) {
296-
throw new RuntimeException(
297-
"unexpected nullable type with default for property `XXX`"); // TODO
316+
throw exceptionBuilder()
317+
.withSourceSection(prop.getSourceSection())
318+
.evalError("commandOptionTypeNullableWithDefaultValue", prop.getName())
319+
.build();
298320
}
299321
required = false;
300322
type = nullable.getBaseType();
@@ -303,10 +325,11 @@ private OptionType getOptionType(
303325
return new Primitive(Primitive.Type.STRING, required);
304326
}
305327

306-
return getOptionType(type, required);
328+
return getOptionType(prop, typeNode, type, required);
307329
}
308330

309-
private OptionType getOptionType(PType type, boolean required) {
331+
private OptionType getOptionType(
332+
ClassProperty prop, PropertyTypeNode typeNode, PType type, boolean required) {
310333
type = stripConstraints(type);
311334
if (type instanceof PType.Class classType) {
312335
var clazz = classType.getPClass();
@@ -321,27 +344,39 @@ private OptionType getOptionType(PType type, boolean required) {
321344
} else if (clazz == BaseModule.getStringClass().export()) {
322345
return new Primitive(Primitive.Type.STRING, required);
323346
} else if (clazz == BaseModule.getListClass().export()) {
324-
var elementType = getOptionType(classType.getTypeArguments().get(0), true);
325-
if (!(elementType instanceof Primitive primitive)) {
326-
throw new RuntimeException("Property doesn't have valid option type"); // TODO
347+
var elementType = getOptionType(prop, typeNode, classType.getTypeArguments().get(0), true);
348+
if (!(elementType instanceof Primitive || elementType instanceof Enum)) {
349+
throw exceptionBuilder()
350+
.withSourceSection(prop.getSourceSection())
351+
.evalError("commandOptionUnsupportedType", prop.getName(), "element ", elementType)
352+
.build();
327353
}
328-
return new Collection(Collection.Type.LIST, primitive, required);
354+
return new Collection(Collection.Type.LIST, elementType, required);
329355
} else if (clazz == BaseModule.getSetClass().export()) {
330-
var elementType = getOptionType(classType.getTypeArguments().get(0), true);
331-
if (!(elementType instanceof Primitive primitive)) {
332-
throw new RuntimeException("Property doesn't have valid option type"); // TODO
356+
var elementType = getOptionType(prop, typeNode, classType.getTypeArguments().get(0), true);
357+
if (!(elementType instanceof Primitive || elementType instanceof Enum)) {
358+
throw exceptionBuilder()
359+
.withSourceSection(prop.getSourceSection())
360+
.evalError("commandOptionUnsupportedType", prop.getName(), "element ", elementType)
361+
.build();
333362
}
334-
return new Collection(Collection.Type.SET, primitive, required);
363+
return new Collection(Collection.Type.SET, elementType, required);
335364
} else if (clazz == BaseModule.getMapClass().export()) {
336-
var keyType = getOptionType(classType.getTypeArguments().get(0), true);
337-
if (!(keyType instanceof Primitive primitiveKeyType)) {
338-
throw new RuntimeException("Property doesn't have valid option type"); // TODO
365+
var keyType = getOptionType(prop, typeNode, classType.getTypeArguments().get(0), true);
366+
if (!(keyType instanceof Primitive || keyType instanceof Enum)) {
367+
throw exceptionBuilder()
368+
.withSourceSection(prop.getSourceSection())
369+
.evalError("commandOptionUnsupportedType", prop.getName(), "key ", keyType)
370+
.build();
339371
}
340-
var valueType = getOptionType(classType.getTypeArguments().get(1), true);
341-
if (!(valueType instanceof Primitive primitiveValueType)) {
342-
throw new RuntimeException("Property doesn't have valid option type"); // TODO
372+
var valueType = getOptionType(prop, typeNode, classType.getTypeArguments().get(1), true);
373+
if (!(valueType instanceof Primitive || valueType instanceof Enum)) {
374+
throw exceptionBuilder()
375+
.withSourceSection(prop.getSourceSection())
376+
.evalError("commandOptionUnsupportedType", prop.getName(), "value ", valueType)
377+
.build();
343378
}
344-
return new OptionType.Map(primitiveKeyType, primitiveValueType, required);
379+
return new OptionType.Map(keyType, valueType, required);
345380
}
346381
} else if (type instanceof PType.Alias aliasType) {
347382
var alias = aliasType.getTypeAlias();
@@ -367,14 +402,28 @@ private OptionType getOptionType(PType type, boolean required) {
367402
var choices = new ArrayList<String>(elements.size());
368403
for (var element : elements) {
369404
if (!(element instanceof PType.StringLiteral stringLiteral)) {
370-
throw new RuntimeException("Property doesn't have valid option type"); // TODO
405+
throw exceptionBuilder()
406+
.withSourceSection(prop.getSourceSection())
407+
.evalError(
408+
"commandOptionUnsupportedType",
409+
prop.getName(),
410+
"",
411+
typeNode.getSourceSection().getCharacters())
412+
.build();
371413
}
372414
choices.add(stringLiteral.getLiteral());
373415
}
374416
return new OptionType.Enum(choices, required);
375417
}
376418

377-
throw new RuntimeException("Property doesn't have valid option type"); // TODO
419+
throw exceptionBuilder()
420+
.withSourceSection(prop.getSourceSection())
421+
.evalError(
422+
"commandOptionUnsupportedType",
423+
prop.getName(),
424+
"",
425+
typeNode.getSourceSection().getCharacters())
426+
.build();
378427
}
379428

380429
private static PType stripConstraints(PType type) {
@@ -452,7 +501,13 @@ private List<CommandSpec> collectSubcommands(VmTyped commandInfo) {
452501
(key, member, value) -> {
453502
var spec = parse((VmTyped) value);
454503
if (subcommandNames.contains(spec.name())) {
455-
throw new RuntimeException("command XXX has duplicate subcommands named YYY"); // TODO
504+
throw exceptionBuilder()
505+
.withSourceSection(member.getSourceSection())
506+
.evalError(
507+
"commandSubcommandConflict",
508+
VmUtils.readMember(commandInfo, Identifier.NAME),
509+
spec.name())
510+
.build();
456511
}
457512
subcommandNames.add(spec.name());
458513
subcommands.add(spec);
@@ -630,13 +685,13 @@ private void checkPropertyIsUndefined(VmTyped value, Identifier name) {
630685
if (member != null) {
631686
var memberNode = member.getMemberNode();
632687
if (memberNode == null) {
633-
throw new VmExceptionBuilder() // TODO
634-
.adhocEvalError("Command modules must not assign or amend the `%s` property", name)
688+
throw new VmExceptionBuilder()
689+
.evalError("commandMustNotAssignOrAmendProperty", name)
635690
.withSourceSection(member.getSourceSection())
636691
.build();
637692
} else if (!(memberNode.getBodyNode() instanceof DefaultPropertyBodyNode)) {
638-
throw new VmExceptionBuilder() // TODO
639-
.adhocEvalError("Command modules must not assign or amend the `%s` property", name)
693+
throw new VmExceptionBuilder()
694+
.evalError("commandMustNotAssignOrAmendProperty", name)
640695
.withSourceSection(memberNode.getSourceSection())
641696
.build();
642697
}

‎pkl-core/src/main/resources/org/pkl/core/errorMessages.properties‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1076,3 +1076,44 @@ invalidStringBase64=\
10761076

10771077
characterCodingException=\
10781078
Invalid bytes for charset "{0}".
1079+
1080+
commandSubcommandConflict=\
1081+
Command `{0}` has subcommands with conflicting name "{1}".
1082+
1083+
commandMustNotAssignOrAmendProperty=\
1084+
Commands must not assign or amend property `{0}`.
1085+
1086+
commandOptionNoTypeAnnotation=\
1087+
No type annotation found for `{0}` property{1}.
1088+
1089+
commandOptionsTypeNotClass=\
1090+
Type annotation `{0}` on `options` property in `pkl:Command` subclass must be a class type.
1091+
1092+
commandOptionsTypeAbstractClass=\
1093+
Class `{0}` for type annotation on `options` property in `pkl:Command` subclass must not be abstract.
1094+
1095+
commandOptionBothFlagAndArgument=\
1096+
Found both `@Flag` and `@Argument` annotations for options property `{0}`.\n\
1097+
Only one option type may be specified.
1098+
1099+
commandOptionTypeNullableWithDefaultValue=\
1100+
Unexpected option property `{0}` with nullable type and default value.\n\
1101+
Options with default values must not be nullable.
1102+
1103+
commandOptionUnsupportedType=\
1104+
Option property `{0}` has unsupported {1}type `{2}`.
1105+
1106+
commandArgumentUnexpectedDefaultValue=\
1107+
Unexpected default value for `@Argument` property `{0}`.\n\
1108+
Arguments may not specify a default value.
1109+
1110+
commandArgumentUnexpectedMapType=\
1111+
Unexpected `Map` type for `@Argument` property `{0}`.\n\
1112+
Arguments may not have type `Map`.
1113+
1114+
commandArgumentsMultipleListOrSet=\
1115+
More than one `List` or `Set` argument found.\n\
1116+
Only one argument that accepts multiple values is permitted per command.
1117+
1118+
commandFlagHelpCollision=\
1119+
Flag option `{0}` may not have name "help" or short name "h".

0 commit comments

Comments
 (0)