From 943832bc16278a03217fb18159b6d4f08de31ed0 Mon Sep 17 00:00:00 2001
From: associahedron <231829+associahedron@users.noreply.github.com>
Date: Mon, 9 Mar 2020 11:28:16 +0000
Subject: [PATCH] Add a warning for data X = Y {...}. (#4892)

* Add a warning for data X = Y {..}.

Part of #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
---
 .../src/DA/Daml/Preprocessor.hs               | 28 ++++++++++++++++++-
 .../tests/daml-test-files/DataTypes.daml      | 14 ++++------
 .../RecordConstructorCheck.daml               |  8 ++++++
 3 files changed, 41 insertions(+), 9 deletions(-)
 create mode 100644 compiler/damlc/tests/daml-test-files/RecordConstructorCheck.daml

diff --git a/compiler/damlc/daml-preprocessor/src/DA/Daml/Preprocessor.hs b/compiler/damlc/daml-preprocessor/src/DA/Daml/Preprocessor.hs
index 3eb8c12149..ca5d6c0497 100644
--- 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
index 6388c21ca0..df4c61d215 100644
--- 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
new file mode 100644
index 0000000000..38e4c45020
--- /dev/null
+++ 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 {}