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.

Sign up for GitHub

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

Jump to bottom

Fix reporting of compilation problems in imported .ksy specs #1063

Open
generalmimon opened this issue Aug 18, 2023 · 0 comments
Open

Fix reporting of compilation problems in imported .ksy specs #1063

generalmimon opened this issue Aug 18, 2023 · 0 comments
Labels
bug found

Comments

@generalmimon
Copy link
Member

generalmimon commented Aug 18, 2023 •
edited
Loading

Consider the following cases when an imported spec contains problems (the compiler version used was the latest KSC 0.11-SNAPSHOT built from master - 2b84e0a1):

  1. imports_err_fatal importing attr_bad_id with a fatal error:

    imports_err_fatal.ksy

    meta:
  id: imports_err_fatal
  imports:
    - attr_bad_id

    attr_bad_id.ksy

    meta:
  id: attr_bad_id
seq:
  - id: BAD # error: invalid attribute ID: 'BAD', (...)
    $ kaitai-struct-compiler -- -d outdir -t ruby imports_err_fatal.ksy
imports_err_fatal: /meta/imports/0:
	error: (main): /seq/0/id:
	error: invalid attribute ID: 'BAD', expected /^[a-z][a-z0-9_]*$/

  2. imports_err_nonfatal importing type_unknown with a reference to unknown type:

    imports_err_nonfatal.ksy

    meta:
  id: imports_err_nonfatal
  imports:
    - type_unknown

    type_unknown.ksy

    meta:
  id: type_unknown
  ks-opaque-types: false
seq:
  - id: foo
    type: some_unknown_name # error: unable to find type 'some_unknown_name', (...)
    $ kaitai-struct-compiler -- -d outdir -t ruby imports_err_nonfatal.ksy
./type_unknown.ksy: /seq/0:
        error: unable to find type 'some_unknown_name', searching from type_unknown

./type_unknown.ksy: /seq/0:
        error: unable to find type 'some_unknown_name', searching from type_unknown

  3. imports_warn importing attr_bad_id with a warning:

    imports_warn.ksy

    meta:
  id: imports_warn
  imports:
    - style_bad_len

    style_bad_len.ksy

    meta:
  id: style_bad_len
seq:
  - id: size_of_foo # warning: use `len_foo` instead of `size_of_foo`, (...)
    type: u1
  - id: foo
    size: size_of_foo
    $ kaitai-struct-compiler -- -d outdir -t ruby imports_warn.ksy
./style_bad_len.ksy: /seq/0/id:
	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)

./style_bad_len.ksy: /seq/0/id:
	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)

You can see that in all these cases, the compiler doesn't report the given problem as expected.

The compiler report in case of imports_err_fatal.ksy is missing the name of the attr_bad_id.ksy spec which actually contains the error (we can see the (main) placeholder instead), and I don't think the imports_err_fatal: /meta/imports/0: part is actually necessary or helpful to diagnose the error. I think it's enough to report that the imported attr_bad_id.ksy spec contains the error, and whether it was imported or not and from where it was imported is irrelevant - it would contain the same error even if it was compiled standalone (i.e. not imported).

Note that warnings and certain non-fatal errors (as in the imports_err_nonfatal case) in an imported spec are reported with just the location info of the particular warning, i.e. without the info about where the spec was imported from - so I think all errors should be reported in the same way.

In imports_err_nonfatal.ksy and imports_warn.ksy cases, the same error/warning is duplicated. Well, apparently the imported .ksy specs are validated twice, but I haven't dug into the code yet, so I don't know why exactly this happens.
generalmimon added a commit to kaitai-io/kaitai_struct_compiler that referenced this issue Mar 4, 2024
@generalmimon
Fix ks-opaque-types: true and duplicate warnings if imports are used
869f551 
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.
@generalmimon generalmimon mentioned this issue Mar 4, 2024
Merged
generalmimon added a commit to kaitai-io/kaitai_struct_compiler that referenced this issue Mar 4, 2024
@generalmimon
Fix ks-opaque-types: true and duplicate warnings if imports are used
143adc9 
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.
generalmimon added a commit to kaitai-io/kaitai_struct_compiler that referenced this issue Mar 7, 2024
@generalmimon
Fix ks-opaque-types: true and duplicate warnings if imports are used
e2dc7f5 
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug found
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@generalmimon