Fix duplicated calls of a Module#const_added callback when a module w…

…ith nested modules is assigned to a constant

CHANGELOG.md

@@ -5,6 +5,7 @@ New features:
 Bug fixes:
 
 * Fix `Module#name` called inside the `Module#const_added` callback when the module is defined in the top-level scope (#3683, @andrykonchin).
+* Fix duplicated calls of a `Module#const_added` callback when a module with nested modules is assigned to a constant (@andrykonchin).
 
 Compatibility:
 

spec/ruby/core/module/const_added_spec.rb

@@ -1,5 +1,6 @@
 require_relative '../../spec_helper'
 require_relative 'fixtures/classes'
+require_relative 'fixtures/const_added'
 
 describe "Module#const_added" do
   ruby_version_is "3.2" do
@@ -63,6 +64,27 @@ module SubModule
       ScratchPad.recorded.should == [:SubModule]
     end
 
+    it "is called when a new module is defined under a named module (assigned to a constant)" do
+      ScratchPad.record []
+
+      ModuleSpecs::ConstAddedSpecs::NamedModule = Module.new do
+        def self.const_added(name)
+          ScratchPad << name
+        end
+
+        module self::A
+          def self.const_added(name)
+            ScratchPad << name
+          end
+
+          module self::B
+          end
+        end
+      end
+
+      ScratchPad.recorded.should == [:A, :B]
+    end
+
     it "is called when a new class is defined under self" do
       ScratchPad.record []
 
@@ -83,6 +105,27 @@ class SubClass
       ScratchPad.recorded.should == [:SubClass]
     end
 
+    it "is called when a new class is defined under a named module (assigned to a constant)" do
+      ScratchPad.record []
+
+      ModuleSpecs::ConstAddedSpecs::NamedModuleB = Module.new do
+        def self.const_added(name)
+          ScratchPad << name
+        end
+
+        class self::A
+          def self.const_added(name)
+            ScratchPad << name
+          end
+
+          class self::B
+          end
+        end
+      end
+
+      ScratchPad.recorded.should == [:A, :B]
+    end
+
     it "is called when an autoload is defined" do
       ScratchPad.record []
 

spec/ruby/core/module/fixtures/const_added.rb

@@ -0,0 +1,4 @@
+module ModuleSpecs
+  module ConstAddedSpecs
+  end
+end

spec/tags/core/module/name_tags.txt

@@ -1,3 +1,2 @@
 fails:Module#name is not nil when assigned to a constant in an anonymous module
-fails:Module#name is set in #const_added callback for a nested module when an outer module defined in the top-level scope
 slow:Module#name is set in #const_added callback when a module defined in the top-level scope

src/main/java/org/truffleruby/core/CoreLibrary.java

@@ -299,11 +299,11 @@ public CoreLibrary(RubyContext context, RubyLanguage language) {
         objectClass = (RubyClass) moduleClass.superclass;
         basicObjectClass = (RubyClass) objectClass.superclass;
 
-        // Set constants in Object and lexical parents
-        classClass.fields.getAdoptedByLexicalParent(context, objectClass, "Class", node);
-        basicObjectClass.fields.getAdoptedByLexicalParent(context, objectClass, "BasicObject", node);
-        objectClass.fields.getAdoptedByLexicalParent(context, objectClass, "Object", node);
-        moduleClass.fields.getAdoptedByLexicalParent(context, objectClass, "Module", node);
+        // Set constants in Object
+        objectClass.fields.setConstant(context, node, "Class", classClass);
+        objectClass.fields.setConstant(context, node, "BasicObject", basicObjectClass);
+        objectClass.fields.setConstant(context, node, "Object", objectClass);
+        objectClass.fields.setConstant(context, node, "Module", moduleClass);
 
         // Create Exception classes
 

src/main/java/org/truffleruby/core/klass/ClassNodes.java

@@ -119,7 +119,7 @@ private static RubyClass createRubyClass(RubyContext context,
                 superclass);
 
         if (lexicalParent != null) {
-            rubyClass.fields.getAdoptedByLexicalParent(context, lexicalParent, name, currentNode);
+            lexicalParent.fields.setConstant(context, currentNode, name, rubyClass);
         }
 
         return rubyClass;

src/main/java/org/truffleruby/core/module/ModuleFields.java

@@ -165,53 +165,34 @@ public void afterConstructed() {
         getName();
     }
 
-    public RubyConstant getAdoptedByLexicalParent(
+    private void nameModule(
             RubyContext context,
             RubyModule lexicalParent,
-            String name,
-            Node currentNode) {
+            String name) {
         assert name != null;
 
         if (!hasFullName()) {
             if (lexicalParent == getObjectClass()) {
                 this.setFullName(name);
-                updateAnonymousChildrenModules(context);
+                nameChildrenModules(context);
             } else if (lexicalParent.fields.hasFullName()) {
                 this.setFullName(lexicalParent.fields.getName() + "::" + name);
-                updateAnonymousChildrenModules(context);
+                nameChildrenModules(context);
             } else {
                 // Our lexicalParent is also an anonymous module
                 // and will name us when it gets named via updateAnonymousChildrenModules(),
                 // or we'll compute an anonymous name on #getName() if needed
             }
         }
-
-        // A module correct final name should be assigned by this time (by #setFullName() method call).
-        // So a temporary anonymous module name isn't visible in the Module#const_added callback, that
-        // is called inside #setConstantInternal().
-        RubyConstant previous = lexicalParent.fields.setConstantInternal(
-                context,
-                currentNode,
-                name,
-                rubyModule,
-                false);
-
-        return previous;
     }
 
-    public void updateAnonymousChildrenModules(RubyContext context) {
+    private void nameChildrenModules(RubyContext context) {
         for (Map.Entry<String, ConstantEntry> entry : constants.entrySet()) {
             ConstantEntry constantEntry = entry.getValue();
             RubyConstant constant = constantEntry.getConstant();
-            if (constant != null && constant.hasValue() && constant.getValue() instanceof RubyModule) {
-                RubyModule module = (RubyModule) constant.getValue();
-                if (!module.fields.hasFullName()) {
-                    module.fields.getAdoptedByLexicalParent(
-                            context,
-                            rubyModule,
-                            entry.getKey(),
-                            null);
-                }
+
+            if (constant != null && constant.hasValue() && constant.getValue() instanceof RubyModule childModule) {
+                childModule.fields.nameModule(context, rubyModule, entry.getKey());
             }
         }
     }
@@ -429,15 +410,13 @@ private List<RubyModule> getPrependedModulesAndSelf() {
     /** Set the value of a constant, possibly redefining it. */
     @TruffleBoundary
     public RubyConstant setConstant(RubyContext context, Node currentNode, String name, Object value) {
-        if (value instanceof RubyModule) {
-            return ((RubyModule) value).fields.getAdoptedByLexicalParent(
-                    context,
-                    rubyModule,
-                    name,
-                    currentNode);
-        } else {
-            return setConstantInternal(context, currentNode, name, value, false);
+        // A module fully qualified name should replace a temporary one before assigning a constant,
+        // and before calling in the #const_added callback.
+        if (value instanceof RubyModule childModule) {
+            childModule.fields.nameModule(context, rubyModule, name);
         }
+
+        return setConstantInternal(context, currentNode, name, value, false);
     }
 
     @TruffleBoundary

src/main/java/org/truffleruby/core/module/ModuleNodes.java

@@ -158,7 +158,7 @@ public static RubyModule createModule(RubyContext context, SourceSection sourceS
         module.fields.afterConstructed();
 
         if (lexicalParent != null) {
-            module.fields.getAdoptedByLexicalParent(context, lexicalParent, name, currentNode);
+            lexicalParent.fields.setConstant(context, currentNode, name, module);
         }
         return module;
     }