Change order of module creation steps and assign a module's full name…

… before calling the Module#const_added callback

CHANGELOG.md

@@ -13,6 +13,7 @@ Compatibility:
 * Add `MAJOR`, `MINOR`, `TEENY`, `PATCHLEVEL`, `RUBY_API_VERSION`, and `RUBY_PROGRAM_VERSION` to `RbConfig::CONFIG` (#3396, @rwstauner).
 * Set `RbConfig::CONFIG['archincludedir']` (#3396, @andrykonchin).
 * Support the index/length arguments for the string argument to `String#bytesplice` added in 3.3 (#3656, @rwstauner).
+* Fix `Module#name` called inside the `Module#const_added` callback when the module is defined in the top-level scope (#3683, @andrykonchin).
 
 Performance:
 

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

@@ -7,4 +7,7 @@ def name
       Cß.name
     end
   end
+
+  module NameSpecs
+  end
 end

spec/ruby/core/module/name_spec.rb

@@ -140,6 +140,48 @@ module ModuleSpecs::Anonymous
     valid_names.should include(m::N.name) # You get one of the two, but you don't know which one.
   end
 
+  ruby_version_is "3.2" do
+    it "is set in #const_added callback when a module defined in the top-level scope" do
+      ruby_exe(<<~RUBY, args: "2>&1").chomp.should == "TEST1\nTEST2"
+        class Module
+          def const_added(name)
+            puts const_get(name).name
+          end
+        end
+        
+        # module with name
+        module TEST1
+        end
+        
+        # anonymous module
+        TEST2 = Module.new
+      RUBY
+    end
+
+    it "is set in #const_added callback for a nested module when an outer module defined in the top-level scope" do
+      ScratchPad.record []
+
+      ModuleSpecs::NameSpecs::NamedModule = Module.new do
+        def self.const_added(name)
+          ScratchPad << const_get(name).name
+        end
+
+        module self::A
+          def self.const_added(name)
+            ScratchPad << const_get(name).name
+          end
+
+          module self::B
+          end
+        end
+      end
+
+      ScratchPad.recorded.size.should == 2
+      ScratchPad.recorded[0].should =~ /#<Module.+>::A$/
+      ScratchPad.recorded[1].should =~ /#<Module.+>::A::B$/
+    end
+  end
+
   it "returns a frozen String" do
     ModuleSpecs.name.should.frozen?
   end

spec/tags/core/module/name_tags.txt

@@ -1 +1,3 @@
 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/module/ModuleFields.java

@@ -172,19 +172,8 @@ public RubyConstant getAdoptedByLexicalParent(
             Node currentNode) {
         assert name != null;
 
-        RubyConstant previous = lexicalParent.fields.setConstantInternal(
-                context,
-                currentNode,
-                name,
-                rubyModule,
-                false);
-
         if (!hasFullName()) {
-            // Tricky, we need to compare with the Object class, but we only have a Class at hand.
-            final RubyClass classClass = getLogicalClass().getLogicalClass();
-            final RubyClass objectClass = (RubyClass) ((RubyClass) classClass.superclass).superclass;
-
-            if (lexicalParent == objectClass) {
+            if (lexicalParent == getObjectClass()) {
                 this.setFullName(name);
                 updateAnonymousChildrenModules(context);
             } else if (lexicalParent.fields.hasFullName()) {
@@ -196,6 +185,17 @@ public RubyConstant getAdoptedByLexicalParent(
                 // 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;
     }
 
@@ -792,7 +792,11 @@ public Object getRubyStringName() {
     @TruffleBoundary
     private String createAnonymousName() {
         if (givenBaseName != null) {
-            return lexicalParent.fields.getName() + "::" + givenBaseName;
+            if (lexicalParent == getObjectClass()) {
+                return givenBaseName;
+            } else {
+                return lexicalParent.fields.getName() + "::" + givenBaseName;
+            }
         } else if (getLogicalClass() == rubyModule) { // For the case of class Class during initialization
             return "#<cyclic>";
         } else if (RubyGuards.isSingletonClass(rubyModule)) {
@@ -1181,4 +1185,12 @@ private void refinedMethod(RubyContext context, String methodName, Node currentN
         }
     }
 
+    private RubyClass getObjectClass() {
+        // Tricky, we need to compare with the Object class, but we only have a Module at hand.
+        final RubyClass classClass = getLogicalClass().getLogicalClass();
+        final RubyClass objectClass = (RubyClass) ((RubyClass) classClass.superclass).superclass;
+        assert objectClass.fields.givenBaseName == "Object";
+        return objectClass;
+    }
+
 }