[modules] Don't throw when detecting cycle while processing star exports.

We must not throw when seeing a cycle while trying to resolve a name through
star exports.  (It may be surprising that we do have to throw when seeing an
ambiguity, but this is what the spec says.)

R=adamk@chromium.org
BUG=v8:1569

Review-Url: https://codereview.chromium.org/2376563002
Cr-Commit-Position: refs/heads/master@{#39787}

src/objects.cc

@@ -19707,22 +19707,25 @@ MaybeHandle<Cell> Module::ResolveExport(Handle<Module> module,
     return Handle<Cell>::cast(object);
   }
 
-  // Must be an indirect export of some sort, so we need to detect cycles.
+  // Check for cycle before recursing.
   {
     // Attempt insertion with a null string set.
     auto result = resolve_set->insert({module, nullptr});
     UnorderedStringSet*& name_set = result.first->second;
-    bool did_insert = result.second;
-    if (did_insert) {
+    if (result.second) {
       // |module| wasn't in the map previously, so allocate a new name set.
       Zone* zone = resolve_set->zone();
       name_set =
           new (zone->New(sizeof(UnorderedStringSet))) UnorderedStringSet(zone);
     } else if (name_set->count(name)) {
-      // TODO(adamk): Throwing here is incorrect in the case of star exports.
-      THROW_NEW_ERROR(
-          isolate,
-          NewSyntaxError(MessageTemplate::kCyclicModuleDependency, name), Cell);
+      // Cycle detected.
+      if (must_resolve) {
+        THROW_NEW_ERROR(
+            isolate,
+            NewSyntaxError(MessageTemplate::kCyclicModuleDependency, name),
+            Cell);
+      }
+      return MaybeHandle<Cell>();
     }
     name_set->insert(name);
   }

src/objects.h

@@ -7989,14 +7989,13 @@ class Module : public Struct {
                                    Handle<ModuleInfoEntry> entry);
 
   // The [must_resolve] argument indicates whether or not an exception should be
-  // thrown if the module does not provide an export named [name].
+  // thrown in case the module does not provide an export named [name]
+  // (including when a cycle is detected).  An exception is always thrown in the
+  // case of conflicting star exports.
   //
   // If [must_resolve] is true, a null result indicates an exception. If
-  // [must_resolve] is false, a null result does not necessarily indicate an
-  // exception, but there may be one pending.
-  //
-  // Currently, an exception is always thrown in the case of a cycle and in the
-  // case of conflicting star exports.  TODO(neis): Make that spec-compliant.
+  // [must_resolve] is false, a null result may or may not indicate an
+  // exception (so check manually!).
   class ResolveSet;
   static MUST_USE_RESULT MaybeHandle<Cell> ResolveExport(
       Handle<Module> module, Handle<String> name, bool must_resolve,

test/mjsunit/modules-fail-star-exports-conflict.js

+// Copyright 2016 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+//
+// MODULE
+
+export * from "modules-skip-star-exports-conflict.js";
+export * from "modules-skip-6.js";
+
+import {a} from "modules-fail-star-exports-conflict.js";

test/mjsunit/modules-skip-star-exports-conflict.js

+// Copyright 2016 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+export * from "modules-skip-1.js";
+export * from "modules-skip-5.js";

test/mjsunit/modules-skip-star-exports-cycle.js

+// Copyright 2016 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+export * from "modules-skip-star-exports-cycle.js";
+export * from "modules-star-exports-cycle.js";

test/mjsunit/modules-star-exports-cycle.js

+// Copyright 2016 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+//
+// MODULE
+
+const bar = 42;
+export {bar as foo};
+
+import {foo} from "modules-skip-star-exports-cycle.js";
+assertEquals(42, foo);