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.

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

Normalize node paths in errors #229

Merged
merged 6 commits into from
Mar 21, 2024

Conversation

Mingun
Copy link
Contributor

@Mingun Mingun commented Dec 5, 2020

After this change all paths in errors will point to existing nodes in KSY file and node, where error is originated

This PR introduces 10 new failures (76 vs 66) because it changes error messages which is it purpose. So, this PR should be merged together with kaitai-io/kaitai_struct_tests#92 which is fixes test data accordingly.

@Mingun
Copy link
Contributor Author

Mingun commented Mar 7, 2024

@GreyCat, @generalmimon, I see that you have some activity in the project recently. Could you find a time to review my PRs, for example, this?

@Mingun Mingun force-pushed the normalize-error-paths branch 2 times, most recently from 48ff6a9 to 75649e5 Compare March 8, 2024 14:17
Mingun added 5 commits March 21, 2024 21:13
getValueStr, getOptValueStr and getValueIdentifier (which is uses getOptValueStr)
all does this for their own errors therefore this is just a unification of behavior

This change requires corresponding change in kaitai_struct_tests repository

Partially fixes the following error (the first line):
```
[info] - switch_on_malformed *** FAILED ***
[info]   switch_on_malformed.ksy: /seq/0:
[info]   	error: parsing expression '42/' failed on 1:3, expected Expected end-of-input:1:3, found "/"
[info]    did not equal switch_on_malformed.ksy: /seq/0/switch-on:
[info]   	error: parsing expression '42/' failed on 1:3, expected "or" | CharsWhile(Set( , n)) | "\\\n" | End (SimpleMatchers.scala:34)
```
This change requires corresponding change in kaitai_struct_tests repository
TypeValidator.validateSwitchType uses path with "type" therefore this is just behavior unification

This change requires corresponding change in kaitai_struct_tests repository
… type/enum" errors

This change requires corresponding change in kaitai_struct_tests repository
This change requires corresponding change in kaitai_struct_tests repository

Partially fixes the following error (the first line):
```
[info] - attr_invalid_switch_inner *** FAILED ***
[info]   attr_invalid_switch_inner.ksy: /seq/1/type/cases/IntNum(42)/size:
[info]   	error: invalid type: expected integer, got CalcBooleanType
[info]
[info]   attr_invalid_switch_inner.ksy: /seq/1/type/cases/Name(identifier(_))/size:
[info]   	error: invalid type: expected integer, got CalcBooleanType
[info]    did not equal attr_invalid_switch_inner.ksy: /seq/1/size:
[info]   	error: invalid type: expected integer, got CalcBooleanType (SimpleMatchers.scala:34)
```
@Mingun Mingun force-pushed the normalize-error-paths branch from 75649e5 to 4235425 Compare March 21, 2024 16:23
…ould not be located

Also all other error messages starts with lower case, so adjust that's too

Fixes the test:
```
[info] - meta_imports_abs_unknown *** FAILED ***
[info]   meta_imports_abs_unknown: /meta/imports/0:
[info]   	error: Unable to find 'unknown_absolute_name' in import search paths, using: List()
[info]    did not equal meta_imports_abs_unknown.ksy: /meta/imports/0:
[info]   	error: Unable to find 'unknown_absolute_name' in import search paths, using: List() (SimpleMatchers.scala:34)
```
@Mingun Mingun force-pushed the normalize-error-paths branch from 4235425 to bb717cb Compare March 21, 2024 16:30
@GreyCat
Copy link
Member

GreyCat commented Mar 21, 2024

Merging this one as well! Thanks so much for cleaning this up and being persistent with all these rebasing!

@GreyCat GreyCat merged commit b7d5169 into kaitai-io:master Mar 21, 2024
1 of 4 checks passed
@Mingun Mingun deleted the normalize-error-paths branch March 21, 2024 19:53
Copy link
Member

@generalmimon generalmimon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I accidentally left this comment "pending" the whole time and forgot about it, so I'm at least posting it now.

@@ -167,13 +167,13 @@ object AttrSpec {
fromYaml2(srcMap, path, metaDef, id)
} catch {
case (epe: Expressions.ParseException) =>
throw KSYParseError.expression(epe, path)
throw KSYParseError.expression(epe, path ++ List("type"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm skeptical that only the type key is always the culprit of ParseException because fromYaml2 method processes a lot of keys inside.

Upon further inspection, these should be the calls that may emit Expressions.ParseException:

val valid = srcMap.get("valid").map(ValidationSpec.fromYaml(_, path ++ List("valid")))

DataType.fromYaml(
None, path, metaDef, yamlAttrArgs
)

DataType.fromYaml(
Some(simpleType), path, metaDef, yamlAttrArgs
)

But even if it's always the type key, exceptions should be definitely handled closer to where they arise. Quite a few keys processed in fromYaml2 need to parse expressions, but they normally catch it right away (as they should), e.g.:

} catch {
case epe: Expressions.ParseException =>
throw KSYParseError.expression(epe, path)
}

try {
Expressions.parse(condition) -> condType
} catch {
case epe: Expressions.ParseException =>
throw KSYParseError.expression(epe, casePath)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm skeptical that only the type key is always the culprit of ParseException because fromYaml2 method processes a lot of keys inside.

That does not matter. You already inside type key here, and all other keys that fromYaml2 processes here are subkeys of a type.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants