Remove `id` from paths to instances in style warnings #306

Mingun · 2024-09-08T10:38:00Z

It seems relatively new error, but the code was not changed since 2021...

Fixes errors like:

[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)

/instances/size_of_foo/id is not-existing path. Instances does not have id, the name of instance is its id.

Fixes kaitai-io/kaitai_struct#920.

Also fixes a couple of other errors

generalmimon · 2024-09-08T10:47:51Z

@Mingun:

It seems relatively new error

It is. I recently extended the formats_err test suite: kaitai-io/kaitai_struct_tests@c5f366f...0427d48

The tests you're fixing here were added in kaitai-io/kaitai_struct_tests@c911e0a to demonstrate kaitai-io/kaitai_struct#920. So ideally you should add Fixes https://github.com/kaitai-io/kaitai_struct/issues/920 to the PR description and commit message.

Mingun · 2024-09-10T16:23:33Z

@generalmimon, could you look again? This PR fixes 13 errors

Mingun · 2025-08-22T08:39:43Z

@GreyCat, @generalmimon , this PR reduces error count from currently 54 to 42. It is small win, do you mind to merge it?

GreyCat

Generally, looks good to me and relatively straightforward fix! Thanks @Mingun!

@generalmimon, do you have any concerns or shall we merge this?

generalmimon

I think it's fine too, it indeed fixes some of the test failures.

You know I'm not a big fan of adding comments unless it's absolutely necessary, but if you do, I'd prefer them to make sense...

shared/src/main/scala/io/kaitai/struct/precompile/StyleCheckIds.scala

shared/src/main/scala/io/kaitai/struct/precompile/ResolveTypes.scala

generalmimon · 2025-08-23T13:23:53Z

shared/src/main/scala/io/kaitai/struct/precompile/ResolveTypes.scala


-  def resolveUserType(curClass: ClassSpec, dataType: DataType, path: List[String]): Iterable[CompilationProblem] = {
+  /**
+    * Resolves the type of the `dataType` of an attribute defined in `curClass`.


I'm not sure what "the type of the dataType" means, maybe just

Suggested change

* Resolves the type of the `dataType` of an attribute defined in `curClass`.

* Resolves the `dataType` of an attribute defined in `curClass`.

...?

It is reference to user-defined class (and maybe to built-in type? Don't remember. Affected by the lack of documentation, so I cannot agree with your statement that something is obvious here...). By the way, the whole procedure looks complicated and buggy, but it is fixed in #309

Fixes kaitai-io/kaitai_struct#920 Error count: 54 -> 50 (-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)

…TH" errors Error count: 50 -> 43 (-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)

Error count: 43 -> 42 (-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)

Mingun · 2025-08-23T14:18:14Z

@generalmimon, your suggestion applied

Mingun marked this pull request as draft September 8, 2024 11:11

Mingun force-pushed the fix-excess-id-in-path branch from 533c134 to 4ed5981 Compare September 8, 2024 11:53

Mingun marked this pull request as ready for review September 8, 2024 11:54

Mingun force-pushed the fix-excess-id-in-path branch 2 times, most recently from d2a946b to af582ae Compare August 21, 2025 18:34

GreyCat requested review from GreyCat and generalmimon August 23, 2025 09:15

GreyCat approved these changes Aug 23, 2025

View reviewed changes

generalmimon approved these changes Aug 23, 2025

View reviewed changes

Mingun added 4 commits August 23, 2025 19:17

Use concatenation with one element instead of list with one element

ef15445

Mingun force-pushed the fix-excess-id-in-path branch from af582ae to b868509 Compare August 23, 2025 14:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Remove `id` from paths to instances in style warnings #306

Remove `id` from paths to instances in style warnings #306

Uh oh!

Mingun commented Sep 8, 2024 •

edited

Loading

Uh oh!

generalmimon commented Sep 8, 2024 •

edited

Loading

Uh oh!

Mingun commented Sep 10, 2024

Uh oh!

Mingun commented Aug 22, 2025

Uh oh!

GreyCat left a comment

Uh oh!

generalmimon left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

generalmimon Aug 23, 2025

Uh oh!

Mingun Aug 23, 2025

Uh oh!

Mingun commented Aug 23, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	* Resolves the type of the `dataType` of an attribute defined in `curClass`.
	* Resolves the `dataType` of an attribute defined in `curClass`.

Remove id from paths to instances in style warnings #306

Are you sure you want to change the base?

Remove id from paths to instances in style warnings #306

Uh oh!

Conversation

Mingun commented Sep 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

generalmimon commented Sep 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mingun commented Sep 10, 2024

Uh oh!

Mingun commented Aug 22, 2025

Uh oh!

GreyCat left a comment

Choose a reason for hiding this comment

Uh oh!

generalmimon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

generalmimon Aug 23, 2025

Choose a reason for hiding this comment

Uh oh!

Mingun Aug 23, 2025

Choose a reason for hiding this comment

Uh oh!

Mingun commented Aug 23, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Remove `id` from paths to instances in style warnings #306

Remove `id` from paths to instances in style warnings #306

Mingun commented Sep 8, 2024 •

edited

Loading

generalmimon commented Sep 8, 2024 •

edited

Loading