Skip to content

Commit

Permalink
Fix ks-opaque-types: true and duplicate warnings if imports are used
Browse files Browse the repository at this point in the history
Fixes kaitai-io/kaitai_struct#295

Fixes duplication of warnings and non-fatal errors as shown in
kaitai-io/kaitai_struct#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 kaitai-io/kaitai_struct#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
kaitai-io/kaitai_struct#295.

This commit ensures that each `ClassSpec` of `ClassSpecs` is precompiled
only once, eliminating both of the problems mentioned.
  • Loading branch information
generalmimon committed Mar 4, 2024
1 parent 7be4a2b commit 143adc9
Show file tree
Hide file tree
Showing 4 changed files with 27 additions and 35 deletions.
44 changes: 16 additions & 28 deletions shared/src/main/scala/io/kaitai/struct/Main.scala
Original file line number Diff line number Diff line change
Expand Up @@ -31,50 +31,38 @@ 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
if (resolveTypeProblems.nonEmpty) {
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
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)

/**
Expand Down

0 comments on commit 143adc9

Please sign in to comment.