From 90b8bad66f5bf9b4269a2bbdaf1ae6b1558a02d0 Mon Sep 17 00:00:00 2001 From: Eduardo Salinas Date: Tue, 14 Jun 2016 13:58:47 -0700 Subject: [PATCH] Add field default checking to GBC - Validate default value type mistmatches (fixes #72, fixes #128) - Validate default value out-of-range values (fixes #73) - Fail when struct field has default value of 'nothing' (fixes #164) - Fail when enum field doesn't have default value (fixes #177) - Validate default value of type aliases --- CHANGELOG.md | 10 ++++ compiler/src/Language/Bond/Parser.hs | 51 +++++++++++++++++-- compiler/src/Language/Bond/Syntax/Util.hs | 7 +++ compiler/tests/Main.hs | 6 +++ compiler/tests/Tests/Syntax.hs | 14 ++++- .../tests/schema/error/aliases_default.bond | 12 +++++ .../tests/schema/error/enum_no_default.bond | 17 +++++++ .../tests/schema/error/int_out_of_range.bond | 10 ++++ .../tests/schema/error/struct_nothing.bond | 15 ++++++ doc/src/compiler.md | 3 +- 10 files changed, 138 insertions(+), 7 deletions(-) create mode 100644 compiler/tests/schema/error/aliases_default.bond create mode 100644 compiler/tests/schema/error/enum_no_default.bond create mode 100644 compiler/tests/schema/error/int_out_of_range.bond create mode 100644 compiler/tests/schema/error/struct_nothing.bond diff --git a/CHANGELOG.md b/CHANGELOG.md index 3e95f6892a..cd9ed68120 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,16 @@ different versioning scheme, following the Haskell community's * Runtime SchemaDef now includes information about whether BT_LIST fields are nullable or blobs. [Issue #161](https://github.com/Microsoft/bond/issues/161) +* Validate default value type mistmatches. + [Issue #72](https://github.com/Microsoft/bond/issues/72) + [Issue #128](https://github.com/Microsoft/bond/issues/128) +* Validate default value out-of-range values. + [Issue #73](https://github.com/Microsoft/bond/issues/73) +* Fail when struct field has default value of `nothing`. + [Issue #164](https://github.com/Microsoft/bond/issues/164) +* Fail when enum field doesn't have default value. + [Issue #177](https://github.com/Microsoft/bond/issues/177) +* Validate default value of type aliases ### C++ ### diff --git a/compiler/src/Language/Bond/Parser.hs b/compiler/src/Language/Bond/Parser.hs index 81c22171a3..ff59f33fb4 100755 --- a/compiler/src/Language/Bond/Parser.hs +++ b/compiler/src/Language/Bond/Parser.hs @@ -1,7 +1,7 @@ -- Copyright (c) Microsoft. All rights reserved. -- Licensed under the MIT license. See LICENSE file in the project root for full license information. -{-# LANGUAGE RecordWildCards #-} +{-# LANGUAGE RecordWildCards, ScopedTypeVariables #-} {-| Copyright : (c) Microsoft @@ -24,6 +24,7 @@ module Language.Bond.Parser import Data.Ord import Data.List import Data.Function +import Data.Int import Data.Word import Control.Applicative import Control.Monad.Reader @@ -247,13 +248,17 @@ manySortedBy = manyAccum . insertBy -- field definition parser field :: Parser Field -field = makeField <$> attributes <*> ordinal <*> modifier <*> ftype <*> identifier <*> optional default_ +field = do + mf <- makeField <$> attributes <*> ordinal <*> modifier <*> ftype <*> identifier <*> optional default_ + case mf of + Left e -> fail e + Right f -> return f where ordinal = word16 <* colon "field ordinal" where word16 = do i <- integer - if i <= toInteger (maxBound :: Word16) && i >= toInteger (minBound :: Word16) + if isInBounds i (0::Word16) then return (fromInteger i) else fail "Field ordinal must be within the range 0-65535" modifier = option Optional @@ -268,8 +273,44 @@ field = makeField <$> attributes <*> ordinal <*> modifier <*> ftype <*> identifi <|> DefaultEnum <$> identifier <|> DefaultFloat <$> try float <|> DefaultInteger <$> fromIntegral <$> integer) - makeField a o m t n d@(Just DefaultNothing) = Field a o m (BT_Maybe t) n d - makeField a o m t n d = Field a o m t n d + makeField a o m t n d@(Just DefaultNothing) + | isStruct t = Left "Struct field can't have default value of 'nothing'" + | otherwise = Right $ Field a o m (BT_Maybe t) n d + makeField a o m t n d + | d == Nothing && isEnum t = Left "Enum field must have a default value" + | otherwise = if validDefaultType t d + then Right $ Field a o m t n d + else Left "Invalid default value for field" + +-- default type validator (type checking, out-of-range, enforce default type) +validDefaultType :: Type -> Maybe Default -> Bool +validDefaultType (BT_UserDefined a@Alias {} args) d = validDefaultType (resolveAlias a args) d +validDefaultType _ Nothing = True +validDefaultType bondType (Just defaultValue) = validDefaultType' bondType defaultValue + where validDefaultType' :: Type -> Default -> Bool + validDefaultType' BT_Int8 (DefaultInteger i) = isInBounds i (0::Int8) + validDefaultType' BT_Int16 (DefaultInteger i) = isInBounds i (0::Int16) + validDefaultType' BT_Int32 (DefaultInteger i) = isInBounds i (0::Int32) + validDefaultType' BT_Int64 (DefaultInteger i) = isInBounds i (0::Int64) + validDefaultType' BT_UInt8 (DefaultInteger i) = isInBounds i (0::Word8) + validDefaultType' BT_UInt16 (DefaultInteger i) = isInBounds i (0::Word16) + validDefaultType' BT_UInt32 (DefaultInteger i) = isInBounds i (0::Word32) + validDefaultType' BT_UInt64 (DefaultInteger i) = isInBounds i (0::Word64) + validDefaultType' BT_Float (DefaultFloat _) = True + validDefaultType' BT_Float (DefaultInteger _) = True + validDefaultType' BT_Double (DefaultFloat _) = True + validDefaultType' BT_Double (DefaultInteger _) = True + validDefaultType' BT_Bool (DefaultBool _) = True + validDefaultType' BT_String (DefaultString _) = True + validDefaultType' BT_WString (DefaultString _) = True + validDefaultType' (BT_UserDefined Enum {} _) (DefaultEnum _) = True + validDefaultType' (BT_TypeParam {}) _ = True + validDefaultType' _ _ = False + +-- checks whether an Integer is within the bounds of some other Integral and Bounded type. +-- The value of the second paramater is never used: only its type is used. +isInBounds :: forall a. (Integral a, Bounded a) => Integer -> a -> Bool +isInBounds value _ = value >= (toInteger (minBound :: a)) && value <= (toInteger (maxBound :: a)) -- enum definition parser enum :: Parser Declaration diff --git a/compiler/src/Language/Bond/Syntax/Util.hs b/compiler/src/Language/Bond/Syntax/Util.hs index dec20fac55..2a7e3460d9 100644 --- a/compiler/src/Language/Bond/Syntax/Util.hs +++ b/compiler/src/Language/Bond/Syntax/Util.hs @@ -26,6 +26,7 @@ module Language.Bond.Syntax.Util , isAssociative , isNullable , isStruct + , isEnum , isMetaName -- * Type mapping , fmapType @@ -127,6 +128,12 @@ isStruct (BT_UserDefined Forward {} _) = True isStruct (BT_UserDefined a@Alias {} args) = isStruct $ resolveAlias a args isStruct _ = False +-- | Returns 'True' if the type represents an enum +isEnum :: Type -> Bool +isEnum (BT_UserDefined Enum {} _) = True +isEnum (BT_UserDefined a@Alias {} args) = isEnum $ resolveAlias a args +isEnum _ = False + -- | Returns 'True' if the type represents a nullable type. isNullable :: Type -> Bool isNullable (BT_Nullable _) = True diff --git a/compiler/tests/Main.hs b/compiler/tests/Main.hs index 8d1dc030f9..946c50a7fa 100644 --- a/compiler/tests/Main.hs +++ b/compiler/tests/Main.hs @@ -45,6 +45,12 @@ tests = testGroup "Compiler tests" , testGroup "Types" [ testCase "type alias resolution" aliasResolution ] + , testGroup "Codegen Failures (Expect to see errors below check for OK or FAIL)" + [ testCase "Struct default value nothing" $ failBadSyntax "Should fail when default value of a struct field is 'nothing'" "struct_nothing" + , testCase "Enum no default value" $ failBadSyntax "Should fail when an enum field has no default value" "enum_no_default" + , testCase "Alias default value" $ failBadSyntax "Should fail when underlying default value is of the wrong type" "aliases_default" + , testCase "Out of range" $ failBadSyntax "Should fail, out of range for int16" "int_out_of_range" + ] , testGroup "Codegen" [ testGroup "C++" [ verifyCppCodegen "attributes" diff --git a/compiler/tests/Tests/Syntax.hs b/compiler/tests/Tests/Syntax.hs index 541a1c1431..7f7d2e8eec 100644 --- a/compiler/tests/Tests/Syntax.hs +++ b/compiler/tests/Tests/Syntax.hs @@ -1,16 +1,18 @@ -- Copyright (c) Microsoft. All rights reserved. -- Licensed under the MIT license. See LICENSE file in the project root for full license information. -{-# LANGUAGE OverloadedStrings, RecordWildCards, TemplateHaskell #-} +{-# LANGUAGE OverloadedStrings, RecordWildCards, TemplateHaskell, ScopedTypeVariables #-} {-# OPTIONS_GHC -fno-warn-orphans #-} module Tests.Syntax ( roundtripAST , compareAST + , failBadSyntax , aliasResolution , verifySchemaDef ) where +import Control.Exception import Data.Maybe import Data.List import Data.Aeson (encode, decode) @@ -46,6 +48,16 @@ derive makeArbitrary ''Method roundtripAST :: Bond -> Bool roundtripAST x = (decode . encode) x == Just x +assertException :: String -> IO a -> Assertion +assertException errMsg action = do + e <- try action + case e of + Left (_ :: SomeException) -> return () + Right _ -> assertFailure errMsg + +failBadSyntax :: String -> FilePath -> Assertion +failBadSyntax errMsg file = assertException errMsg (parseBondFile [] $ "tests" "schema" "error" file <.> "bond") + compareAST :: FilePath -> Assertion compareAST file = do bond <- parseBondFile [] $ "tests" "schema" file <.> "bond" diff --git a/compiler/tests/schema/error/aliases_default.bond b/compiler/tests/schema/error/aliases_default.bond new file mode 100644 index 0000000000..aa4d265c15 --- /dev/null +++ b/compiler/tests/schema/error/aliases_default.bond @@ -0,0 +1,12 @@ + +//this bond file is invalid; do not use as example + + +namespace tests + +using String = string; + +struct Test +{ + 0: String s = 6; +} \ No newline at end of file diff --git a/compiler/tests/schema/error/enum_no_default.bond b/compiler/tests/schema/error/enum_no_default.bond new file mode 100644 index 0000000000..0d539e4630 --- /dev/null +++ b/compiler/tests/schema/error/enum_no_default.bond @@ -0,0 +1,17 @@ + +//this bond file is invalid; do not use as example + + +namespace tests + +enum Priority +{ + Low; + Normal; + High; +} + +struct Test +{ + 0: Priority p; +} \ No newline at end of file diff --git a/compiler/tests/schema/error/int_out_of_range.bond b/compiler/tests/schema/error/int_out_of_range.bond new file mode 100644 index 0000000000..538e5cff8a --- /dev/null +++ b/compiler/tests/schema/error/int_out_of_range.bond @@ -0,0 +1,10 @@ + +//this bond file is invalid; do not use as example + + +namespace tests + +struct Test +{ + 0: int16 i = 32768; +} \ No newline at end of file diff --git a/compiler/tests/schema/error/struct_nothing.bond b/compiler/tests/schema/error/struct_nothing.bond new file mode 100644 index 0000000000..808a602159 --- /dev/null +++ b/compiler/tests/schema/error/struct_nothing.bond @@ -0,0 +1,15 @@ + +//this bond file is invalid; do not use as example + + +namespace tests + +struct BasicTypes +{ + 0: bool _bool; +} + +struct Test +{ + 0: BasicTypes bt = nothing; +} \ No newline at end of file diff --git a/doc/src/compiler.md b/doc/src/compiler.md index c7e173ac0b..714ff711a6 100644 --- a/doc/src/compiler.md +++ b/doc/src/compiler.md @@ -115,7 +115,8 @@ An optional default value can be specified for fields of basic types. For integers the default can be specified as either a decimal number or a hexadecimal number prefixed with `0x`. The only explicit default value allowed for containers is [`nothing`](#default-value-of-nothing). Enum fields must have -an explicit default value which must be one of the enum named constants. +an explicit default value which must be one of the enum named constants or +[`nothing`](#default-value-of-nothing). Names of structs and enums defined in another namespace must be qualified with the namespace name: