Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Normalize node paths in errors #229

Merged
merged 6 commits into from
Mar 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ class JavaClassSpecs(relPath: String, absPaths: Seq[String], firstSpec: ClassSpe
}
}
}
throw new FileNotFoundException(s"Unable to find '$name' in import search paths, using: $absPaths")
throw new FileNotFoundException(s"unable to find '$name.ksy' in import search paths, using: $absPaths")
}
}

Expand Down
7 changes: 3 additions & 4 deletions shared/src/main/scala/io/kaitai/struct/format/AttrSpec.scala
Original file line number Diff line number Diff line change
Expand Up @@ -167,14 +167,13 @@ object AttrSpec {
fromYaml2(srcMap, path, metaDef, id)
} catch {
case (epe: Expressions.ParseException) =>
throw KSYParseError.expression(epe, path)
throw KSYParseError.expression(epe, path ++ List("type"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm skeptical that only the type key is always the culprit of ParseException because fromYaml2 method processes a lot of keys inside.

Upon further inspection, these should be the calls that may emit Expressions.ParseException:

val valid = srcMap.get("valid").map(ValidationSpec.fromYaml(_, path ++ List("valid")))

DataType.fromYaml(
None, path, metaDef, yamlAttrArgs
)

DataType.fromYaml(
Some(simpleType), path, metaDef, yamlAttrArgs
)

But even if it's always the type key, exceptions should be definitely handled closer to where they arise. Quite a few keys processed in fromYaml2 need to parse expressions, but they normally catch it right away (as they should), e.g.:

} catch {
case epe: Expressions.ParseException =>
throw KSYParseError.expression(epe, path)
}

try {
Expressions.parse(condition) -> condType
} catch {
case epe: Expressions.ParseException =>
throw KSYParseError.expression(epe, casePath)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm skeptical that only the type key is always the culprit of ParseException because fromYaml2 method processes a lot of keys inside.

That does not matter. You already inside type key here, and all other keys that fromYaml2 processes here are subkeys of a type.

}
}

def fromYaml2(srcMap: Map[String, Any], path: List[String], metaDef: MetaSpec, id: Identifier): AttrSpec = {
val doc = DocSpec.fromYaml(srcMap, path)
val process = ProcessExpr.fromStr(ParseUtils.getOptValueStr(srcMap, "process", path), path)
// TODO: add proper path propagation
val process = ProcessExpr.fromStr(ParseUtils.getOptValueStr(srcMap, "process", path), path ++ List("process"))
val contents = srcMap.get("contents").map(parseContentSpec(_, path ++ List("contents")))
val size = ParseUtils.getOptValueExpression(srcMap, "size", path)
val sizeEos = ParseUtils.getOptValueBool(srcMap, "size-eos", path).getOrElse(false)
Expand Down Expand Up @@ -222,7 +221,7 @@ object AttrSpec {
)
case switchMap: Map[Any, Any] =>
val switchMapStr = ParseUtils.anyMapToStrMap(switchMap, path)
parseSwitch(switchMapStr, path, metaDef, yamlAttrArgs)
parseSwitch(switchMapStr, path ++ List("type"), metaDef, yamlAttrArgs)
case unknown =>
throw KSYParseError.withText(s"expected map or string, found $unknown", path ++ List("type"))
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ object ClassSpec {
if (path.isEmpty) {
explicitMeta.id match {
case None =>
throw KSYParseError.withText("no `meta/id` encountered in top-level class spec", path ++ List("meta", "id"))
throw KSYParseError.withText("no `meta/id` encountered in top-level class spec", path ++ List("meta"))
case Some(id) =>
cs.name = List(id)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ object ParseUtils {
case Some(value) =>
asStr(value, path ++ List(field))
case None =>
throw KSYParseError.noKey(path ++ List(field))
throw KSYParseError.noKey(field, path)
}
}

Expand All @@ -35,7 +35,7 @@ object ParseUtils {
case Some(value) =>
asMapStrStr(value, path ++ List(field))
case None =>
throw KSYParseError.noKey(path ++ List(field))
throw KSYParseError.noKey(field, path)
}
}

Expand Down Expand Up @@ -90,7 +90,7 @@ object ParseUtils {
Expressions.parse(getValueStr(src, field, path))
} catch {
case epe: Expressions.ParseException =>
throw KSYParseError.expression(epe, path)
throw KSYParseError.expression(epe, path ++ List(field))
}
}

Expand All @@ -99,7 +99,7 @@ object ParseUtils {
getOptValueStr(src, field, path).map(Expressions.parse)
} catch {
case epe: Expressions.ParseException =>
throw KSYParseError.expression(epe, path)
throw KSYParseError.expression(epe, path ++ List(field))
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,13 +46,13 @@ class ResolveTypes(specs: ClassSpecs, topClass: ClassSpec, opaqueTypes: Boolean)
def resolveUserType(curClass: ClassSpec, dataType: DataType, path: List[String]): Iterable[CompilationProblem] = {
dataType match {
case ut: UserType =>
val (resClassSpec, problems) = resolveUserType(curClass, ut.name, path)
val (resClassSpec, problems) = resolveUserType(curClass, ut.name, path ++ List("type"))
ut.classSpec = resClassSpec
problems
case et: EnumType =>
et.enumSpec = resolveEnumSpec(curClass, et.name)
if (et.enumSpec.isEmpty) {
Some(EnumNotFoundErr(et.name, curClass, path))
Some(EnumNotFoundErr(et.name, curClass, path ++ List("enum")))
} else {
None
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,11 @@ class TypeValidator(specs: ClassSpecs) extends PrecompileStep {
} else {
None
}
val problems2 = validateDataType(caseType, casePath)
// All properties of types is declared on the common level for all variants so
// we don't use `casePath` here
// FIXME: We need to filter repeated errors here, because some errors influences
// many cases
val problems2 = validateDataType(caseType, path)
problems1 ++ problems2
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,8 @@ object KSYParseError {
def withText(text: String, path: List[String]): CompilationProblemException =
KSYParseError(text, path).toException

def noKey(path: List[String]) =
withText(s"missing mandatory argument `${path.last}`", path)
def noKey(field: String, path: List[String]) =
withText(s"missing mandatory argument `$field`", path)

def noKeys(path: List[String], expectedKeys: Set[String]) =
withText(s"expected any of ${expectedKeys.toList.sorted.mkString(", ")}, found none", path)
Expand Down
Loading