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

@@ -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++ ###
 

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

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

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"

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"

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;
+}

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;
+}

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;
+}

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;
+}

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: