Skip to content

Commit

Permalink
Ensure that contents and valid apply to individual elements
Browse files Browse the repository at this point in the history
Resolves kaitai-io/kaitai_struct#1117

This is how the `contents` key worked until KS 0.8, but then in KS 0.9,
`contents` started to be converted to the newly introduced `valid`,
which was applied only once at the end on the final value. This means
that it was applied on the entire array in the case of repeated fields.
However, the behavior of `valid` + `repeat` wasn't tested anywhere, so I
think it was an oversight. Note that the Kaitai Struct language doesn't
define an equality `==` operation for true arrays, so it wouldn't even
work. Running the `valid` and `contents` checks on each element is more
flexible (and also seems more useful).

Fixes the following tests for all languages except Go (will be fixed in
the following commit), Nim, Perl and Construct:

* ValidFailRepeatAnyofInt
* ValidFailRepeatContents
* ValidFailRepeatEqInt
* ValidFailRepeatExpr
* ValidFailRepeatInst
* ValidFailRepeatMaxInt
* ValidFailRepeatMinInt

These tests were added in
kaitai-io/kaitai_struct_tests@98a68fc
  • Loading branch information
generalmimon committed Jul 16, 2024
1 parent 06f5658 commit 7154301
Show file tree
Hide file tree
Showing 3 changed files with 34 additions and 21 deletions.
13 changes: 13 additions & 0 deletions shared/src/main/scala/io/kaitai/struct/format/Identifier.scala
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,19 @@ object Identifier {
}
}

def itemExpr(id: Identifier, rep: RepeatSpec): Ast.expr = {
val astId = Ast.expr.InternalName(id)
rep match {
case NoRepeat =>
astId
case _ =>
Ast.expr.Subscript(
astId,
Ast.expr.Name(Ast.identifier(Identifier.INDEX))
)
}
}

// Constants for special names used in expression language
val ROOT = "_root"
val PARENT = "_parent"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,6 @@ trait CommonReads extends LanguageCompiler {
case _ => // no seeking required for sequence attributes
}

// Run validations (still inside "if", if applicable)
attrValidateAll(attr)

attrParseIfFooter(attr.cond.ifExpr)
}

Expand All @@ -79,6 +76,7 @@ trait CommonReads extends LanguageCompiler {
case NoRepeat =>
}
attrParse2(id, attr.dataType, io, attr.cond.repeat, false, defEndian)
attrValidateAll(attr)
attr.cond.repeat match {
case RepeatEos =>
condRepeatEosFooter
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,22 +14,23 @@ trait ValidateOps extends ExceptionNames {
val typeProvider: ClassTypeProvider

def attrValidate(attrId: Identifier, attr: AttrLikeSpec, valid: ValidationSpec): Unit = {
val itemValue = Identifier.itemExpr(attrId, attr.cond.repeat)
valid match {
case ValidationEq(expected) =>
attrValidateExprCompare(attrId, attr, Ast.cmpop.Eq, expected, ValidationNotEqualError(attr.dataTypeComposite))
attrValidateExprCompare(attrId, attr, Ast.cmpop.Eq, expected, ValidationNotEqualError(attr.dataType))
case ValidationMin(min) =>
attrValidateExprCompare(attrId, attr, Ast.cmpop.GtE, min, ValidationLessThanError(attr.dataTypeComposite))
attrValidateExprCompare(attrId, attr, Ast.cmpop.GtE, min, ValidationLessThanError(attr.dataType))
case ValidationMax(max) =>
attrValidateExprCompare(attrId, attr, Ast.cmpop.LtE, max, ValidationGreaterThanError(attr.dataTypeComposite))
attrValidateExprCompare(attrId, attr, Ast.cmpop.LtE, max, ValidationGreaterThanError(attr.dataType))
case ValidationRange(min, max) =>
attrValidateExprCompare(attrId, attr, Ast.cmpop.GtE, min, ValidationLessThanError(attr.dataTypeComposite))
attrValidateExprCompare(attrId, attr, Ast.cmpop.LtE, max, ValidationGreaterThanError(attr.dataTypeComposite))
attrValidateExprCompare(attrId, attr, Ast.cmpop.GtE, min, ValidationLessThanError(attr.dataType))
attrValidateExprCompare(attrId, attr, Ast.cmpop.LtE, max, ValidationGreaterThanError(attr.dataType))
case ValidationAnyOf(values) =>
val bigOrExpr = Ast.expr.BoolOp(
Ast.boolop.Or,
values.map(expected =>
Ast.expr.Compare(
Ast.expr.InternalName(attrId),
itemValue,
Ast.cmpop.Eq,
expected
)
Expand All @@ -38,30 +39,30 @@ trait ValidateOps extends ExceptionNames {

attrValidateExpr(
attrId,
attr.dataTypeComposite,
attr.dataType,
checkExpr = bigOrExpr,
err = ValidationNotAnyOfError(attr.dataTypeComposite),
err = ValidationNotAnyOfError(attr.dataType),
errArgs = List(
Ast.expr.InternalName(attrId),
itemValue,
Ast.expr.InternalName(IoIdentifier),
Ast.expr.Str(attr.path.mkString("/", "/", ""))
)
)
case ValidationExpr(expr) =>
blockScopeHeader
typeProvider._currentIteratorType = Some(attr.dataTypeComposite)
typeProvider._currentIteratorType = Some(attr.dataType)
handleAssignmentTempVar(
attr.dataTypeComposite,
attr.dataType,
translator.translate(Ast.expr.Name(Ast.identifier(Identifier.ITERATOR))),
translator.translate(Ast.expr.InternalName(attrId))
translator.translate(itemValue)
)
attrValidateExpr(
attrId,
attr.dataTypeComposite,
attr.dataType,
expr,
ValidationExprError(attr.dataTypeComposite),
ValidationExprError(attr.dataType),
List(
Ast.expr.InternalName(attrId),
itemValue,
Ast.expr.InternalName(IoIdentifier),
Ast.expr.Str(attr.path.mkString("/", "/", ""))
)
Expand All @@ -71,18 +72,19 @@ trait ValidateOps extends ExceptionNames {
}

def attrValidateExprCompare(attrId: Identifier, attr: AttrLikeSpec, op: Ast.cmpop, expected: Ast.expr, err: KSError): Unit = {
val itemValue = Identifier.itemExpr(attrId, attr.cond.repeat)
attrValidateExpr(
attrId,
attr.dataTypeComposite,
attr.dataType,
checkExpr = Ast.expr.Compare(
Ast.expr.InternalName(attrId),
itemValue,
op,
expected
),
err = err,
errArgs = List(
expected,
Ast.expr.InternalName(attrId),
itemValue,
Ast.expr.InternalName(IoIdentifier),
Ast.expr.Str(attr.path.mkString("/", "/", ""))
)
Expand Down

0 comments on commit 7154301

Please sign in to comment.