Skip to content

Commit 586dffa

Browse files
dafyddcrosbydafyddcrosby
committed
[Cookstyle][ShipIt] Open-source cookstyle cops
1 parent 2efb7e7 commit 586dffa

File tree

5 files changed

+450
-0
lines changed

5 files changed

+450
-0
lines changed

‎cookstyle/CompoundBooleanLiteralCleanup.rb‎

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
# Copyright (c) 2025-present, Meta Platforms, Inc. and affiliates
2+
# All rights reserved.
3+
#
4+
# Licensed under the Apache License, Version 2.0 (the "License");
5+
# you may not use this file except in compliance with the License.
6+
# You may obtain a copy of the License at
7+
#
8+
# http://www.apache.org/licenses/LICENSE-2.0
9+
#
10+
# Unless required by applicable law or agreed to in writing, software
11+
# distributed under the License is distributed on an "AS IS" BASIS,
12+
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
# See the License for the specific language governing permissions and
14+
# limitations under the License.
15+
require 'rubocop'
16+
17+
# TODO(dcrosby) This will eventually be upstreamed as a fix to the
18+
# boolean literal cops in RuboCop, but for now this addresses some
19+
# cleanup that other Cookstyle cops do (which is why this is a Cookstyle
20+
# cop, and not in our RuboCop-only config).
21+
22+
module RuboCop::Cop::Chef::Meta
23+
# This lint removes boolean literals (true/false keywords) where we can guarantee it:
24+
# A) does *not* change the logic flow
25+
# B) does *not* change the returning value
26+
# C) does *not* change any side effects
27+
#
28+
# This removes booleans that are on the left-hand side (LHS) of a logical operation
29+
# true && foo => foo
30+
# false || foo => foo
31+
#
32+
# We do not auto-correct RHS booleans, since this can create side-effects.
33+
# An example:
34+
# if File.write('test', 'sideeffect') && false
35+
# puts 'never runs because of `&& false`'
36+
# end
37+
# File.read('test') # Runtime failure if 'test' file isn't written!
38+
#
39+
# TODO: Handle semantic (and/or) operators? Currently avoiding them to avoid precedence issues.
40+
class CompoundBooleanLiteralCleanup < Base
41+
include RangeHelp
42+
extend AutoCorrector
43+
MSG = fb_msg('Boolean literals with logic operators should be cleaned up')
44+
45+
def node_is_lhs_operand?(node)
46+
if (node.parent.and_type? || node.parent.or_type?) &&
47+
node.sibling_index == 0 &&
48+
node.parent.logical_operator?
49+
node.parent.type
50+
end
51+
end
52+
53+
def on_true(node)
54+
if node.parent?
55+
if (op = node_is_lhs_operand?(node))
56+
if op == :and
57+
# redundant boolean
58+
remove_lhs(node)
59+
else
60+
# short circuit
61+
remove_rhs(node)
62+
end
63+
end
64+
end
65+
end
66+
67+
def on_false(node)
68+
if node.parent?
69+
if (op = node_is_lhs_operand?(node))
70+
if op == :and
71+
# short circuit
72+
remove_rhs(node)
73+
else
74+
# redundant boolean
75+
remove_lhs(node)
76+
end
77+
end
78+
end
79+
end
80+
81+
def remove_rhs(node)
82+
add_offense(node,
83+
:severity => :refactor) do |corrector|
84+
corrector.replace(
85+
node.parent,
86+
node.parent.lhs.source,
87+
)
88+
end
89+
end
90+
91+
def remove_lhs(node)
92+
add_offense(node,
93+
:severity => :refactor) do |corrector|
94+
corrector.replace(
95+
node.parent,
96+
node.parent.rhs.source,
97+
)
98+
end
99+
end
100+
end
101+
end

‎cookstyle/ConditionalBooleanLiteralCleanup.rb‎

Lines changed: 226 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,226 @@
1+
# Copyright (c) 2025-present, Meta Platforms, Inc. and affiliates
2+
# All rights reserved.
3+
#
4+
# Licensed under the Apache License, Version 2.0 (the "License");
5+
# you may not use this file except in compliance with the License.
6+
# You may obtain a copy of the License at
7+
#
8+
# http://www.apache.org/licenses/LICENSE-2.0
9+
#
10+
# Unless required by applicable law or agreed to in writing, software
11+
# distributed under the License is distributed on an "AS IS" BASIS,
12+
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
# See the License for the specific language governing permissions and
14+
# limitations under the License.
15+
require 'rubocop'
16+
17+
# TODO(dcrosby) This will eventually be upstreamed as a fix to the
18+
# boolean literal cops in RuboCop, but for now this addresses some
19+
# cleanup that other Cookstyle cops do (which is why this is a Cookstyle
20+
# cop, and not in our RuboCop-only config).
21+
22+
# A caveat with this linter is that it cannot determine `if false`
23+
# because some deprecated thing was converted to `false`, versus you
24+
# wanting to disable something temporarily with `if false`. It's like in
25+
# addition to nil and false we need some other way of saying "false,
26+
# because I'll forget about this reverted diff in 2 months" ;-)
27+
# In the event you really want to keep that boolean literal condition
28+
# add a comment with `rubocop:disable Chef/Meta/ConditionalBooleanLiteralCleanup`
29+
30+
module RuboCop::Cop::Chef::Meta
31+
class ConditionalBooleanLiteralCleanup < Base
32+
include RangeHelp
33+
extend AutoCorrector
34+
MSG = fb_msg('Boolean literals as conditionals should be cleaned up')
35+
36+
# This class only fires on true/false boolean literals, as firing on
37+
# every single if-type node would be expensive. However, you'll
38+
# quickly notice most operations are on the `if` node of the AST,
39+
# hence the `ifnode` variable.
40+
#
41+
# To keep the logic readable, we'll be using the `if` helper methods
42+
# from rubocop-ast wherever possible
43+
# https://github.com/rubocop/rubocop-ast/blob/master/lib/rubocop/ast/node/if_node.rb
44+
45+
def on_true(node)
46+
return unless node&.parent&.if_type?
47+
return unless node.sibling_index == 0 # Checks `true` is the conditional, not the value
48+
49+
ifnode = node.parent
50+
51+
add_offense(node,
52+
:severity => :refactor) do |corrector|
53+
if ifnode.elsif?
54+
replace_elsif_true(corrector, ifnode)
55+
elsif ifnode.unless?
56+
if ifnode.branches.count == 1
57+
replace_unless_true(corrector, ifnode)
58+
else
59+
replace_unless_true_else(corrector, ifnode)
60+
end
61+
else
62+
replace_if_true(corrector, ifnode)
63+
end
64+
end
65+
end
66+
67+
def on_false(node)
68+
return unless node&.parent&.if_type?
69+
return unless node.sibling_index == 0 # Checks `false` is the conditional, not the value
70+
71+
ifnode = node.parent
72+
73+
add_offense(node,
74+
:severity => :refactor) do |corrector|
75+
if ifnode.if? || ifnode.ternary?
76+
# The false is in the if branch
77+
if ifnode.branches.count == 1
78+
replace_if_false(corrector, ifnode)
79+
elsif ifnode.elsif_conditional?
80+
replace_if_false_elsif(corrector, ifnode)
81+
else
82+
replace_if_false_else(corrector, ifnode)
83+
end
84+
elsif ifnode.unless?
85+
replace_unless_false(corrector, ifnode)
86+
elsif ifnode.branches.count == 1
87+
# The false is in the elsif branch
88+
replace_elsif_false(corrector, ifnode)
89+
elsif ifnode.elsif_conditional?
90+
replace_elsif_false_elsif(corrector, ifnode)
91+
else
92+
replace_elsif_false_else(corrector, ifnode)
93+
end
94+
end
95+
end
96+
97+
private
98+
99+
# if foo
100+
# untouched_case
101+
# elsif true <- fires here
102+
# contents
103+
# else
104+
# will_be_removed
105+
# end
106+
def replace_elsif_true(corrector, ifnode)
107+
str = "else\n"
108+
str << ' ' * ifnode.if_branch.loc.column
109+
str << ifnode.if_branch.source
110+
corrector.replace(ifnode, str)
111+
end
112+
113+
# # if/else
114+
# if true <- fires here
115+
# contents
116+
# else
117+
# will_be_removed
118+
# end
119+
#
120+
# # if/elsif/else
121+
# if true <- fires here
122+
# contents
123+
# elsif whatever
124+
# will_be_removed
125+
# else
126+
# will_be_removed
127+
# end
128+
#
129+
# # ternary
130+
# foo = true ? contents : will_be_removed
131+
def replace_if_true(corrector, ifnode)
132+
corrector.replace(ifnode, ifnode.if_branch.source)
133+
end
134+
alias replace_unless_false replace_if_true
135+
136+
# if false <-- you are here
137+
# will_be_removed
138+
# end
139+
def replace_if_false(corrector, ifnode)
140+
corrector.replace(ifnode, '')
141+
end
142+
alias replace_unless_true replace_if_false
143+
144+
# if foo
145+
# untouched_case
146+
# elsif false <-- you are here
147+
# will_be_removed
148+
# end
149+
def replace_elsif_false(corrector, ifnode)
150+
range = range_between(
151+
ifnode.parent.else_branch.loc.expression.begin_pos,
152+
ifnode.parent.loc.end.begin_pos,
153+
)
154+
corrector.replace(range, '')
155+
end
156+
157+
# foo = false ? will_be_removed : contents
158+
#
159+
# if false
160+
# will_be_removed
161+
# else
162+
# untouched_case
163+
# end
164+
def replace_if_false_else(corrector, ifnode)
165+
corrector.replace(ifnode, ifnode.else_branch.source)
166+
end
167+
alias replace_unless_true_else replace_if_false_else
168+
169+
# if false
170+
# puts "this should go"
171+
# elsif var
172+
# puts "this should be the new if"
173+
# elsif bar
174+
# puts "this should remain"
175+
# else
176+
# puts "this should remain, too"
177+
# end
178+
#
179+
# if false
180+
# puts "this should go"
181+
# elsif var
182+
# puts "this should be the new if"
183+
# else
184+
# puts "this should remain, too"
185+
# end
186+
def replace_if_false_elsif(corrector, ifnode)
187+
range = range_between(
188+
ifnode.condition.loc.expression.begin_pos,
189+
ifnode.else_branch.condition.loc.expression.begin_pos,
190+
)
191+
corrector.replace(range, '')
192+
end
193+
194+
# if foo
195+
# puts "this should stay"
196+
# elsif false
197+
# puts "this should go"
198+
# else
199+
# puts "this should remain"
200+
# end
201+
def replace_elsif_false_else(corrector, ifnode)
202+
range = range_between(
203+
ifnode.loc.expression.begin_pos,
204+
ifnode.loc.else.begin_pos,
205+
)
206+
corrector.replace(range, '')
207+
end
208+
209+
# if foo
210+
# puts "this should stay"
211+
# elsif false
212+
# puts "this should go"
213+
# elsif var
214+
# puts "this should remain"
215+
# else
216+
# puts "this should remain, too"
217+
# end
218+
def replace_elsif_false_elsif(corrector, ifnode)
219+
range = range_between(
220+
ifnode.loc.expression.begin_pos,
221+
ifnode.else_branch.loc.expression.begin_pos,
222+
)
223+
corrector.replace(range, '')
224+
end
225+
end
226+
end

‎cookstyle/InvertedBooleanLiteralCleanup.rb‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
# Copyright (c) 2025-present, Meta Platforms, Inc. and affiliates
2+
# All rights reserved.
3+
#
4+
# Licensed under the Apache License, Version 2.0 (the "License");
5+
# you may not use this file except in compliance with the License.
6+
# You may obtain a copy of the License at
7+
#
8+
# http://www.apache.org/licenses/LICENSE-2.0
9+
#
10+
# Unless required by applicable law or agreed to in writing, software
11+
# distributed under the License is distributed on an "AS IS" BASIS,
12+
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
# See the License for the specific language governing permissions and
14+
# limitations under the License.
15+
16+
require 'rubocop'
17+
18+
# TODO(dcrosby) This will eventually be upstreamed as a fix to the
19+
# boolean literal cops in RuboCop, but for now this addresses some
20+
# cleanup that other Cookstyle cops do (which is why this is a Cookstyle
21+
# cop, and not in our RuboCop-only config).
22+
23+
module RuboCop::Cop::Chef::Meta
24+
class InvertedBooleanLiteralCleanup < Base
25+
include RangeHelp
26+
extend AutoCorrector
27+
MSG = fb_msg('Inverted boolean literals should be cleaned up')
28+
29+
def_node_matcher :inverted_boolean_literal?, <<-PATTERN
30+
(send (${true false}) :!)
31+
PATTERN
32+
33+
RESTRICT_ON_SEND = [:!].freeze
34+
def on_send(node)
35+
expression = inverted_boolean_literal?(node)
36+
return unless expression
37+
add_offense(node,
38+
:severity => :refactor) do |corrector|
39+
corrector.replace(
40+
# Here we flip !true->false and !false->true
41+
node,
42+
# rubocop:disable Lint/BooleanSymbol
43+
(expression == :false).to_s,
44+
# rubocop:enable Lint/BooleanSymbol
45+
)
46+
end
47+
end
48+
end
49+
end

0 commit comments

Comments
 (0)