Skip to content

Commit ec974a9

Browse files
eregoneregon
committed
[GR-14590] Fix direct require after autoload
PullRequest: truffleruby/711
2 parents def97c5 + 450c16e commit ec974a9

File tree

15 files changed

+227
-85
lines changed

15 files changed

+227
-85
lines changed

‎CHANGELOG.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,9 @@ Changes:
1010
unsupported, have been removed.
1111
* Our experimental JRuby-compatible Java interop has been removed - use
1212
`Polyglot` and `Java` instead.
13+
Bug fixes:
14+
15+
* `autoload :C, "path"; require "path"` now correctly triggers the autoload.
1316

1417
# 1.0 RC 14
1518

‎lib/mri/net/http.rb

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -22,13 +22,7 @@
2222

2323
require_relative 'protocol'
2424
require 'uri'
25-
26-
if RUBY_ENGINE == 'truffleruby'
27-
# See tagged specs around autoload
28-
require 'openssl'
29-
else
30-
autoload :OpenSSL, 'openssl'
31-
end
25+
autoload :OpenSSL, 'openssl'
3226

3327
module Net #:nodoc:
3428

‎spec/ruby/core/module/autoload_spec.rb

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -217,7 +217,7 @@ module ModuleSpecs::Autoload
217217
end
218218
end
219219

220-
describe "the autoload is removed when the same file is required directly without autoload" do
220+
describe "the autoload is triggered when the same file is required directly" do
221221
before :each do
222222
module ModuleSpecs::Autoload
223223
autoload :RequiredDirectly, fixture(__FILE__, "autoload_required_directly.rb")
@@ -260,7 +260,7 @@ module ModuleSpecs::Autoload
260260
nested_require = -> {
261261
result = nil
262262
ScratchPad.record -> {
263-
result = [@check.call, Thread.new { @check.call }.value]
263+
result = @check.call
264264
}
265265
require nested
266266
result
@@ -269,9 +269,7 @@ module ModuleSpecs::Autoload
269269

270270
@check.call.should == ["constant", @path]
271271
require @path
272-
cur, other = ScratchPad.recorded
273-
cur.should == [nil, nil]
274-
other.should == [nil, nil]
272+
ScratchPad.recorded.should == [nil, nil]
275273
@check.call.should == ["constant", nil]
276274
end
277275
end

‎spec/ruby/core/module/const_set_spec.rb

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,53 @@
7272
lambda { ConstantSpecs.const_set name, 1 }.should raise_error(TypeError)
7373
end
7474

75+
describe "when overwriting an existing constant" do
76+
it "warns if the previous value was a normal value" do
77+
mod = Module.new
78+
mod.const_set :Foo, 42
79+
-> {
80+
mod.const_set :Foo, 1
81+
}.should complain(/already initialized constant/)
82+
mod.const_get(:Foo).should == 1
83+
end
84+
85+
it "does not warn if the previous value was an autoload" do
86+
mod = Module.new
87+
mod.autoload :Foo, "not-existing"
88+
-> {
89+
mod.const_set :Foo, 1
90+
}.should_not complain
91+
mod.const_get(:Foo).should == 1
92+
end
93+
94+
it "does not warn if the previous value was undefined" do
95+
path = fixture(__FILE__, "autoload_o.rb")
96+
ScratchPad.record []
97+
mod = Module.new
98+
99+
mod.autoload :Foo, path
100+
-> { mod::Foo }.should raise_error(NameError)
101+
102+
mod.should have_constant(:Foo)
103+
mod.const_defined?(:Foo).should == false
104+
mod.autoload?(:Foo).should == nil
105+
106+
-> {
107+
mod.const_set :Foo, 1
108+
}.should_not complain
109+
mod.const_get(:Foo).should == 1
110+
end
111+
112+
it "does not warn if the new value is an autoload" do
113+
mod = Module.new
114+
mod.const_set :Foo, 42
115+
-> {
116+
mod.autoload :Foo, "not-existing"
117+
}.should_not complain
118+
mod.const_get(:Foo).should == 42
119+
end
120+
end
121+
75122
describe "on a frozen module" do
76123
before :each do
77124
@frozen = Module.new.freeze

‎spec/tags/core/module/autoload_tags.txt

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1 @@
11
fails:Module#autoload (concurrently) blocks others threads while doing an autoload
2-
fails:Module#autoload the autoload is removed when the same file is required directly without autoload with a full path
3-
fails:Module#autoload the autoload is removed when the same file is required directly without autoload with a relative path
4-
fails:Module#autoload the autoload is removed when the same file is required directly without autoload in a nested require

‎src/main/java/org/truffleruby/collections/ConcurrentOperations.java

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,10 +19,10 @@ public abstract class ConcurrentOperations {
1919
* Replaces {@link ConcurrentHashMap#computeIfAbsent(Object, Function)} as it does not scale.
2020
* The JDK method takes a monitor for every access if the key is present.
2121
* See https://bugs.openjdk.java.net/browse/JDK-8161372 which only fixes it in Java 9
22-
* if they are no collisions in the bucket.
22+
* if there are no collisions in the bucket.
2323
* This method might execute the function multiple times, in contrast to computeIfAbsent().
2424
*/
25-
public static <V, K> V getOrCompute(Map<K, V> map, K key, Function<? super K, ? extends V> compute) {
25+
public static <K, V> V getOrCompute(Map<K, V> map, K key, Function<? super K, ? extends V> compute) {
2626
V value = map.get(key);
2727
if (value != null) {
2828
return value;
@@ -33,4 +33,13 @@ public static <V, K> V getOrCompute(Map<K, V> map, K key, Function<? super K, ?
3333
}
3434
}
3535

36+
/**
37+
* Similar to {@link Map#replace(Object, Object, Object)} except that the old value can also be
38+
* null (missing). Returns true if the replace succeeded, or false if it should be retried
39+
* because <code>map.get(key)</code> is no longer associated to <code>oldValue</code>.
40+
*/
41+
public static <K, V> boolean replace(Map<K, V> map, K key, V oldValue, V newValue) {
42+
return oldValue == null ? map.putIfAbsent(key, newValue) == null : map.replace(key, oldValue, newValue);
43+
}
44+
3645
}

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

Lines changed: 20 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020

2121
import org.truffleruby.Layouts;
2222
import org.truffleruby.RubyLanguage;
23+
import org.truffleruby.collections.ConcurrentOperations;
2324
import org.truffleruby.RubyContext;
2425
import org.truffleruby.core.klass.ClassNodes;
2526
import org.truffleruby.core.method.MethodFilter;
@@ -320,35 +321,36 @@ public RubyConstant setConstant(RubyContext context, Node currentNode, String na
320321
@TruffleBoundary
321322
public void setAutoloadConstant(RubyContext context, Node currentNode, String name, DynamicObject filename) {
322323
assert RubyGuards.isRubyString(filename);
324+
RubyConstant autoloadConstant = setConstantInternal(context, currentNode, name, filename, true);
323325
if (context.getOptions().LOG_AUTOLOAD) {
324-
RubyLanguage.LOGGER.info(() -> String.format("%s: setting up autoload %s::%s with %s",
326+
RubyLanguage.LOGGER.info(() -> String.format("%s: setting up autoload %s with %s",
325327
context.fileLine(context.getCallStack().getTopMostUserSourceSection()),
326-
getName(), name, filename));
328+
autoloadConstant, filename));
327329
}
328-
setConstantInternal(context, currentNode, name, filename, true);
330+
context.getFeatureLoader().addAutoload(autoloadConstant);
329331
}
330332

331333
private RubyConstant setConstantInternal(RubyContext context, Node currentNode, String name, Object value, boolean autoload) {
332334
checkFrozen(context, currentNode);
333335

334336
SharedObjects.propagate(context, rubyModuleObject, value);
335337

336-
while (true) {
337-
final RubyConstant previous = constants.get(name);
338-
final boolean isPrivate = previous != null && previous.isPrivate();
339-
final boolean isDeprecated = previous != null && previous.isDeprecated();
340-
final SourceSection sourceSection = currentNode != null ? currentNode.getSourceSection() : null;
341-
final RubyConstant newValue = new RubyConstant(rubyModuleObject, value, isPrivate, autoload, isDeprecated, sourceSection);
338+
RubyConstant previous;
339+
RubyConstant newConstant;
340+
do {
341+
previous = constants.get(name);
342+
newConstant = newConstant(currentNode, name, value, autoload, previous);
343+
} while (!ConcurrentOperations.replace(constants, name, previous, newConstant));
342344

343-
if ((previous == null) ? (constants.putIfAbsent(name, newValue) == null) : constants.replace(name, previous, newValue)) {
344-
newConstantsVersion();
345-
if (!autoload && previous != null && !previous.isAutoload()) {
346-
return previous;
347-
} else {
348-
return null;
349-
}
350-
}
351-
}
345+
newConstantsVersion();
346+
return autoload ? newConstant : previous;
347+
}
348+
349+
private RubyConstant newConstant(Node currentNode, String name, Object value, boolean autoload, RubyConstant previous) {
350+
final boolean isPrivate = previous != null && previous.isPrivate();
351+
final boolean isDeprecated = previous != null && previous.isDeprecated();
352+
final SourceSection sourceSection = currentNode != null ? currentNode.getSourceSection() : null;
353+
return new RubyConstant(rubyModuleObject, name, value, isPrivate, autoload, isDeprecated, sourceSection);
352354
}
353355

354356
@TruffleBoundary

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

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -557,19 +557,19 @@ public DynamicObject autoload(DynamicObject module, String name, DynamicObject f
557557
}
558558

559559
@CoreMethod(names = "autoload?", required = 1)
560-
public abstract static class AutoloadQueryNode extends CoreMethodArrayArgumentsNode {
560+
public abstract static class IsAutoloadNode extends CoreMethodArrayArgumentsNode {
561561

562562
@Specialization(guards = "isRubySymbol(name)")
563-
public Object autoloadQuerySymbol(DynamicObject module, DynamicObject name) {
564-
return autoloadQuery(module, Layouts.SYMBOL.getString(name));
563+
public Object isAutoloadSymbol(DynamicObject module, DynamicObject name) {
564+
return isAutoload(module, Layouts.SYMBOL.getString(name));
565565
}
566566

567567
@Specialization(guards = "isRubyString(name)")
568-
public Object autoloadQueryString(DynamicObject module, DynamicObject name) {
569-
return autoloadQuery(module, StringOperations.getString(name));
568+
public Object isAutoloadString(DynamicObject module, DynamicObject name) {
569+
return isAutoload(module, StringOperations.getString(name));
570570
}
571571

572-
private Object autoloadQuery(DynamicObject module, String name) {
572+
private Object isAutoload(DynamicObject module, String name) {
573573
final ConstantLookupResult constant = ModuleOperations.lookupConstant(getContext(), module, name);
574574

575575
if (constant.isAutoload() && !constant.getConstant().isAutoloadingThread()) {
@@ -997,7 +997,7 @@ public Object setConstant(DynamicObject module, String name, Object value) {
997997
@TruffleBoundary
998998
public Object setConstantNoCheckName(DynamicObject module, String name, Object value) {
999999
final RubyConstant previous = Layouts.MODULE.getFields(module).setConstant(getContext(), this, name, value);
1000-
if (previous != null) {
1000+
if (previous != null && previous.hasValue()) {
10011001
warnAlreadyInitializedConstant(module, name, previous.getSourceSection());
10021002
}
10031003
return value;

‎src/main/java/org/truffleruby/language/RubyConstant.java

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
public class RubyConstant implements ObjectGraphNode {
2424

2525
private final DynamicObject declaringModule;
26+
private final String name;
2627
private final Object value;
2728
private final boolean isPrivate;
2829
private final boolean isDeprecated;
@@ -34,13 +35,14 @@ public class RubyConstant implements ObjectGraphNode {
3435

3536
private final SourceSection sourceSection;
3637

37-
public RubyConstant(DynamicObject declaringModule, Object value, boolean isPrivate, boolean autoload, boolean isDeprecated, SourceSection sourceSection) {
38-
this(declaringModule, value, isPrivate, autoload, false, isDeprecated, sourceSection);
38+
public RubyConstant(DynamicObject declaringModule, String name, Object value, boolean isPrivate, boolean autoload, boolean isDeprecated, SourceSection sourceSection) {
39+
this(declaringModule, name, value, isPrivate, autoload, false, isDeprecated, sourceSection);
3940
}
4041

41-
private RubyConstant(DynamicObject declaringModule, Object value, boolean isPrivate, boolean autoload, boolean undefined, boolean isDeprecated, SourceSection sourceSection) {
42+
private RubyConstant(DynamicObject declaringModule, String name, Object value, boolean isPrivate, boolean autoload, boolean undefined, boolean isDeprecated, SourceSection sourceSection) {
4243
assert RubyGuards.isRubyModule(declaringModule);
4344
this.declaringModule = declaringModule;
45+
this.name = name;
4446
this.value = value;
4547
this.isPrivate = isPrivate;
4648
this.isDeprecated = isDeprecated;
@@ -53,6 +55,10 @@ public DynamicObject getDeclaringModule() {
5355
return declaringModule;
5456
}
5557

58+
public String getName() {
59+
return name;
60+
}
61+
5662
public boolean hasValue() {
5763
return !autoload && !undefined;
5864
}
@@ -78,21 +84,21 @@ public RubyConstant withPrivate(boolean isPrivate) {
7884
if (isPrivate == this.isPrivate) {
7985
return this;
8086
} else {
81-
return new RubyConstant(declaringModule, value, isPrivate, autoload, undefined, isDeprecated, sourceSection);
87+
return new RubyConstant(declaringModule, name, value, isPrivate, autoload, undefined, isDeprecated, sourceSection);
8288
}
8389
}
8490

8591
public RubyConstant withDeprecated() {
8692
if (this.isDeprecated()) {
8793
return this;
8894
} else {
89-
return new RubyConstant(declaringModule, value, isPrivate, autoload, undefined, true, sourceSection);
95+
return new RubyConstant(declaringModule, name, value, isPrivate, autoload, undefined, true, sourceSection);
9096
}
9197
}
9298

9399
public RubyConstant undefined() {
94100
assert autoload;
95-
return new RubyConstant(declaringModule, null, isPrivate, false, true, isDeprecated, sourceSection);
101+
return new RubyConstant(declaringModule, name, null, isPrivate, false, true, isDeprecated, sourceSection);
96102
}
97103

98104
@TruffleBoundary
@@ -180,4 +186,9 @@ public void getAdjacentObjects(Set<DynamicObject> adjacent) {
180186
}
181187
}
182188

189+
@Override
190+
public String toString() {
191+
return Layouts.MODULE.getFields(getDeclaringModule()).getName() + "::" + getName();
192+
}
193+
183194
}

‎src/main/java/org/truffleruby/language/constants/GetConstantNode.java

Lines changed: 13 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -20,12 +20,10 @@
2020
import org.truffleruby.RubyLanguage;
2121
import org.truffleruby.core.module.ModuleFields;
2222
import org.truffleruby.core.module.ModuleOperations;
23-
import org.truffleruby.core.string.StringOperations;
2423
import org.truffleruby.language.LexicalScope;
2524
import org.truffleruby.language.RubyBaseNode;
2625
import org.truffleruby.language.RubyConstant;
2726
import org.truffleruby.language.dispatch.CallDispatchHeadNode;
28-
import org.truffleruby.language.loader.FeatureLoader;
2927

3028
public abstract class GetConstantNode extends RubyBaseNode {
3129

@@ -63,44 +61,36 @@ protected Object autoloadConstant(LexicalScope lexicalScope, DynamicObject modul
6361
@Cached("createPrivate()") CallDispatchHeadNode callRequireNode) {
6462

6563
final DynamicObject feature = autoloadConstant.getAutoloadPath();
66-
final DynamicObject autoloadConstantModule = autoloadConstant.getDeclaringModule();
67-
final ModuleFields fields = Layouts.MODULE.getFields(autoloadConstantModule);
6864

6965
if (autoloadConstant.isAutoloadingThread()) {
7066
// Pretend the constant does not exist while it is autoloading
7167
return doMissingConstant(module, name, getSymbol(name));
7268
}
7369

74-
final FeatureLoader featureLoader = getContext().getFeatureLoader();
75-
final String expandedPath = featureLoader.findFeature(StringOperations.getString(feature));
76-
if (expandedPath != null && featureLoader.getFileLocks().isCurrentThreadHoldingLock(expandedPath)) {
77-
// We found an autoload constant while we are already require-ing the autoload file,
78-
// consider it missing to avoid circular require warnings and calling #require twice.
79-
// For instance, autoload :RbConfig, "rbconfig"; require "rbconfig" causes this.
80-
if (getContext().getOptions().LOG_AUTOLOAD) {
81-
RubyLanguage.LOGGER.info(() -> String.format("%s: %s::%s is being treated as missing while loading %s",
82-
getContext().fileLine(getContext().getCallStack().getTopMostUserSourceSection()),
83-
Layouts.MODULE.getFields(module).getName(), name, expandedPath));
84-
}
85-
return doMissingConstant(module, name, getSymbol(name));
86-
}
87-
88-
autoloadConstant.startAutoLoad();
89-
9070
if (getContext().getOptions().LOG_AUTOLOAD) {
91-
RubyLanguage.LOGGER.info(() -> String.format("%s: autoloading %s::%s with %s",
71+
RubyLanguage.LOGGER.info(() -> String.format("%s: autoloading %s with %s",
9272
getContext().fileLine(getContext().getCallStack().getTopMostUserSourceSection()),
93-
Layouts.MODULE.getFields(autoloadConstantModule).getName(), name, autoloadConstant.getAutoloadPath()));
73+
autoloadConstant, autoloadConstant.getAutoloadPath()));
9474
}
9575

76+
final Runnable require = () -> callRequireNode.call(coreLibrary().getMainObject(), "require", feature);
77+
return autoloadConstant(lexicalScope, module, name, autoloadConstant, lookupConstantNode, require);
78+
}
79+
80+
@TruffleBoundary
81+
public Object autoloadConstant(LexicalScope lexicalScope, DynamicObject module, String name, RubyConstant autoloadConstant, LookupConstantInterface lookupConstantNode, Runnable require) {
82+
final DynamicObject autoloadConstantModule = autoloadConstant.getDeclaringModule();
83+
final ModuleFields fields = Layouts.MODULE.getFields(autoloadConstantModule);
84+
85+
autoloadConstant.startAutoLoad();
9686
try {
9787

9888
// We need to notify cached lookup that we are autoloading the constant, as constant
9989
// lookup changes based on whether an autoload constant is loading or not (constant
10090
// lookup ignores being-autoloaded constants).
10191
fields.newConstantsVersion();
10292

103-
callRequireNode.call(coreLibrary().getMainObject(), "require", feature);
93+
require.run();
10494

10595
RubyConstant resolvedConstant = lookupConstantNode.lookupConstant(lexicalScope, module, name);
10696

@@ -122,7 +112,6 @@ protected Object autoloadConstant(LexicalScope lexicalScope, DynamicObject modul
122112
} finally {
123113
autoloadConstant.stopAutoLoad();
124114
}
125-
126115
}
127116

128117
@Specialization(

0 commit comments

Comments
 (0)