From 143adc9e465afd97acd28651bbb377cb2a496566 Mon Sep 17 00:00:00 2001 From: Petr Pucil Date: Mon, 4 Mar 2024 15:47:37 +0100 Subject: [PATCH] Fix `ks-opaque-types: true` and duplicate warnings if imports are used Fixes https://github.com/kaitai-io/kaitai_struct/issues/295 Fixes duplication of warnings and non-fatal errors as shown in https://github.com/kaitai-io/kaitai_struct/issues/1063 The existing code structure hinted that the `precompile(classSpecs: ClassSpecs, topClass: ClassSpec)` overload should precompile only the `topClass` specified, but its actual implementation for the most part didn't use `topClass` at all and only passed `classSpecs` to all precompilation steps, which processed all types in the given `ClassSpecs`. Given that the `precompile(classSpecs: ClassSpecs, topClass: ClassSpec)` overload is called from the `precompile(specs: ClassSpecs)` overload for each type in `ClassSpecs`, this resulted in unnecessary repeated precompilation of all types when imports are used (because then there are more types in `ClassSpecs`). If there are `N` top-level types in `ClassSpecs`, instead of running the precompile phase only once for each top-level type, it was run `N` times per each top-level type, resulting in `N**2` total precompilations of top-level types. This is not only unnecessary, but it also had some observable negative effects, e.g. a single warning being reported to the console `N` times (and `N >= 2` any time you use imports) instead of just once (see https://github.com/kaitai-io/kaitai_struct/issues/295). Another problem was caused by the only actual use of `topClass` in the `precompile(classSpecs: ClassSpecs, topClass: ClassSpec)` overload, namely for checking if opaque types are enabled or not (according to the `/meta/ks-opaque-types` key) and passing the setting to the `ResolveTypes` precompile step. This meant that if one spec had opaque types enabled and the other disabled, they would be rejected even in the spec where they should be enabled (because `ResolveTypes` processes all specs first with opaque types enabled and then once more with them disabled), as reported in https://github.com/kaitai-io/kaitai_struct/issues/295. This commit ensures that each `ClassSpec` of `ClassSpecs` is precompiled only once, eliminating both of the problems mentioned. --- .../main/scala/io/kaitai/struct/Main.scala | 44 +++++++------------ .../struct/precompile/ResolveTypes.scala | 5 ++- .../struct/precompile/StyleCheckIds.scala | 9 ++-- .../struct/precompile/TypeValidator.scala | 4 +- 4 files changed, 27 insertions(+), 35 deletions(-) diff --git a/shared/src/main/scala/io/kaitai/struct/Main.scala b/shared/src/main/scala/io/kaitai/struct/Main.scala index 5a1ceefc5..0ab95da3b 100644 --- a/shared/src/main/scala/io/kaitai/struct/Main.scala +++ b/shared/src/main/scala/io/kaitai/struct/Main.scala @@ -31,33 +31,19 @@ object Main { /** * Runs precompilation steps on every type in the given [[format.ClassSpecs]] collection, - * using provided configuration. + * using provided configuration. See individual precompile steps invocations for more + * in-depth description of what each step includes. * * @param specs [[format.ClassSpecs]] container with all types loaded into it * @param config runtime configuration to be passed to precompile steps * @return a list of compilation problems encountered during precompilation steps */ - def precompile(specs: ClassSpecs, config: RuntimeConfig): Iterable[CompilationProblem] = { - specs.flatMap { case (_, classSpec) => - precompile(specs, classSpec, config) + def precompile(specs: ClassSpecs, conf: RuntimeConfig): Iterable[CompilationProblem] = { + new MarkupClassNames(specs).run() + val resolveTypeProblems = specs.flatMap { case (_, topClass) => + val config = updateConfigFromMeta(conf, topClass.meta) + new ResolveTypes(specs, topClass, config.opaqueTypes).run() } - } - - /** - * Does all precompiles steps on a single [[format.ClassSpec]] using provided configuration. - * See individual precompile steps invocations for more in-depth description of - * what each step includes. - * - * @param classSpecs [[format.ClassSpecs]] container with all types loaded into it - * @param topClass one top type to precompile - * @param conf runtime configuration to be passed to precompile steps - * @return a list of compilation problems encountered during precompilation steps - */ - def precompile(classSpecs: ClassSpecs, topClass: ClassSpec, conf: RuntimeConfig): Iterable[CompilationProblem] = { - val config = updateConfigFromMeta(conf, topClass.meta) - - new MarkupClassNames(classSpecs).run() - val resolveTypeProblems = new ResolveTypes(classSpecs, config.opaqueTypes).run() // For now, bail out early in case we have any type resolution problems // TODO: try to recover and do some more passes even in face of these @@ -65,16 +51,18 @@ object Main { return resolveTypeProblems } - new ParentTypes(classSpecs).run() - new SpecsValueTypeDerive(classSpecs).run() - new CalculateSeqSizes(classSpecs).run() - val typeValidatorProblems = new TypeValidator(classSpecs, topClass).run() + new ParentTypes(specs).run() + new SpecsValueTypeDerive(specs).run() + new CalculateSeqSizes(specs).run() + val typeValidatorProblems = new TypeValidator(specs).run() // Warnings - val styleWarnings = new StyleCheckIds(classSpecs, topClass).run() - val encodingProblems = new CanonicalizeEncodingNames(classSpecs).run() + val styleWarnings = new StyleCheckIds(specs).run() + val encodingProblems = new CanonicalizeEncodingNames(specs).run() - topClass.parentClass = GenericStructClassSpec + specs.forEachTopLevel((_, spec) => { + spec.parentClass = GenericStructClassSpec + }) resolveTypeProblems ++ typeValidatorProblems ++ styleWarnings ++ encodingProblems } 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 01b7f494c..23730902e 100644 --- a/shared/src/main/scala/io/kaitai/struct/precompile/ResolveTypes.scala +++ b/shared/src/main/scala/io/kaitai/struct/precompile/ResolveTypes.scala @@ -10,8 +10,9 @@ import io.kaitai.struct.problems._ * A collection of methods that resolves user types and enum types, i.e. * converts names into ClassSpec / EnumSpec references. */ -class ResolveTypes(specs: ClassSpecs, opaqueTypes: Boolean) extends PrecompileStep { - override def run(): Iterable[CompilationProblem] = specs.mapRec(resolveUserTypes) +class ResolveTypes(specs: ClassSpecs, topClass: ClassSpec, opaqueTypes: Boolean) extends PrecompileStep { + override def run(): Iterable[CompilationProblem] = + topClass.mapRec(resolveUserTypes).map(problem => problem.localizedInType(topClass)) /** * Resolves user types and enum types recursively starting from a certain 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 fd0752b28..4c4753847 100644 --- a/shared/src/main/scala/io/kaitai/struct/precompile/StyleCheckIds.scala +++ b/shared/src/main/scala/io/kaitai/struct/precompile/StyleCheckIds.scala @@ -6,11 +6,14 @@ import io.kaitai.struct.exprlang.Ast import io.kaitai.struct.format._ import io.kaitai.struct.problems._ -class StyleCheckIds(specs: ClassSpecs, topClass: ClassSpec) extends PrecompileStep { - val provider = new ClassTypeProvider(specs, topClass) +class StyleCheckIds(specs: ClassSpecs) extends PrecompileStep { + val provider = new ClassTypeProvider(specs, specs.firstSpec) override def run(): Iterable[CompilationProblem] = - specs.mapRec(processType) + specs.mapTopLevel((_, typeSpec) => { + provider.topClass = typeSpec + typeSpec.mapRec(processType) + }) def processType(spec: ClassSpec): Iterable[CompilationProblem] = { provider.nowClass = spec diff --git a/shared/src/main/scala/io/kaitai/struct/precompile/TypeValidator.scala b/shared/src/main/scala/io/kaitai/struct/precompile/TypeValidator.scala index 036fcbceb..d752619c1 100644 --- a/shared/src/main/scala/io/kaitai/struct/precompile/TypeValidator.scala +++ b/shared/src/main/scala/io/kaitai/struct/precompile/TypeValidator.scala @@ -17,8 +17,8 @@ import scala.reflect.ClassTag * @param specs bundle of class specifications (used only to find external references) * @param topClass class to start check with */ -class TypeValidator(specs: ClassSpecs, topClass: ClassSpec) extends PrecompileStep { - val provider = new ClassTypeProvider(specs, topClass) +class TypeValidator(specs: ClassSpecs) extends PrecompileStep { + val provider = new ClassTypeProvider(specs, specs.firstSpec) val detector = new ExpressionValidator(provider) /**