From ff3e286ca02525c1393304617b4c6f6519c63a6f Mon Sep 17 00:00:00 2001
From: Sam Zhou <samzhou19815@meta.com>
Date: Fri, 7 Mar 2025 09:45:42 -0800
Subject: [PATCH] [flow] Scoped libdef support 5/n: Error on libdef module
 override

Summary:
Same as last diffs, but for modules.

Changelog: [errors] Overriding already defined modules in library definition will now error.

Reviewed By: panagosg7

Differential Revision: D70575060

fbshipit-source-id: 72edd21a089e2b30319cc3c28103b6239f9288dc
---
 .../type_sig/__tests__/type_sig_tests.ml      |  7 ++++-
 src/parser_utils/type_sig/type_sig_parse.ml   | 12 ++++++--
 tests/duplicate_libs/duplicate_libs.exp       | 28 ++++++++++++++++++-
 tests/duplicate_libs/lib/shared/lib.js        |  4 +++
 tests/duplicate_libs/test.js                  |  1 +
 5 files changed, 47 insertions(+), 5 deletions(-)

diff --git a/src/parser_utils/type_sig/__tests__/type_sig_tests.ml b/src/parser_utils/type_sig/__tests__/type_sig_tests.ml
index d7c1121f3f8..670c992c871 100644
--- a/src/parser_utils/type_sig/__tests__/type_sig_tests.ml
+++ b/src/parser_utils/type_sig/__tests__/type_sig_tests.ml
@@ -4657,7 +4657,12 @@ let%expect_test "builtin_cjs_ignore_later" =
                 info =
                 CJSModuleInfo {type_export_keys = [||];
                   type_stars = []; strict = true;
-                  platform_availability_set = None}} |}]
+                  platform_availability_set = None}}
+    Errors:
+    (BindingValidationError
+       Signature_error.ModuleOverride {
+         name = "foo"; override_binding_loc = [2:15-18];
+         existing_binding_loc = [5:15-18]}) |}]
 
 let%expect_test "builtin_cjs_module_auto_export_type" =
   (* All types in cjs modules are auto exported. *)
diff --git a/src/parser_utils/type_sig/type_sig_parse.ml b/src/parser_utils/type_sig/type_sig_parse.ml
index bcd8bddb50d..24bac9db71b 100644
--- a/src/parser_utils/type_sig/type_sig_parse.ml
+++ b/src/parser_utils/type_sig/type_sig_parse.ml
@@ -544,7 +544,7 @@ module Scope = struct
   let push_declare_namespace parent =
     DeclareNamespace { parent; values = SMap.empty; types = SMap.empty }
 
-  let push_declare_module loc name parent =
+  let push_declare_module loc name parent tbls =
     let exports = Exports.create ~strict:true ~platform_availability_set:None in
     begin
       match parent with
@@ -554,7 +554,13 @@ module Scope = struct
             name
             (function
               | None -> Some (loc, exports)
-              | Some existing -> Some existing)
+              | Some existing ->
+                let override_binding_loc = fst existing in
+                tbls.additional_errors <-
+                  Signature_error.ModuleOverride
+                    { name; override_binding_loc; existing_binding_loc = loc }
+                  :: tbls.additional_errors;
+                Some existing)
             g.modules
       | _ -> ()
     end;
@@ -4824,7 +4830,7 @@ let rec statement opts scope tbls (loc, stmt) =
       | S.DeclareModule.Literal (id_loc, { Ast.StringLiteral.value; _ }) ->
         (push_loc tbls id_loc, value)
     in
-    let scope = Scope.push_declare_module loc name scope in
+    let scope = Scope.push_declare_module loc name scope tbls in
     let (_, { S.Block.body = stmts; comments = _ }) = body in
     let visit_statement ((_, stmt') as stmt) =
       match
diff --git a/tests/duplicate_libs/duplicate_libs.exp b/tests/duplicate_libs/duplicate_libs.exp
index 154f329e9bb..7ce57c1023b 100644
--- a/tests/duplicate_libs/duplicate_libs.exp
+++ b/tests/duplicate_libs/duplicate_libs.exp
@@ -7,6 +7,15 @@ surprising behaviors. [libdef-override]
                   ^^^^^^^^^^ [1]
 
 
+Error ------------------------------------------------------------------------------------------- lib/shared/lib.js:5:16
+
+This module declaration overrides an existing module `bar` [1]. Overriding in library definitions can lead to surprising
+behaviors. [libdef-override]
+
+   5| declare module 'bar' {
+                     ^^^^^ [1]
+
+
 Error ------------------------------------------------------------------------------------------------------ test.js:1:2
 
 Cannot cast `global_foo` to string because number [1] is incompatible with string [2]. [incompatible-cast]
@@ -24,5 +33,22 @@ References:
                    ^^^^^^ [2]
 
 
+Error ------------------------------------------------------------------------------------------------------ test.js:2:2
+
+Cannot cast `require(...)` to number because string [1] is incompatible with number [2]. [incompatible-cast]
+
+   test.js:2:2
+   2| (require('bar'): number); // error: string ~> number
+       ^^^^^^^^^^^^^^
+
+References:
+   lib/shared/lib.js:6:27
+   6|   declare module.exports: string;
+                                ^^^^^^ [1]
+   test.js:2:18
+   2| (require('bar'): number); // error: string ~> number
+                       ^^^^^^ [2]
+
+
 
-Found 2 errors
+Found 4 errors
diff --git a/tests/duplicate_libs/lib/shared/lib.js b/tests/duplicate_libs/lib/shared/lib.js
index 94c6bace474..544909e6a6c 100644
--- a/tests/duplicate_libs/lib/shared/lib.js
+++ b/tests/duplicate_libs/lib/shared/lib.js
@@ -1,3 +1,7 @@
 // The lib file is included twice, because it is included in both `lib` and
 // `lib/shared`, which are both listed in the `.flowconfig`
 declare var global_foo: number; // intentional-libdef-override
+
+declare module 'bar' {
+  declare module.exports: string;
+}
diff --git a/tests/duplicate_libs/test.js b/tests/duplicate_libs/test.js
index ef878096bd0..69b1af27ff6 100644
--- a/tests/duplicate_libs/test.js
+++ b/tests/duplicate_libs/test.js
@@ -1 +1,2 @@
 (global_foo: string); // error: number ~> string
+(require('bar'): number); // error: string ~> number