[GR-26090] Fix coverage results when source contains eval

PullRequest: truffleruby/1976

Author: bjfish

CHANGELOG.md

@@ -13,6 +13,7 @@ Bug fixes:
 * Fixed issue with `Kernel#freeze` not freezing singleton class (#2093).
 * Fixed `String#encode` with options issue (#2091, #2095, @LillianZ)
 * Fixed issue with `spawn` when `:close` redirect is used (#2097).
+* Fixed `coverage` issue when `*eval` is used (#2078).
 
 Compatibility:
 

spec/ruby/.mspec.constants

@@ -41,6 +41,7 @@ ComparisonTest
 ConstantSpecsIncludedModule
 ConstantVisibility
 Coverage
+CoverageSpecs
 CustomArgumentError
 DRb
 DRbIdConv

spec/ruby/library/coverage/fixtures/eval_code.rb

@@ -0,0 +1,11 @@
+5 + 5
+
+module CoverageSpecs
+
+  class_eval <<-RUBY, __FILE__, __LINE__ + 1
+    attr_reader :ok
+  RUBY
+
+end
+
+4 + 4

spec/ruby/library/coverage/result_spec.rb

@@ -5,11 +5,13 @@
   before :all do
     @class_file = fixture __FILE__, 'some_class.rb'
     @config_file = fixture __FILE__, 'start_coverage.rb'
+    @eval_code_file = fixture __FILE__, 'eval_code.rb'
   end
 
   after :each do
     $LOADED_FEATURES.delete(@class_file)
     $LOADED_FEATURES.delete(@config_file)
+    $LOADED_FEATURES.delete(@eval_code_file)
   end
 
   it 'gives the covered files as a hash with arrays of count or nil' do
@@ -75,4 +77,16 @@
     require @config_file.chomp('.rb')
     Coverage.result.should_not include(@config_file)
   end
+
+  it 'returns the correct results when eval is used' do
+    Coverage.start
+    require @eval_code_file.chomp('.rb')
+    result = Coverage.result
+
+    result.should == {
+        @eval_code_file => [
+            1, nil, 1, nil, 1, nil, nil, nil, nil, nil, 1
+        ]
+    }
+  end
 end

src/main/java/org/truffleruby/language/eval/CreateEvalSourceNode.java

@@ -48,7 +48,7 @@ public RubySource createEvalSource(Rope code, String method, String file, int li
 
         final Source source = Source.newBuilder(TruffleRuby.LANGUAGE_ID, sourceString, file).build();
 
-        return new RubySource(source, file, sourceRope);
+        return new RubySource(source, file, sourceRope, true);
     }
 
     private static Rope createEvalRope(Rope source, String method, String file, int line) {

src/main/java/org/truffleruby/parser/RubySource.java

@@ -24,18 +24,23 @@ public class RubySource {
      * for non-file Sources in the future (but then we'll need to still use this path in Ruby backtraces). */
     private final String sourcePath;
     private final Rope sourceRope;
+    private final boolean isEval;
 
     public RubySource(Source source, String sourcePath) {
-        this(source, sourcePath, null);
+        this(source, sourcePath, null, false);
     }
 
     public RubySource(Source source, String sourcePath, Rope sourceRope) {
-        assert RubyContext.getPath(source).equals(sourcePath) : RubyContext.getPath(source) + " vs " + sourcePath;
+        this(source, sourcePath, sourceRope, false);
+    }
 
+    public RubySource(Source source, String sourcePath, Rope sourceRope, boolean isEval) {
+        assert RubyContext.getPath(source).equals(sourcePath) : RubyContext.getPath(source) + " vs " + sourcePath;
         this.source = Objects.requireNonNull(source);
         //intern() to improve footprint
         this.sourcePath = Objects.requireNonNull(sourcePath).intern();
         this.sourceRope = sourceRope;
+        this.isEval = isEval;
     }
 
     public Source getSource() {
@@ -50,4 +55,7 @@ public Rope getRope() {
         return sourceRope;
     }
 
+    public boolean isEval() {
+        return isEval;
+    }
 }

src/main/java/org/truffleruby/parser/TranslatorDriver.java

@@ -233,7 +233,10 @@ public RubyRootNode parse(RubySource rubySource, ParserContext parserContext, St
 
         // Translate to Ruby Truffle nodes
 
-        context.getCoverageManager().loadingSource(source);
+        // Like MRI, do not track coverage of eval's. This also avoids having to merge multiple Sources with the same RubyContext.getPath().
+        if (!rubySource.isEval()) {
+            context.getCoverageManager().loadingSource(source);
+        }
 
         final BodyTranslator translator = new BodyTranslator(
                 context,

src/main/java/org/truffleruby/stdlib/CoverageManager.java

@@ -21,7 +21,6 @@
 import org.truffleruby.RubyContext;
 import org.truffleruby.SuppressFBWarnings;
 import org.truffleruby.collections.ConcurrentOperations;
-import org.truffleruby.shared.TruffleRuby;
 
 import com.oracle.truffle.api.CompilerDirectives;
 import com.oracle.truffle.api.CompilerDirectives.CompilationFinal;
@@ -88,7 +87,6 @@ public synchronized void enable() {
         binding = instrumenter.attachExecutionEventFactory(
                 SourceSectionFilter
                         .newBuilder()
-                        .mimeTypeIs(TruffleRuby.MIME_TYPE)
                         .sourceIs(coveredSources::contains)
                         .tagIs(LineTag.class)
                         .build(),

src/main/java/org/truffleruby/stdlib/CoverageNodes.java

@@ -9,8 +9,7 @@
  */
 package org.truffleruby.stdlib;
 
-import java.util.ArrayList;
-import java.util.List;
+import java.util.HashMap;
 import java.util.Map;
 
 import org.jcodings.specific.UTF8Encoding;
@@ -60,14 +59,14 @@ public abstract static class CoverageResultNode extends CoreMethodArrayArguments
         @TruffleBoundary
         @Specialization
         protected RubyArray resultArray() {
-            final List<RubyArray> results = new ArrayList<>();
 
             final Map<Source, long[]> counts = getContext().getCoverageManager().getCounts();
 
             if (counts == null) {
                 throw new RaiseException(getContext(), coreExceptions().runtimeErrorCoverageNotEnabled(this));
             }
 
+            Map<String, RubyArray> results = new HashMap<>();
             for (Map.Entry<Source, long[]> source : counts.entrySet()) {
                 final long[] countsArray = source.getValue();
 
@@ -81,16 +80,18 @@ protected RubyArray resultArray() {
                     }
                 }
 
-                results.add(createArray(new Object[]{
+                final String path = RubyContext.getPath(source.getKey());
+                assert !results.containsKey(path) : "path already exists in coverage results";
+                results.put(path, createArray(new Object[]{
                         makeStringNode.executeMake(
-                                RubyContext.getPath(source.getKey()),
+                                path,
                                 UTF8Encoding.INSTANCE,
                                 CodeRange.CR_UNKNOWN),
                         createArray(countsStore)
                 }));
             }
 
-            return createArray(results.toArray());
+            return createArray(results.values().toArray());
         }
 
     }