Add a warning for data X = Y {...}. (digital-asset#4892)

* Add a warning for data X = Y {..}. Part of digital-asset#4718. changelog_begin - [DAML Compiler] The DAML compiler now emits a warning when you declare a single-constructor record type where the constructor name does not match the type name, such as ``data X = Y {...}``. This kind of type declaration can cause problems with cross-SDK upgrades because the record constructor name cannot be recorded in the DAML-LF representation. In the future, this warning will be upgraded to an error. changelog_end * s/Ctor/Constructor/g
conr2d · Mar 9, 2020 · 943832b · 943832b
1 parent 0e046d9
commit 943832b
Show file tree

Hide file tree

Showing 3 changed files with 41 additions and 9 deletions.
diff --git a/compiler/damlc/daml-preprocessor/src/DA/Daml/Preprocessor.hs b/compiler/damlc/daml-preprocessor/src/DA/Daml/Preprocessor.hs
@@ -16,6 +16,8 @@ import Development.IDE.Types.Options
 import qualified "ghc-lib" GHC
 import qualified "ghc-lib-parser" SrcLoc as GHC
 import qualified "ghc-lib-parser" Module as GHC
+import qualified "ghc-lib-parser" RdrName as GHC
+import qualified "ghc-lib-parser" OccName as GHC
 import qualified "ghc-lib-parser" FastString as GHC
 import Outputable
 
@@ -60,7 +62,7 @@ damlPreprocessor :: Maybe GHC.UnitId -> GHC.ParsedSource -> IdePreprocessedSourc
 damlPreprocessor mbUnitId x
     | maybe False (isInternal ||^ (`elem` mayImportInternal)) name = noPreprocessor x
     | otherwise = IdePreprocessedSource
-        { preprocWarnings = checkModuleName x
+        { preprocWarnings = checkModuleName x ++ checkRecordConstructor x
         , preprocErrors = checkImports x ++ checkDataTypes x ++ checkModuleDefinition x
         , preprocSource = recordDotPreprocessor $ importDamlPreprocessor $ genericsPreprocessor mbUnitId $ enumTypePreprocessor "GHC.Types" x
         }
@@ -117,6 +119,30 @@ checkImports x =
     [ (ss, "Import of internal module " ++ GHC.moduleNameString m ++ " is not allowed.")
     | GHC.L ss GHC.ImportDecl{ideclName=GHC.L _ m} <- GHC.hsmodImports $ GHC.unLoc x, isInternal m]
 
+-- | Emit a warning if a record constructor name does not match the record type name.
+-- See issue #4718. This ought to be moved into 'checkDataTypes' before too long.
+checkRecordConstructor :: GHC.ParsedSource -> [(GHC.SrcSpan, String)]
+checkRecordConstructor (GHC.L _ m) = mapMaybe getRecordError (GHC.hsmodDecls m)
+  where
+    getRecordError :: GHC.LHsDecl GHC.GhcPs -> Maybe (GHC.SrcSpan, String)
+    getRecordError (GHC.L ss decl)
+        | GHC.TyClD _ GHC.DataDecl{tcdLName=ltyName, tcdDataDefn=dataDefn} <- decl
+        , GHC.HsDataDefn{dd_cons=[con]} <- dataDefn
+        , GHC.RecCon{} <- GHC.con_args (GHC.unLoc con)
+        , GHC.L _ tyName <- ltyName
+        , [GHC.L _ conName] <- GHC.getConNames (GHC.unLoc con)
+        , tyNameStr <- GHC.occNameString (GHC.rdrNameOcc tyName)
+        , conNameStr <- GHC.occNameString (GHC.rdrNameOcc conName)
+        , tyNameStr /= conNameStr
+        = Just (ss, message tyNameStr conNameStr)
+
+        | otherwise
+        = Nothing
+
+    message tyNameStr conNameStr = unwords
+        [ "Record type", tyNameStr, "has constructor", conNameStr
+        , "with different name. This may cause problems with cross-SDK upgrades."
+        , "Possible solution: Change the constructor name to", tyNameStr ]
 
 checkDataTypes :: GHC.ParsedSource -> [(GHC.SrcSpan, String)]
 checkDataTypes m = checkAmbiguousDataTypes m ++ checkUnlabelledConArgs m ++ checkThetas m

diff --git a/compiler/damlc/tests/daml-test-files/DataTypes.daml b/compiler/damlc/tests/daml-test-files/DataTypes.daml
@@ -7,13 +7,11 @@
 
 module DataTypes where
 
-data Rec = MkRec with x: Int
+data Rec = Rec with x: Int
 
-newtype RecNT = MkRecNT with x: Int
+newtype RecNT = RecNT with x: Int
 
--- NOTE(MH): This is translted to a variant with one constructor and _not_
--- to a record.
-data Unit = MkUnit{}
+data Unit = Unit{}
 
 data Tag = MkTag Int
 
@@ -40,11 +38,11 @@ eval = \case
 
 
 main = scenario do
-  assert $ (MkRec with x = 5).x == 5
+  assert $ (Rec with x = 5).x == 5
 
-  assert $ (MkRecNT with x = 7).x == 7
+  assert $ (RecNT with x = 7).x == 7
 
-  assert $ case MkUnit of {MkUnit -> True}
+  assert $ case Unit of {Unit -> True}
 
   assert $ untag (MkTag 3) == 3
 

diff --git a/compiler/damlc/tests/daml-test-files/RecordConstructorCheck.daml b/compiler/damlc/tests/daml-test-files/RecordConstructorCheck.daml
@@ -0,0 +1,8 @@
+-- Copyright (c) 2020, Digital Asset (Switzerland) GmbH and/or its affiliates.
+-- All rights reserved.
+
+-- @WARN Record type X has constructor Y with different name. This may cause problems with cross-SDK upgrades. Possible solution: Change the constructor name to X
+
+module RecordConstructorCheck where
+
+data X = Y {}