Skip to content

Commit 9c542a2

Browse files
andrykonchinandrykonchin
committed
Fix assignment operators with fully qualified constants and evaluate parent only once
1 parent 49b1843 commit 9c542a2

File tree

11 files changed

+565
-85
lines changed

11 files changed

+565
-85
lines changed

‎spec/ruby/language/assignments_spec.rb

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
require_relative '../spec_helper'
2+
3+
# Should be synchronized with spec/ruby/language/optional_assignments_spec.rb
4+
describe 'Assignments' do
5+
describe 'using +=' do
6+
describe 'using an accessor' do
7+
before do
8+
klass = Class.new { attr_accessor :b }
9+
@a = klass.new
10+
end
11+
12+
it 'does evaluate receiver only once when assigns' do
13+
ScratchPad.record []
14+
@a.b = 1
15+
16+
(ScratchPad << :evaluated; @a).b += 2
17+
18+
ScratchPad.recorded.should == [:evaluated]
19+
@a.b.should == 3
20+
end
21+
end
22+
23+
describe 'using a #[]' do
24+
it 'evaluates receiver only once when assigns' do
25+
ScratchPad.record []
26+
a = {k: 1}
27+
28+
(ScratchPad << :evaluated; a)[:k] += 2
29+
30+
ScratchPad.recorded.should == [:evaluated]
31+
a[:k].should == 3
32+
end
33+
end
34+
35+
describe 'using compounded constants' do
36+
it 'causes side-effects of the module part to be applied only once (when assigns)' do
37+
module ConstantSpecs
38+
OpAssignTrue = 1
39+
end
40+
41+
suppress_warning do # already initialized constant
42+
x = 0
43+
(x += 1; ConstantSpecs)::OpAssignTrue += 2
44+
x.should == 1
45+
ConstantSpecs::OpAssignTrue.should == 3
46+
end
47+
48+
ConstantSpecs.send :remove_const, :OpAssignTrue
49+
end
50+
end
51+
end
52+
end

‎spec/ruby/language/optional_assignments_spec.rb

Lines changed: 129 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@
5757
end
5858
end
5959

60-
describe 'using a accessor' do
60+
describe 'using an accessor' do
6161
before do
6262
klass = Class.new { attr_accessor :b }
6363
@a = klass.new
@@ -103,6 +103,16 @@
103103
@a.b.should == 10
104104
end
105105

106+
it 'does evaluate receiver only once when assigns' do
107+
ScratchPad.record []
108+
@a.b = nil
109+
110+
(ScratchPad << :evaluated; @a).b ||= 10
111+
112+
ScratchPad.recorded.should == [:evaluated]
113+
@a.b.should == 10
114+
end
115+
106116
it 'returns the new value if set to false' do
107117
def @a.b=(x)
108118
:v
@@ -144,7 +154,77 @@ def b=(x)
144154

145155
klass.new.t
146156
end
157+
end
158+
159+
describe 'using a #[]' do
160+
before do
161+
@a = {}
162+
klass = Class.new do
163+
def [](k)
164+
@hash ||= {}
165+
@hash[k]
166+
end
167+
168+
def []=(k, v)
169+
@hash ||= {}
170+
@hash[k] = v
171+
7
172+
end
173+
end
174+
@b = klass.new
175+
end
176+
177+
it 'returns the assigned value, not the result of the []= method with ||=' do
178+
(@b[:k] ||= 12).should == 12
179+
end
180+
181+
it 'correctly handles a splatted argument for the index' do
182+
(@b[*[:k]] ||= 12).should == 12
183+
end
184+
185+
it "evaluates the index precisely once" do
186+
ary = [:x, :y]
187+
@a[:x] = 15
188+
@a[ary.pop] ||= 25
189+
ary.should == [:x]
190+
@a.should == { x: 15, y: 25 }
191+
end
192+
193+
it "evaluates the index arguments in the correct order" do
194+
ary = Class.new(Array) do
195+
def [](x, y)
196+
super(x + 3 * y)
197+
end
147198

199+
def []=(x, y, value)
200+
super(x + 3 * y, value)
201+
end
202+
end.new
203+
ary[0, 0] = 1
204+
ary[1, 0] = 1
205+
ary[2, 0] = nil
206+
ary[3, 0] = 1
207+
ary[4, 0] = 1
208+
ary[5, 0] = 1
209+
ary[6, 0] = nil
210+
211+
foo = [0, 2]
212+
213+
ary[foo.pop, foo.pop] ||= 2 # expected `ary[2, 0] ||= 2`
214+
215+
ary[2, 0].should == 2
216+
ary[6, 0].should == nil # returns the same element as `ary[0, 2]`
217+
end
218+
219+
it 'evaluates receiver only once when assigns' do
220+
ScratchPad.record []
221+
@a[:k] = nil
222+
223+
(ScratchPad << :evaluated; @a)[:k] ||= 2
224+
225+
ScratchPad.recorded.should == [:evaluated]
226+
@a[:k].should == 2
227+
end
148228
end
149229
end
150230

@@ -191,7 +271,7 @@ def b=(x)
191271
end
192272
end
193273

194-
describe 'using a single variable' do
274+
describe 'using an accessor' do
195275
before do
196276
klass = Class.new { attr_accessor :b }
197277
@a = klass.new
@@ -236,6 +316,16 @@ def b=(x)
236316

237317
@a.b.should == 20
238318
end
319+
320+
it 'does evaluate receiver only once when assigns' do
321+
ScratchPad.record []
322+
@a.b = 10
323+
324+
(ScratchPad << :evaluated; @a).b &&= 20
325+
326+
ScratchPad.recorded.should == [:evaluated]
327+
@a.b.should == 20
328+
end
239329
end
240330

241331
describe 'using a #[]' do
@@ -297,17 +387,21 @@ def []=(k, v)
297387
end
298388

299389
it 'returns the assigned value, not the result of the []= method with ||=' do
300-
(@b[:k] ||= 12).should == 12
390+
@b[:k] = 10
391+
(@b[:k] &&= 12).should == 12
301392
end
302393

303394
it 'correctly handles a splatted argument for the index' do
304-
(@b[*[:k]] ||= 12).should == 12
395+
@b[:k] = 10
396+
(@b[*[:k]] &&= 12).should == 12
397+
@b[:k].should == 12
305398
end
306399

307400
it "evaluates the index precisely once" do
308401
ary = [:x, :y]
309402
@a[:x] = 15
310-
@a[ary.pop] ||= 25
403+
@a[:y] = 20
404+
@a[ary.pop] &&= 25
311405
ary.should == [:x]
312406
@a.should == { x: 15, y: 25 }
313407
end
@@ -324,18 +418,28 @@ def []=(x, y, value)
324418
end.new
325419
ary[0, 0] = 1
326420
ary[1, 0] = 1
327-
ary[2, 0] = nil
421+
ary[2, 0] = 1
328422
ary[3, 0] = 1
329423
ary[4, 0] = 1
330424
ary[5, 0] = 1
331-
ary[6, 0] = nil
425+
ary[6, 0] = 1
332426

333427
foo = [0, 2]
334428

335-
ary[foo.pop, foo.pop] ||= 2
429+
ary[foo.pop, foo.pop] &&= 2 # expected `ary[2, 0] &&= 2`
336430

337431
ary[2, 0].should == 2
338-
ary[6, 0].should == nil
432+
ary[6, 0].should == 1 # returns the same element as `ary[0, 2]`
433+
end
434+
435+
it 'evaluates receiver only once when assigns' do
436+
ScratchPad.record []
437+
@a[:k] = 1
438+
439+
(ScratchPad << :evaluated; @a)[:k] &&= 2
440+
441+
ScratchPad.recorded.should == [:evaluated]
442+
@a[:k].should == 2
339443
end
340444

341445
it 'returns the assigned value, not the result of the []= method with +=' do
@@ -434,7 +538,7 @@ module ConstantSpecs
434538
ConstantSpecs::ClassA::OR_ASSIGNED_CONSTANT2.should == :assigned
435539
end
436540

437-
it 'causes side-effects of the module part to be applied (for nil constant)' do
541+
it 'causes side-effects of the module part to be applied only once (for nil constant)' do
438542
suppress_warning do # already initialized constant
439543
ConstantSpecs::ClassA::NIL_OR_ASSIGNED_CONSTANT2 = nil
440544
x = 0
@@ -492,5 +596,20 @@ module ConstantSpecs
492596
ConstantSpecs::OpAssignFalse.should == false
493597
ConstantSpecs.send :remove_const, :OpAssignFalse
494598
end
599+
600+
it 'causes side-effects of the module part to be applied only once (when assigns)' do
601+
module ConstantSpecs
602+
OpAssignTrue = true
603+
end
604+
605+
suppress_warning do # already initialized constant
606+
x = 0
607+
(x += 1; ConstantSpecs)::OpAssignTrue &&= :assigned
608+
x.should == 1
609+
ConstantSpecs::OpAssignTrue.should == :assigned
610+
end
611+
612+
ConstantSpecs.send :remove_const, :OpAssignTrue
613+
end
495614
end
496615
end

‎spec/tags/language/assignments_tags.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
fails:Assignments using += using compounded constants causes side-effects of the module part to be applied only once (when assigns)

‎spec/tags/language/optional_assignments_tags.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
fails:Optional constant assignment with &&= causes side-effects of the module part to be applied only once (when assigns)

‎spec/truffle/parsing/fixtures/operators/&&=/variable_assignments/constant_fully_qualified.yaml

Lines changed: 47 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,43 +1,73 @@
11
subject: "&&="
2-
description: "Variable assignment/fully qualified constant (::A &&= b)"
2+
description: "Variable assignment/fully qualified constant (A::B &&= c)"
33
notes: >
4-
`::A &&= b` is translated into `::A && ::A = b`
4+
`A::B &&= c` is translated into `A::B && A::B = c`
5+
yarp_specific: true # evaluate parent (FOO::) only once and cache its value into a local variable
56
focused_on_node: "org.truffleruby.language.defined.DefinedWrapperNode"
67
ruby: |
7-
::FOO &&= 42
8+
FOO::BAR &&= 42
89
ast: |
910
DefinedWrapperNode
1011
attributes:
1112
definition = assignment
1213
flags = 1
1314
children:
1415
child =
15-
AndNodeGen
16+
SequenceNode
1617
attributes:
1718
flags = 0
1819
children:
19-
left =
20-
ReadConstantNode
20+
body = [
21+
WriteLocalVariableNode
2122
attributes:
2223
flags = 0
23-
name = "FOO"
24+
frameSlot = 2 # %value_0
2425
children:
25-
moduleNode =
26-
ObjectClassLiteralNode
26+
valueNode =
27+
ReadConstantWithLexicalScopeNode
2728
attributes:
2829
flags = 0
29-
right =
30-
WriteConstantNode
30+
lexicalScope = :: Object
31+
name = "FOO"
32+
children:
33+
getConstantNode =
34+
GetConstantNodeGen
35+
lookupConstantNode =
36+
LookupConstantWithLexicalScopeNodeGen
37+
attributes:
38+
lexicalScope = :: Object
39+
name = "FOO"
40+
AndNodeGen
3141
attributes:
3242
flags = 0
33-
name = "FOO"
3443
children:
35-
moduleNode =
36-
ObjectClassLiteralNode
44+
left =
45+
ReadConstantNode
3746
attributes:
3847
flags = 0
39-
valueNode =
40-
IntegerFixnumLiteralNode
48+
name = "BAR"
49+
children:
50+
moduleNode =
51+
ReadLocalVariableNode
52+
attributes:
53+
flags = 0
54+
frameSlot = 2 # %value_0
55+
type = FRAME_LOCAL
56+
right =
57+
WriteConstantNode
4158
attributes:
4259
flags = 0
43-
value = 42
60+
name = "BAR"
61+
children:
62+
moduleNode =
63+
ReadLocalVariableNode
64+
attributes:
65+
flags = 0
66+
frameSlot = 2 # %value_0
67+
type = FRAME_LOCAL
68+
valueNode =
69+
IntegerFixnumLiteralNode
70+
attributes:
71+
flags = 0
72+
value = 42
73+
]

0 commit comments

Comments
 (0)