From 343cb32d0983f576d344a2d04a35c3fd6eecf2c5 Mon Sep 17 00:00:00 2001
From: Soham Chowdhury <chow.soham@gmail.com>
Date: Thu, 11 May 2017 15:40:18 -0400
Subject: [PATCH] Fix incorrect ambiguity error on identically-named data
 constructors

Given multiple in-scope constructors with the same name, say `A`, and a
function of type `A -> Int`, say, the compiler reports both a "type `A`
is not in scope" and (incorrectly) an ambiguity error. The latter
shouldn't be there if `DataKinds` isn't enabled.

This issue was recommended to me by @mpickering as a suitable first
task, and the fix was also outlined in the original Trac ticket. It
involved a simple reordering of the steps taken in `lookup_demoted` in
`RnEnv.hs`. The fix is to make the `DataKinds` check happen earlier,
ensuring that the ambiguity check doesn't happen at all if we know the
constructors couldn't have been promoted.

Signed-off-by: Soham Chowdhury <chow.soham@gmail.com>

Reviewers: mpickering, austin, bgamari

Reviewed By: mpickering, bgamari

Subscribers: rwbarton, thomie

GHC Trac Issues: #13568

Differential Revision: https://phabricator.haskell.org/D3547

(cherry picked from commit 1381c142cd8d030f9997cdc206dcad006c028bbb)
---
 compiler/rename/RnEnv.hs                      | 21 ++++++++++---------
 testsuite/tests/module/mod122.stderr          |  4 +++-
 testsuite/tests/module/mod123.stderr          |  4 +++-
 testsuite/tests/module/mod124.stderr          |  1 +
 testsuite/tests/module/mod127.stderr          |  1 +
 testsuite/tests/module/mod29.stderr           |  1 +
 testsuite/tests/module/mod50.stderr           |  4 +++-
 .../parser/should_fail/readFail001.stderr     |  1 +
 .../rename/prog003/rename.prog003.stderr      |  4 +++-
 testsuite/tests/rename/should_fail/T13568.hs  |  8 +++++++
 .../tests/rename/should_fail/T13568.stderr    |  4 ++++
 testsuite/tests/rename/should_fail/T13568a.hs |  3 +++
 .../tests/rename/should_fail/T1595a.stderr    |  4 +++-
 .../tests/rename/should_fail/T5745.stderr     |  4 +++-
 testsuite/tests/rename/should_fail/all.T      |  1 +
 .../tests/typecheck/should_fail/T1595.stderr  |  6 ++++--
 .../typecheck/should_fail/tcfail048.stderr    |  4 +++-
 .../typecheck/should_fail/tcfail053.stderr    |  4 +++-
 18 files changed, 59 insertions(+), 20 deletions(-)
 create mode 100644 testsuite/tests/rename/should_fail/T13568.hs
 create mode 100644 testsuite/tests/rename/should_fail/T13568.stderr
 create mode 100644 testsuite/tests/rename/should_fail/T13568a.hs

diff --git a/compiler/rename/RnEnv.hs b/compiler/rename/RnEnv.hs
index dbc5a902ab7c..8791625e06df 100644
--- a/compiler/rename/RnEnv.hs
+++ b/compiler/rename/RnEnv.hs
@@ -723,16 +723,17 @@ lookup_demoted rdr_name dflags
   | Just demoted_rdr <- demoteRdrName rdr_name
     -- Maybe it's the name of a *data* constructor
   = do { data_kinds <- xoptM LangExt.DataKinds
-       ; mb_demoted_name <- lookupOccRn_maybe demoted_rdr
-       ; case mb_demoted_name of
-           Nothing -> unboundNameX WL_Any rdr_name star_info
-           Just demoted_name
-             | data_kinds ->
-             do { whenWOptM Opt_WarnUntickedPromotedConstructors $
-                  addWarn (Reason Opt_WarnUntickedPromotedConstructors)
-                          (untickedPromConstrWarn demoted_name)
-                ; return demoted_name }
-             | otherwise  -> unboundNameX WL_Any rdr_name suggest_dk }
+       ; if data_kinds
+            then do { mb_demoted_name <- lookupOccRn_maybe demoted_rdr
+                    ; case mb_demoted_name of
+                        Nothing -> unboundNameX WL_Any rdr_name star_info
+                        Just demoted_name ->
+                          do { whenWOptM Opt_WarnUntickedPromotedConstructors $
+                               addWarn
+                                 (Reason Opt_WarnUntickedPromotedConstructors)
+                                 (untickedPromConstrWarn demoted_name)
+                             ; return demoted_name } }
+            else unboundNameX WL_Any rdr_name suggest_dk }
 
   | otherwise
   = reportUnboundName rdr_name
diff --git a/testsuite/tests/module/mod122.stderr b/testsuite/tests/module/mod122.stderr
index 90719ecf0661..66aaaf23041e 100644
--- a/testsuite/tests/module/mod122.stderr
+++ b/testsuite/tests/module/mod122.stderr
@@ -1,2 +1,4 @@
 
-mod122.hs:5:6: Not in scope: type constructor or class ‘C’
+mod122.hs:5:6: error:
+    Not in scope: type constructor or class ‘C’
+    A data constructor of that name is in scope; did you mean DataKinds?
diff --git a/testsuite/tests/module/mod123.stderr b/testsuite/tests/module/mod123.stderr
index 9d9de6bbb26e..38390d05d1f5 100644
--- a/testsuite/tests/module/mod123.stderr
+++ b/testsuite/tests/module/mod123.stderr
@@ -1,2 +1,4 @@
 
-mod123.hs:5:6: Not in scope: type constructor or class ‘T’
+mod123.hs:5:6: error:
+    Not in scope: type constructor or class ‘T’
+    A data constructor of that name is in scope; did you mean DataKinds?
diff --git a/testsuite/tests/module/mod124.stderr b/testsuite/tests/module/mod124.stderr
index a052a506ad2b..cbf9f4558e55 100644
--- a/testsuite/tests/module/mod124.stderr
+++ b/testsuite/tests/module/mod124.stderr
@@ -1,5 +1,6 @@
 
 mod124.hs:6:6: error:
     Not in scope: type constructor or class ‘T’
+    A data constructor of that name is in scope; did you mean DataKinds?
     Perhaps you want to remove ‘T’ from the explicit hiding list
     in the import of ‘Mod124_A’ (mod124.hs:4:1-26).
diff --git a/testsuite/tests/module/mod127.stderr b/testsuite/tests/module/mod127.stderr
index 861d492d1a35..462ebbccaf71 100644
--- a/testsuite/tests/module/mod127.stderr
+++ b/testsuite/tests/module/mod127.stderr
@@ -1,5 +1,6 @@
 
 mod127.hs:6:6: error:
     Not in scope: type constructor or class ‘T’
+    A data constructor of that name is in scope; did you mean DataKinds?
     Perhaps you want to remove ‘T’ from the explicit hiding list
     in the import of ‘Mod127_A’ (mod127.hs:4:1-26).
diff --git a/testsuite/tests/module/mod29.stderr b/testsuite/tests/module/mod29.stderr
index e70c5df83d49..08a019e13d06 100644
--- a/testsuite/tests/module/mod29.stderr
+++ b/testsuite/tests/module/mod29.stderr
@@ -1,5 +1,6 @@
 
 mod29.hs:6:12: error:
     Not in scope: type constructor or class ‘Char’
+    A data constructor of that name is in scope; did you mean DataKinds?
     Perhaps you want to add ‘Char’ to the import list in the import of
     ‘Prelude’ (mod29.hs:5:1-19).
diff --git a/testsuite/tests/module/mod50.stderr b/testsuite/tests/module/mod50.stderr
index 593148e3ab06..94878a8faa9a 100644
--- a/testsuite/tests/module/mod50.stderr
+++ b/testsuite/tests/module/mod50.stderr
@@ -1,2 +1,4 @@
 
-mod50.hs:3:22: Not in scope: type constructor or class ‘Foo’
+mod50.hs:3:22: error:
+    Not in scope: type constructor or class ‘Foo’
+    A data constructor of that name is in scope; did you mean DataKinds?
diff --git a/testsuite/tests/parser/should_fail/readFail001.stderr b/testsuite/tests/parser/should_fail/readFail001.stderr
index 6425d16c49d9..3284c1b51c45 100644
--- a/testsuite/tests/parser/should_fail/readFail001.stderr
+++ b/testsuite/tests/parser/should_fail/readFail001.stderr
@@ -16,3 +16,4 @@ readFail001.hs:107:42: error: Not in scope: data constructor ‘Bar’
 
 readFail001.hs:112:23: error:
     Not in scope: type constructor or class ‘Foo’
+    A data constructor of that name is in scope; did you mean DataKinds?
diff --git a/testsuite/tests/rename/prog003/rename.prog003.stderr b/testsuite/tests/rename/prog003/rename.prog003.stderr
index 7a0b5244c0c3..6babd0383bd9 100644
--- a/testsuite/tests/rename/prog003/rename.prog003.stderr
+++ b/testsuite/tests/rename/prog003/rename.prog003.stderr
@@ -1,2 +1,4 @@
 
-B.hs:4:6: Not in scope: type constructor or class ‘Class’
+B.hs:4:6: error:
+    Not in scope: type constructor or class ‘Class’
+    A data constructor of that name is in scope; did you mean DataKinds?
diff --git a/testsuite/tests/rename/should_fail/T13568.hs b/testsuite/tests/rename/should_fail/T13568.hs
new file mode 100644
index 000000000000..f815c839ce1c
--- /dev/null
+++ b/testsuite/tests/rename/should_fail/T13568.hs
@@ -0,0 +1,8 @@
+module T13568 where
+
+import T13568a
+
+data S = A
+
+foo :: A -> ()
+foo = undefined
diff --git a/testsuite/tests/rename/should_fail/T13568.stderr b/testsuite/tests/rename/should_fail/T13568.stderr
new file mode 100644
index 000000000000..63ee18409ac2
--- /dev/null
+++ b/testsuite/tests/rename/should_fail/T13568.stderr
@@ -0,0 +1,4 @@
+
+T13568.hs:7:8: error:
+    Not in scope: type constructor or class ‘A’
+    A data constructor of that name is in scope; did you mean DataKinds?
diff --git a/testsuite/tests/rename/should_fail/T13568a.hs b/testsuite/tests/rename/should_fail/T13568a.hs
new file mode 100644
index 000000000000..c25a48ad3799
--- /dev/null
+++ b/testsuite/tests/rename/should_fail/T13568a.hs
@@ -0,0 +1,3 @@
+module T13568a where
+
+data T = A
diff --git a/testsuite/tests/rename/should_fail/T1595a.stderr b/testsuite/tests/rename/should_fail/T1595a.stderr
index 9705293a0a7a..9b19421f3a45 100644
--- a/testsuite/tests/rename/should_fail/T1595a.stderr
+++ b/testsuite/tests/rename/should_fail/T1595a.stderr
@@ -1,2 +1,4 @@
 
-T1595a.hs:3:20: Not in scope: type constructor or class ‘Tpyo’
+T1595a.hs:3:20: error:
+    Not in scope: type constructor or class ‘Tpyo’
+    A data constructor of that name is in scope; did you mean DataKinds?
diff --git a/testsuite/tests/rename/should_fail/T5745.stderr b/testsuite/tests/rename/should_fail/T5745.stderr
index 577ae069b3be..94e3bd521718 100644
--- a/testsuite/tests/rename/should_fail/T5745.stderr
+++ b/testsuite/tests/rename/should_fail/T5745.stderr
@@ -1,2 +1,4 @@
 
-T5745.hs:5:6: Not in scope: type constructor or class ‘T’
+T5745.hs:5:6: error:
+    Not in scope: type constructor or class ‘T’
+    A data constructor of that name is in scope; did you mean DataKinds?
diff --git a/testsuite/tests/rename/should_fail/all.T b/testsuite/tests/rename/should_fail/all.T
index 4782685f5e26..9acd5b83ddef 100644
--- a/testsuite/tests/rename/should_fail/all.T
+++ b/testsuite/tests/rename/should_fail/all.T
@@ -125,3 +125,4 @@ test('T12681', normal, multimod_compile_fail, ['T12681','-v0'])
 test('T12686', normal, compile_fail, [''])
 test('T11592', normal, compile_fail, [''])
 test('T12879', normal, compile_fail, [''])
+test('T13568', normal, multimod_compile_fail, ['T13568','-v0'])
diff --git a/testsuite/tests/typecheck/should_fail/T1595.stderr b/testsuite/tests/typecheck/should_fail/T1595.stderr
index 1f999c636b0a..bed30c42cf99 100644
--- a/testsuite/tests/typecheck/should_fail/T1595.stderr
+++ b/testsuite/tests/typecheck/should_fail/T1595.stderr
@@ -1,6 +1,8 @@
 
-T1595.hs:8:15:
+T1595.hs:8:15: error:
     Not in scope: type constructor or class ‘DoesNotExist’
+    A data constructor of that name is in scope; did you mean DataKinds?
 
-T1595.hs:13:22:
+T1595.hs:13:22: error:
     Not in scope: type constructor or class ‘DoesNotExist’
+    A data constructor of that name is in scope; did you mean DataKinds?
diff --git a/testsuite/tests/typecheck/should_fail/tcfail048.stderr b/testsuite/tests/typecheck/should_fail/tcfail048.stderr
index eaa2e52d36ef..4c1c300ef8f5 100644
--- a/testsuite/tests/typecheck/should_fail/tcfail048.stderr
+++ b/testsuite/tests/typecheck/should_fail/tcfail048.stderr
@@ -1,2 +1,4 @@
 
-tcfail048.hs:3:8: Not in scope: type constructor or class ‘B’
+tcfail048.hs:3:8: error:
+    Not in scope: type constructor or class ‘B’
+    A data constructor of that name is in scope; did you mean DataKinds?
diff --git a/testsuite/tests/typecheck/should_fail/tcfail053.stderr b/testsuite/tests/typecheck/should_fail/tcfail053.stderr
index a9b13bd6dae2..edd1537b1424 100644
--- a/testsuite/tests/typecheck/should_fail/tcfail053.stderr
+++ b/testsuite/tests/typecheck/should_fail/tcfail053.stderr
@@ -1,2 +1,4 @@
 
-tcfail053.hs:3:12: Not in scope: type constructor or class ‘A’
+tcfail053.hs:3:12: error:
+    Not in scope: type constructor or class ‘A’
+    A data constructor of that name is in scope; did you mean DataKinds?
-- 
GitLab