[GR-18163] Warn when a global variable is not initialized

PullRequest: truffleruby/3382

Author: andrykonchin

CHANGELOG.md

@@ -10,6 +10,7 @@ Bug fixes:
 Compatibility:
 
 * Fix `Array#fill` to raise `TypeError` instead of `ArgumentError` when the length argument is not numeric (#2652, @andrykonchin).
+* Warn when a global variable is not initialized (#2595, @andrykonchin).
 
 Performance:
 

spec/ruby/language/predefined_spec.rb

@@ -570,7 +570,6 @@ def foo
     ($/ = "xyz").should == "xyz"
   end
 
-
   it "changes $-0" do
     $/ = "xyz"
     $-0.should equal($/)
@@ -641,6 +640,45 @@ def foo
   end
 end
 
+describe "Predefined global $\\" do
+  before :each do
+    @verbose, $VERBOSE = $VERBOSE, nil
+    @dollar_backslash = $\
+  end
+
+  after :each do
+    $\ = @dollar_backslash
+    $VERBOSE = @verbose
+  end
+
+  it "can be assigned a String" do
+    str = "abc"
+    $\ = str
+    $\.should equal(str)
+  end
+
+  it "can be assigned nil" do
+    $\ = nil
+    $\.should be_nil
+  end
+
+  it "returns the value assigned" do
+    ($\ = "xyz").should == "xyz"
+  end
+
+  it "does not call #to_str to convert the object to a String" do
+    obj = mock("$\\ value")
+    obj.should_not_receive(:to_str)
+
+    -> { $\ = obj }.should raise_error(TypeError)
+  end
+
+  it "raises a TypeError if assigned not String" do
+    -> { $\ = 1 }.should raise_error(TypeError)
+    -> { $\ = true }.should raise_error(TypeError)
+  end
+end
+
 describe "Predefined global $," do
   after :each do
     $, = nil
@@ -1340,3 +1378,29 @@ def obj.foo2; yield; end
     end
   end
 end
+
+# Some other pre-defined global variables
+
+describe "Predefined global $=" do
+  before :each do
+    @verbose, $VERBOSE = $VERBOSE, nil
+    @dollar_assign = $=
+  end
+
+  after :each do
+    $= = @dollar_assign
+    $VERBOSE = @verbose
+  end
+
+  it "warns when accessed" do
+    -> { a = $= }.should complain(/is no longer effective/)
+  end
+
+  it "warns when assigned" do
+    -> { $= = "_" }.should complain(/is no longer effective/)
+  end
+
+  it "returns the value assigned" do
+    ($= = "xyz").should == "xyz"
+  end
+end

spec/ruby/language/variables_spec.rb

@@ -849,4 +849,22 @@ def obj.foobar; @a ||= 42; end
       -> { obj.foobar }.should_not complain(verbose: true)
     end
   end
+
+  describe "global variable" do
+    context "when global variable is uninitialized" do
+      it "warns about accessing uninitialized global variable in verbose mode" do
+        obj = Object.new
+        def obj.foobar; a = $specs_uninitialized_global_variable; end
+
+        -> { obj.foobar }.should complain(/warning: global variable `\$specs_uninitialized_global_variable' not initialized/, verbose: true)
+      end
+
+      it "doesn't warn at lazy initialization" do
+        obj = Object.new
+        def obj.foobar; $specs_uninitialized_global_variable_lazy ||= 42; end
+
+        -> { obj.foobar }.should_not complain(verbose: true)
+      end
+    end
+  end
 end

src/main/java/org/truffleruby/language/globals/GlobalVariableStorage.java

@@ -21,7 +21,7 @@
 
 public final class GlobalVariableStorage {
 
-    private static final Object UNSET_VALUE = NotProvided.INSTANCE;
+    public static final Object UNSET_VALUE = NotProvided.INSTANCE;
 
     private final CyclicAssumption unchangedAssumption = new CyclicAssumption("global variable unchanged");
     private int changes = 0;
@@ -53,6 +53,10 @@ public Object getValue() {
         return currentValue == UNSET_VALUE ? Nil.INSTANCE : currentValue;
     }
 
+    public Object getRawValue() {
+        return value;
+    }
+
     public boolean isDefined() {
         return value != UNSET_VALUE;
     }

src/main/java/org/truffleruby/language/globals/ReadSimpleGlobalVariableNode.java

@@ -10,10 +10,15 @@
 package org.truffleruby.language.globals;
 
 import com.oracle.truffle.api.CompilerDirectives;
+import com.oracle.truffle.api.profiles.ConditionProfile;
+import com.oracle.truffle.api.source.SourceSection;
+import org.truffleruby.core.string.StringUtils;
+import org.truffleruby.language.Nil;
 import org.truffleruby.language.RubyBaseNode;
 
 import com.oracle.truffle.api.dsl.Cached;
 import com.oracle.truffle.api.dsl.Specialization;
+import org.truffleruby.language.WarningNode;
 
 public abstract class ReadSimpleGlobalVariableNode extends RubyBaseNode {
 
@@ -38,19 +43,50 @@ public ReadSimpleGlobalVariableNode(String name) {
     protected Object readConstant(
             @Cached("getLanguage().getGlobalVariableIndex(name)") int index,
             @Cached("getContext().getGlobalVariableStorage(index)") GlobalVariableStorage storage,
-            @Cached("storage.getValue()") Object value) {
+            @Cached("storage.getValue()") Object value,
+            @Cached("new()") WarningNode warningNode,
+            @Cached("storage.isDefined()") boolean isDefined) {
+        if (!isDefined && warningNode.shouldWarn()) {
+            SourceSection sourceSection = getEncapsulatingSourceSection();
+            String message = globalVariableNotInitializedMessageFor(name);
+
+            warningNode.warningMessage(sourceSection, message);
+        }
+
         return value;
     }
 
     @Specialization
-    protected Object read() {
+    protected Object read(
+            @Cached("new()") WarningNode warningNode,
+            @Cached ConditionProfile isDefinedProfile) {
         if (lookupGlobalVariableStorageNode == null) {
             CompilerDirectives.transferToInterpreterAndInvalidate();
             lookupGlobalVariableStorageNode = insert(LookupGlobalVariableStorageNode.create(name));
         }
+
         final GlobalVariableStorage storage = lookupGlobalVariableStorageNode.execute(null);
 
-        return storage.getValue();
+        // don't use storage.getValue() and storage.isDefined() to avoid
+        // accessing volatile storage.value several times
+        final Object rawValue = storage.getRawValue();
+
+        if (isDefinedProfile.profile(rawValue != GlobalVariableStorage.UNSET_VALUE)) {
+            return rawValue;
+        } else {
+            if (warningNode.shouldWarn()) {
+                SourceSection sourceSection = getEncapsulatingSourceSection();
+                String message = globalVariableNotInitializedMessageFor(name);
+
+                warningNode.warningMessage(sourceSection, message);
+            }
+
+            return Nil.INSTANCE;
+        }
+    }
+
+    private String globalVariableNotInitializedMessageFor(String name) {
+        return StringUtils.format("global variable `%s' not initialized", name);
     }
 
 }

src/main/ruby/truffleruby/core/truffle/kernel_operations.rb

@@ -85,6 +85,18 @@ def $LOAD_PATH.resolve_feature_path(file_name)
 
     $/ = "\n".freeze
 
+    define_hooked_variable(
+      :$\,
+      -> { Primitive.global_variable_get :$\ },
+      -> v {
+        if v && !Primitive.object_kind_of?(v, String)
+          raise TypeError, '$\ must be a String'
+        end
+        Primitive.global_variable_set :$\, v
+      })
+
+    $\ = nil
+
     Truffle::Boot.delay do
       if Truffle::Boot.get_option 'chomp-loop'
         $\ = $/
@@ -121,7 +133,18 @@ def $LOAD_PATH.resolve_feature_path(file_name)
 
     $, = nil # It should be defined by the time boot has finished.
 
-    $= = false
+    define_hooked_variable(
+      :$=,
+      -> {
+        warn 'variable $= is no longer effective', uplevel: 1 if Warning[:deprecated]
+        Primitive.global_variable_get :$=
+      },
+      -> v {
+        warn 'variable $= is no longer effective', uplevel: 1 if Warning[:deprecated]
+        Primitive.global_variable_set :$=, v
+      })
+
+    Primitive.global_variable_set :$=, false
 
     define_hooked_variable(
       :$VERBOSE,
@@ -173,6 +196,8 @@ def $LOAD_PATH.resolve_feature_path(file_name)
         Primitive.global_variable_set :"$;", v
       })
 
+    $; = nil
+
     def self.load_error(name)
       load_error = LoadError.new("cannot load such file -- #{name}")
       load_error.path = name