Fix ks-opaque-types: true and duplicate warnings if imports are used
#267
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
generalmimon
force-pushed
the
fix-ks-opaque-types-incompat-with-imports
branch
from
869f551 to
143adc9
Compare
GreyCat
approved these changes
Mar 5, 2024
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 is more than 1 type 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#1063). 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` processed 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.
generalmimon
force-pushed
the
fix-ks-opaque-types-incompat-with-imports
branch
from
143adc9 to
e2dc7f5
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 thetopClassspecified, but its actual implementation for the most part didn't usetopClassat all and only passedclassSpecsto all precompilation steps, which processed all types in the givenClassSpecs. Given that theprecompile(classSpecs: ClassSpecs, topClass: ClassSpec)overload is called from theprecompile(specs: ClassSpecs)overload for each type inClassSpecs, this resulted in unnecessary repeated precompilation of all types when imports are used (because then there is more than 1 type inClassSpecs).If there are
Ntop-level types inClassSpecs, instead of running the precompile phase only once for each top-level type, it was runNtimes per each top-level type, resulting inN**2total 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 consoleNtimes (andN >= 2any time you use imports) instead of just once (see kaitai-io/kaitai_struct#1063).Another problem was caused by the only actual use of
topClassin theprecompile(classSpecs: ClassSpecs, topClass: ClassSpec)overload, namely for checking if opaque types are enabled or not (according to the/meta/ks-opaque-typeskey) and passing the setting to theResolveTypesprecompile 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 (becauseResolveTypesprocessed 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
ClassSpecofClassSpecsis precompiled only once, eliminating both of the problems mentioned.