From 639e3eb9d84c732bb98c76eb8d0047eed1ae4a2f Mon Sep 17 00:00:00 2001 From: Mingun Date: Sun, 8 Sep 2024 15:36:49 +0500 Subject: [PATCH 1/4] Remove `id` from paths to instances in style warnings Fixes https://github.com/kaitai-io/kaitai_struct/issues/920 Error count: 62 -> 58 (-4). Fixed: [info] - style_bad_len_inst_pos *** FAILED *** [info] [style_bad_len_inst_pos.ksy: /instances/size_of_foo/id: [info] warning: use `len_foo` instead of `size_of_foo`, given that it's only used as a byte size of `foo` (see https://doc.kaitai.io/ksy_style_guide.html#attr-id) [info] ] [info] did not equal [info] [style_bad_len_inst_pos.ksy: /instances/size_of_foo: [info] warning: use `len_foo` instead of `size_of_foo`, given that it's only used as a byte size of `foo` (see https://doc.kaitai.io/ksy_style_guide.html#attr-id) [info] ] (SimpleMatchers.scala:34) [info] - style_bad_len_inst_value *** FAILED *** [info] [style_bad_len_inst_value.ksy: /instances/size_of_foo/id: [info] warning: use `len_foo` instead of `size_of_foo`, given that it's only used as a byte size of `foo` (see https://doc.kaitai.io/ksy_style_guide.html#attr-id) [info] ] [info] did not equal [info] [style_bad_len_inst_value.ksy: /instances/size_of_foo: [info] warning: use `len_foo` instead of `size_of_foo`, given that it's only used as a byte size of `foo` (see https://doc.kaitai.io/ksy_style_guide.html#attr-id) [info] ] (SimpleMatchers.scala:34) [info] - style_bad_num_inst_pos *** FAILED *** [info] [style_bad_num_inst_pos.ksy: /instances/size_of_foo/id: [info] warning: use `num_foos` instead of `size_of_foo`, given that it's only used as repeat count of `foos` (see https://doc.kaitai.io/ksy_style_guide.html#attr-id) [info] ] [info] did not equal [info] [style_bad_num_inst_pos.ksy: /instances/size_of_foo: [info] warning: use `num_foos` instead of `size_of_foo`, given that it's only used as repeat count of `foos` (see https://doc.kaitai.io/ksy_style_guide.html#attr-id) [info] ] (SimpleMatchers.scala:34) [info] - style_bad_num_inst_value *** FAILED *** [info] [style_bad_num_inst_value.ksy: /instances/size_of_foo/id: [info] warning: use `num_foos` instead of `size_of_foo`, given that it's only used as repeat count of `foos` (see https://doc.kaitai.io/ksy_style_guide.html#attr-id) [info] ] [info] did not equal [info] [style_bad_num_inst_value.ksy: /instances/size_of_foo: [info] warning: use `num_foos` instead of `size_of_foo`, given that it's only used as repeat count of `foos` (see https://doc.kaitai.io/ksy_style_guide.html#attr-id) [info] ] (SimpleMatchers.scala:34) --- .../struct/precompile/StyleCheckIds.scala | 24 +++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/shared/src/main/scala/io/kaitai/struct/precompile/StyleCheckIds.scala b/shared/src/main/scala/io/kaitai/struct/precompile/StyleCheckIds.scala index 4c4753847..e68564131 100644 --- a/shared/src/main/scala/io/kaitai/struct/precompile/StyleCheckIds.scala +++ b/shared/src/main/scala/io/kaitai/struct/precompile/StyleCheckIds.scala @@ -39,16 +39,26 @@ class StyleCheckIds(specs: ClassSpecs) extends PrecompileStep { } } + /** + * @param spec Owner of `attr` + * @param attr Attribute that references attribute with probably not-conformant name + */ def getSizeRefProblem(spec: ClassSpec, attr: MemberSpec): Option[CompilationProblem] = { getSizeReference(spec, attr.dataType).flatMap(sizeRefAttr => { val existingName = sizeRefAttr.id.humanReadable val goodName = s"len_${attr.id.humanReadable}" if (existingName != goodName) { + // Report error at position where referenced attribute is defined. + // Add `id` for attributes in `seq`, do not add for instances + val path = sizeRefAttr match { + case _: InstanceSpec => sizeRefAttr.path + case _ => sizeRefAttr.path :+ "id" + } Some(StyleWarningSizeLen( goodName, existingName, attr.id.humanReadable, - ProblemCoords(path = Some(sizeRefAttr.path ++ List("id"))) + ProblemCoords(path = Some(path)) )) } else { None @@ -56,16 +66,26 @@ class StyleCheckIds(specs: ClassSpecs) extends PrecompileStep { }) } + /** + * @param spec Owner of `attr` + * @param attr Attribute that references attribute with probably not-conformant name + */ def getRepeatExprRefProblem(spec: ClassSpec, attr: AttrLikeSpec): Option[CompilationProblem] = { getRepeatExprReference(spec, attr).flatMap(repeatExprRefAttr => { val existingName = repeatExprRefAttr.id.humanReadable val goodName = s"num_${attr.id.humanReadable}" if (existingName != goodName) { + // Report error at position where referenced attribute is defined. + // Add `id` for attributes in `seq`, do not add for instances + val path = repeatExprRefAttr match { + case _: InstanceSpec => repeatExprRefAttr.path + case _ => repeatExprRefAttr.path :+ "id" + } Some(StyleWarningRepeatExprNum( goodName, existingName, attr.id.humanReadable, - ProblemCoords(path = Some(repeatExprRefAttr.path ++ List("id"))) + ProblemCoords(path = Some(path)) )) } else { None From 6830e6b9d40477ccb69279f20d30d0df8f23dd32 Mon Sep 17 00:00:00 2001 From: Mingun Date: Sun, 8 Sep 2024 16:45:08 +0500 Subject: [PATCH 2/4] Fix paths in "duplicate attribute ID '$ID', previously defined at $PATH" errors Error count: 58 -> 51 (-7). Fixed: [info] - id_clash_params_vs_inst_pos *** FAILED *** [info] [id_clash_params_vs_inst_pos.ksy: /instances/foo: [info] error: duplicate attribute ID 'foo', previously defined at /params/0 [info] ] [info] did not equal [info] [id_clash_params_vs_inst_pos.ksy: /instances/foo: [info] error: duplicate attribute ID 'foo', previously defined at /params/0/id [info] ] (SimpleMatchers.scala:34) [info] - id_clash_params_vs_inst_value *** FAILED *** [info] [id_clash_params_vs_inst_value.ksy: /instances/bar: [info] error: duplicate attribute ID 'bar', previously defined at /params/1 [info] ] [info] did not equal [info] [id_clash_params_vs_inst_value.ksy: /instances/bar: [info] error: duplicate attribute ID 'bar', previously defined at /params/1/id [info] ] (SimpleMatchers.scala:34) [info] - id_clash_params_vs_seq *** FAILED *** [info] [id_clash_params_vs_seq.ksy: /seq/0: [info] error: duplicate attribute ID 'foo', previously defined at /params/1 [info] ] [info] did not equal [info] [id_clash_params_vs_seq.ksy: /seq/0/id: [info] error: duplicate attribute ID 'foo', previously defined at /params/1/id [info] ] (SimpleMatchers.scala:34) [info] - id_clash_seq_vs_inst_pos *** FAILED *** [info] [id_clash_seq_vs_inst_pos.ksy: /instances/foo: [info] error: duplicate attribute ID 'foo', previously defined at /seq/0 [info] ] [info] did not equal [info] [id_clash_seq_vs_inst_pos.ksy: /instances/foo: [info] error: duplicate attribute ID 'foo', previously defined at /seq/0/id [info] ] (SimpleMatchers.scala:34) [info] - id_clash_seq_vs_inst_value *** FAILED *** [info] [id_clash_seq_vs_inst_value.ksy: /instances/bar: [info] error: duplicate attribute ID 'bar', previously defined at /seq/1 [info] ] [info] did not equal [info] [id_clash_seq_vs_inst_value.ksy: /instances/bar: [info] error: duplicate attribute ID 'bar', previously defined at /seq/1/id [info] ] (SimpleMatchers.scala:34) [info] - id_dup_params *** FAILED *** [info] [id_dup_params.ksy: /params/2: [info] error: duplicate attribute ID 'foo', previously defined at /params/0 [info] ] [info] did not equal [info] [id_dup_params.ksy: /params/2/id: [info] error: duplicate attribute ID 'foo', previously defined at /params/0/id [info] ] (SimpleMatchers.scala:34) [info] - id_dup_seq *** FAILED *** [info] [id_dup_seq.ksy: /seq/2: [info] error: duplicate attribute ID 'foo', previously defined at /seq/0 [info] ] [info] did not equal [info] [id_dup_seq.ksy: /seq/2/id: [info] error: duplicate attribute ID 'foo', previously defined at /seq/0/id [info] ] (SimpleMatchers.scala:34) --- .../scala/io/kaitai/struct/format/ClassSpec.scala | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/shared/src/main/scala/io/kaitai/struct/format/ClassSpec.scala b/shared/src/main/scala/io/kaitai/struct/format/ClassSpec.scala index 6a3cb0135..7fabe6a3c 100644 --- a/shared/src/main/scala/io/kaitai/struct/format/ClassSpec.scala +++ b/shared/src/main/scala/io/kaitai/struct/format/ClassSpec.scala @@ -253,9 +253,19 @@ object ClassSpec { private def checkDupId(prevAttrOpt: Option[MemberSpec], id: String, nowAttr: YAMLPath): Unit = { prevAttrOpt match { case Some(prevAttr) => + // Report error at position where referenced param / attribute / instance is defined. + // Add `id` for attributes in `seq` and `params`, do not add for instances + val path = nowAttr match { + case _: InstanceSpec => nowAttr.path + case _ => nowAttr.path :+ "id" + } + val prevPath = prevAttr match { + case _: InstanceSpec => prevAttr.path + case _ => prevAttr.path :+ "id" + } throw KSYParseError.withText( - s"duplicate attribute ID '$id', previously defined at /${prevAttr.pathStr}", - nowAttr.path + s"duplicate attribute ID '$id', previously defined at /${prevPath.mkString("/")}", + path ) case None => // no dups, ok From 4ed59815491a2d26b01445375a51f672b0b311b1 Mon Sep 17 00:00:00 2001 From: Mingun Date: Sun, 8 Sep 2024 16:50:39 +0500 Subject: [PATCH 3/4] Fix path in "this ksy requires compiler version at least $VERSION, but you have $KS_VERSION" error Errors count: 51 -> 50 (-1). Fixed: [info] - meta_high_version *** FAILED *** [info] [meta_high_version.ksy: /meta: [info] error: this ksy requires compiler version at least 99.99.99, but you have $KS_VERSION [info] ] [info] did not equal [info] [meta_high_version.ksy: /meta/ks-version: [info] error: this ksy requires compiler version at least 99.99.99, but you have $KS_VERSION [info] ] (SimpleMatchers.scala:34) --- shared/src/main/scala/io/kaitai/struct/format/MetaSpec.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/shared/src/main/scala/io/kaitai/struct/format/MetaSpec.scala b/shared/src/main/scala/io/kaitai/struct/format/MetaSpec.scala index d6d1d44aa..9c289f26e 100644 --- a/shared/src/main/scala/io/kaitai/struct/format/MetaSpec.scala +++ b/shared/src/main/scala/io/kaitai/struct/format/MetaSpec.scala @@ -92,7 +92,7 @@ object MetaSpec { ParseUtils.getOptValueStr(srcMap, "ks-version", path).foreach { (verStr) => val ver = KSVersion.fromStr(verStr) if (ver > KSVersion.current) - throw KSYParseError.incompatibleVersion(ver, KSVersion.current, path) + throw KSYParseError.incompatibleVersion(ver, KSVersion.current, path :+ "ks-version") } val endian: Option[Endianness] = Endianness.fromYaml(srcMap.get("endian"), path) From 8b67e13846f369688442fc160398365955242558 Mon Sep 17 00:00:00 2001 From: Mingun Date: Tue, 10 Sep 2024 21:20:25 +0500 Subject: [PATCH 4/4] Fix duplication of `type` in the path of `switch-on` types in errors Error count: 50 -> 49 (-1). Fixed: [info] - type_unknown_switch *** FAILED *** [info] [type_unknown_switch.ksy: /seq/0/type/cases/IntNum(42)/type: [info] error: unable to find type 'some_unknown_name', searching from type_unknown_switch [info] ] [info] did not equal [info] [type_unknown_switch.ksy: /seq/0/type/cases/IntNum(42): [info] error: unable to find type 'some_unknown_name', searching from type_unknown_switch [info] ] (SimpleMatchers.scala:34) --- .../struct/precompile/ResolveTypes.scala | 32 +++++++++++++++---- 1 file changed, 26 insertions(+), 6 deletions(-) diff --git a/shared/src/main/scala/io/kaitai/struct/precompile/ResolveTypes.scala b/shared/src/main/scala/io/kaitai/struct/precompile/ResolveTypes.scala index f6f0c888b..f366a7a53 100644 --- a/shared/src/main/scala/io/kaitai/struct/precompile/ResolveTypes.scala +++ b/shared/src/main/scala/io/kaitai/struct/precompile/ResolveTypes.scala @@ -41,27 +41,47 @@ class ResolveTypes(specs: ClassSpecs, topClass: ClassSpec, opaqueTypes: Boolean) } def resolveUserTypeForMember(curClass: ClassSpec, attr: MemberSpec): Iterable[CompilationProblem] = - resolveUserType(curClass, attr.dataType, attr.path) + resolveUserType(curClass, attr.dataType, attr.path, None) - def resolveUserType(curClass: ClassSpec, dataType: DataType, path: List[String]): Iterable[CompilationProblem] = { + /** + * Resolves the type of the `dataType` of an attribute defined in `curClass`. + * + * @param curClass The owner of attribute which type is being resolved + * @param dataType The type of the attribute being resolved + * @param path YAML path to the attribute where diagnostics should bу reported + * @param caseExpr If attribute type is switchable type then contains the specific case name + */ + def resolveUserType( + curClass: ClassSpec, + dataType: DataType, + path: List[String], + caseExpr: Option[String], + ): Iterable[CompilationProblem] = { dataType match { case ut: UserType => - val (resClassSpec, problems) = resolveUserType(curClass, ut.name, path ++ List("type")) + val (resClassSpec, problems) = resolveUserType( + curClass, + ut.name, + caseExpr match { + case Some(case_) => path ++ List("type", "cases", case_) + case None => path :+ "type" + } + ) ut.classSpec = resClassSpec problems case et: EnumType => et.enumSpec = resolveEnumSpec(curClass, et.name) if (et.enumSpec.isEmpty) { - Some(EnumNotFoundErr(et.name, curClass, path ++ List("enum"))) + Some(EnumNotFoundErr(et.name, curClass, path :+ "enum")) } else { None } case st: SwitchType => st.cases.flatMap { case (caseName, ut) => - resolveUserType(curClass, ut, path ++ List("type", "cases", caseName.toString)) + resolveUserType(curClass, ut, path, Some(caseName.toString)) } case at: ArrayType => - resolveUserType(curClass, at.elType, path) + resolveUserType(curClass, at.elType, path, caseExpr) case _ => // not a user type, nothing to resolve None