[GR-18163] Change order of module creation steps and assign a module's full name before calling the Module#const_added callback

PullRequest: truffleruby/4375

Author: andrykonchin

CHANGELOG.md

@@ -4,6 +4,9 @@ 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:
 
 * Fix `Module#include` so a module included into a reopened nested module is added into an ancestors chain (#3570, @andrykonchin).

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/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,47 @@ 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.should.one?(/#<Module.+>::A$/)
+      ScratchPad.recorded.should.one?(/#<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,2 @@
 fails:Module#name is not nil when assigned to a constant in an anonymous module
+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;
 
-        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);
+                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
             }
         }
-        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
@@ -792,7 +771,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 +1164,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;
+    }
+
 }

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;
     }

test/mri/excludes/TestSetTraceFunc.rb

@@ -26,6 +26,7 @@
 exclude :test_raise, "<[\"c-return\", 1, :set_trace_func, Kernel]> expected but was <nil>."
 exclude :test_rb_rescue, "ArgumentError: unknown event: a_call"
 exclude :test_recursive, "| <internal:core> core/tracepoint.rb:24:in `block in initialize': unknown event: c_call (ArgumentError)"
+exclude :test_remove_in_trace, "transient: [ruby-dev:42350]. <#<TestSetTraceFunc:...> expected but was <false>"
 exclude :test_rescue_and_ensure_should_not_cause_b_return, "ArgumentError: unknown event: a_call"
 exclude :test_return, "<[\"c-return\", 1, :set_trace_func, Kernel]> expected but was <nil>."
 exclude :test_return2, "<[\"c-return\", 1, :set_trace_func, Kernel]> expected but was <nil>."

test/mri/excludes/TestThread.rb

@@ -1,4 +1,5 @@
 exclude :test_abort_on_exception, "| Exception: `RuntimeError' at -:3:in `block in <main>' - "
+exclude :test_handle_interrupt, "transient: <[:on_blocking, :c1]> expected but was <[:on_blocking, :c2]>."
 exclude :test_handle_interrupt_and_io, "needs investigation"
 exclude :test_handle_interrupt_and_p, "transient"
 exclude :test_handle_interrupt_blocking, "<:ok> expected but was <nil>."