Skip to content

Commit 8bee2cd

Browse files
pugnascotiapugnascotia
committed
Fix shadowed vars pt7 (elastic#80996)
Part of elastic#19752. Fix more instances where local variable names were shadowing field names. Also modify our fork of HiddenFieldCheck to add the ignoreConstructorBody and ignoredMethodNames parameters, so that the check can ignore more matches.
1 parent deb0d4c commit 8bee2cd

File tree

34 files changed

+219
-144
lines changed

34 files changed

+219
-144
lines changed

‎build-conventions/src/main/java/org/elasticsearch/gradle/internal/checkstyle/HiddenFieldCheck.java

Lines changed: 60 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -71,9 +71,15 @@ public class HiddenFieldCheck extends AbstractCheck {
7171
/** Control whether to ignore constructor parameters. */
7272
private boolean ignoreConstructorParameter;
7373

74+
/** Control whether to ignore variables in constructor bodies. */
75+
private boolean ignoreConstructorBody;
76+
7477
/** Control whether to ignore parameters of abstract methods. */
7578
private boolean ignoreAbstractMethods;
7679

80+
/** If set, specifies a regex of method names that should be ignored */
81+
private String ignoredMethodNames;
82+
7783
@Override
7884
public int[] getDefaultTokens() {
7985
return getAcceptableTokens();
@@ -224,7 +230,8 @@ private void processVariable(DetailAST ast) {
224230

225231
if ((frame.containsStaticField(name) || isInstanceField(ast, name))
226232
&& isMatchingRegexp(name) == false
227-
&& isIgnoredParam(ast, name) == false) {
233+
&& isIgnoredParam(ast, name) == false
234+
&& isIgnoredVariable(ast, name) == false) {
228235
log(nameAST, MSG_KEY, name);
229236
}
230237
}
@@ -238,7 +245,14 @@ && isIgnoredParam(ast, name) == false) {
238245
* @return true if parameter is ignored.
239246
*/
240247
private boolean isIgnoredParam(DetailAST ast, String name) {
241-
return isIgnoredSetterParam(ast, name) || isIgnoredConstructorParam(ast) || isIgnoredParamOfAbstractMethod(ast);
248+
return isVariableInIgnoredMethod(ast, name)
249+
|| isIgnoredSetterParam(ast, name)
250+
|| isIgnoredConstructorParam(ast)
251+
|| isIgnoredParamOfAbstractMethod(ast);
252+
}
253+
254+
private boolean isIgnoredVariable(DetailAST ast, String name) {
255+
return isIgnoredVariableInConstructorBody(ast, name);
242256
}
243257

244258
/**
@@ -410,6 +424,42 @@ private boolean isIgnoredParamOfAbstractMethod(DetailAST ast) {
410424
return result;
411425
}
412426

427+
/**
428+
* Decides whether to ignore an AST node that is the parameter of a method whose
429+
* name matches the {@link #ignoredMethodNames} regex, if set.
430+
* @param ast the AST to check
431+
* @return true is the ast should be ignored because the parameter belongs to a
432+
* method whose name matches the regex.
433+
*/
434+
private boolean isVariableInIgnoredMethod(DetailAST ast, String name) {
435+
boolean result = false;
436+
if (ignoredMethodNames != null && (ast.getType() == TokenTypes.PARAMETER_DEF || ast.getType() == TokenTypes.VARIABLE_DEF)) {
437+
DetailAST method = ast.getParent();
438+
while (method != null && method.getType() != TokenTypes.METHOD_DEF) {
439+
method = method.getParent();
440+
}
441+
if (method != null && method.getType() == TokenTypes.METHOD_DEF) {
442+
final String methodName = method.findFirstToken(TokenTypes.IDENT).getText();
443+
result = methodName.matches(ignoredMethodNames);
444+
}
445+
}
446+
return result;
447+
}
448+
449+
private boolean isIgnoredVariableInConstructorBody(DetailAST ast, String name) {
450+
boolean result = false;
451+
452+
if (ignoreConstructorBody && ast.getType() == TokenTypes.VARIABLE_DEF) {
453+
DetailAST method = ast.getParent();
454+
while (method != null && method.getType() != TokenTypes.CTOR_DEF) {
455+
method = method.getParent();
456+
}
457+
result = method != null && method.getType() == TokenTypes.CTOR_DEF;
458+
}
459+
460+
return result;
461+
}
462+
413463
/**
414464
* Setter to define the RegExp for names of variables and parameters to ignore.
415465
*
@@ -463,6 +513,14 @@ public void setIgnoreAbstractMethods(boolean ignoreAbstractMethods) {
463513
this.ignoreAbstractMethods = ignoreAbstractMethods;
464514
}
465515

516+
public void setIgnoredMethodNames(String ignoredMethodNames) {
517+
this.ignoredMethodNames = ignoredMethodNames;
518+
}
519+
520+
public void setIgnoreConstructorBody(boolean ignoreConstructorBody) {
521+
this.ignoreConstructorBody = ignoreConstructorBody;
522+
}
523+
466524
/**
467525
* Holds the names of static and instance fields of a type.
468526
*/

‎build-tools-internal/src/main/resources/checkstyle.xml

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -101,9 +101,12 @@
101101
<!--
102102
<module name="org.elasticsearch.gradle.internal.checkstyle.HiddenFieldCheck">
103103
<property name="ignoreConstructorParameter" value="true" />
104+
<property name="ignoreConstructorBody" value="true"/>
104105
<property name="ignoreSetter" value="true" />
105106
<property name="setterCanReturnItsClass" value="true"/>
106-
<property name="ignoreFormat" value="^(threadPool)$"/>
107+
<property name="ignoreFormat" value="^(?:threadPool)$"/>
108+
<property name="ignoreAbstractMethods" value="true"/>
109+
<property name="ignoredMethodNames" value="^(?:createParser)$"/>
107110
<message key="hidden.field" value="''{0}'' hides a field." />
108111
</module>
109112
-->

‎x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/action/ShardFollowNodeTaskTests.java

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1421,8 +1421,8 @@ protected void innerSendShardChangesRequest(
14211421
@Override
14221422
protected Scheduler.Cancellable scheduleBackgroundRetentionLeaseRenewal(final LongSupplier followerGlobalCheckpoint) {
14231423
if (scheduleRetentionLeaseRenewal.get()) {
1424-
final ScheduledThreadPoolExecutor scheduler = Scheduler.initScheduler(Settings.EMPTY, "test-scheduler");
1425-
final ScheduledFuture<?> future = scheduler.scheduleWithFixedDelay(
1424+
final ScheduledThreadPoolExecutor testScheduler = Scheduler.initScheduler(Settings.EMPTY, "test-scheduler");
1425+
final ScheduledFuture<?> future = testScheduler.scheduleWithFixedDelay(
14261426
() -> retentionLeaseRenewal.accept(followerGlobalCheckpoint.getAsLong()),
14271427
0,
14281428
TimeValue.timeValueMillis(200).millis(),
@@ -1433,7 +1433,7 @@ protected Scheduler.Cancellable scheduleBackgroundRetentionLeaseRenewal(final Lo
14331433
@Override
14341434
public boolean cancel() {
14351435
final boolean cancel = future.cancel(true);
1436-
scheduler.shutdown();
1436+
testScheduler.shutdown();
14371437
return cancel;
14381438
}
14391439

‎x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/index/engine/FollowingEngineTests.java

Lines changed: 27 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -112,8 +112,8 @@ public void testFollowingEngineRejectsNonFollowingIndex() throws IOException {
112112

113113
public void testIndexSeqNoIsMaintained() throws IOException {
114114
final long seqNo = randomIntBetween(0, Integer.MAX_VALUE);
115-
runIndexTest(seqNo, Engine.Operation.Origin.PRIMARY, (followingEngine, index) -> {
116-
final Engine.IndexResult result = followingEngine.index(index);
115+
runIndexTest(seqNo, Engine.Operation.Origin.PRIMARY, (followingEngine, indexToTest) -> {
116+
final Engine.IndexResult result = followingEngine.index(indexToTest);
117117
assertThat(result.getSeqNo(), equalTo(seqNo));
118118
});
119119
}
@@ -159,8 +159,8 @@ public void runIndexTest(
159159
try (Store store = createStore(shardId, indexSettings, newDirectory())) {
160160
final EngineConfig engineConfig = engineConfig(shardId, indexSettings, threadPool, store);
161161
try (FollowingEngine followingEngine = createEngine(store, engineConfig)) {
162-
final Engine.Index index = indexForFollowing("id", seqNo, origin);
163-
consumer.accept(followingEngine, index);
162+
final Engine.Index indexToTest = indexForFollowing("id", seqNo, origin);
163+
consumer.accept(followingEngine, indexToTest);
164164
}
165165
}
166166
}
@@ -230,16 +230,21 @@ public void testDoNotFillSeqNoGaps() throws Exception {
230230
}
231231

232232
private EngineConfig engineConfig(
233-
final ShardId shardId,
233+
final ShardId shardIdValue,
234234
final IndexSettings indexSettings,
235235
final ThreadPool threadPool,
236236
final Store store
237237
) {
238238
final IndexWriterConfig indexWriterConfig = newIndexWriterConfig();
239239
final Path translogPath = createTempDir("translog");
240-
final TranslogConfig translogConfig = new TranslogConfig(shardId, translogPath, indexSettings, BigArrays.NON_RECYCLING_INSTANCE);
240+
final TranslogConfig translogConfig = new TranslogConfig(
241+
shardIdValue,
242+
translogPath,
243+
indexSettings,
244+
BigArrays.NON_RECYCLING_INSTANCE
245+
);
241246
return new EngineConfig(
242-
shardId,
247+
shardIdValue,
243248
threadPool,
244249
indexSettings,
245250
null,
@@ -341,26 +346,26 @@ private Engine.Delete deleteForPrimary(String id) {
341346
return new Engine.Delete(parsedDoc.type(), parsedDoc.id(), EngineTestCase.newUid(parsedDoc), primaryTerm.get());
342347
}
343348

344-
private Engine.Result applyOperation(Engine engine, Engine.Operation op, long primaryTerm, Engine.Operation.Origin origin)
349+
private Engine.Result applyOperation(Engine engine, Engine.Operation op, long primaryTermValue, Engine.Operation.Origin origin)
345350
throws IOException {
346351
final VersionType versionType = origin == Engine.Operation.Origin.PRIMARY ? VersionType.EXTERNAL : null;
347352
final Engine.Result result;
348353
if (op instanceof Engine.Index) {
349-
Engine.Index index = (Engine.Index) op;
354+
Engine.Index engineIndex = (Engine.Index) op;
350355
result = engine.index(
351356
new Engine.Index(
352-
index.uid(),
353-
index.parsedDoc(),
354-
index.seqNo(),
355-
primaryTerm,
356-
index.version(),
357+
engineIndex.uid(),
358+
engineIndex.parsedDoc(),
359+
engineIndex.seqNo(),
360+
primaryTermValue,
361+
engineIndex.version(),
357362
versionType,
358363
origin,
359-
index.startTime(),
360-
index.getAutoGeneratedIdTimestamp(),
361-
index.isRetry(),
362-
index.getIfSeqNo(),
363-
index.getIfPrimaryTerm()
364+
engineIndex.startTime(),
365+
engineIndex.getAutoGeneratedIdTimestamp(),
366+
engineIndex.isRetry(),
367+
engineIndex.getIfSeqNo(),
368+
engineIndex.getIfPrimaryTerm()
364369
)
365370
);
366371
} else if (op instanceof Engine.Delete) {
@@ -371,7 +376,7 @@ private Engine.Result applyOperation(Engine engine, Engine.Operation op, long pr
371376
delete.id(),
372377
delete.uid(),
373378
delete.seqNo(),
374-
primaryTerm,
379+
primaryTermValue,
375380
delete.version(),
376381
versionType,
377382
origin,
@@ -382,7 +387,7 @@ private Engine.Result applyOperation(Engine engine, Engine.Operation op, long pr
382387
);
383388
} else {
384389
Engine.NoOp noOp = (Engine.NoOp) op;
385-
result = engine.noOp(new Engine.NoOp(noOp.seqNo(), primaryTerm, origin, noOp.startTime(), noOp.reason()));
390+
result = engine.noOp(new Engine.NoOp(noOp.seqNo(), primaryTermValue, origin, noOp.startTime(), noOp.reason()));
386391
}
387392
return result;
388393
}
@@ -843,7 +848,7 @@ public void testProcessOnceOnPrimary() throws Exception {
843848
*/
844849
public void testVerifyShardBeforeIndexClosingIsNoOp() throws IOException {
845850
final long seqNo = randomIntBetween(0, Integer.MAX_VALUE);
846-
runIndexTest(seqNo, Engine.Operation.Origin.PRIMARY, (followingEngine, index) -> {
851+
runIndexTest(seqNo, Engine.Operation.Origin.PRIMARY, (followingEngine, indexToTest) -> {
847852
globalCheckpoint.set(randomNonNegativeLong());
848853
try {
849854
followingEngine.verifyEngineBeforeIndexClosing();

‎x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/monitoring/collector/ccr/AutoFollowStatsMonitoringDocTests.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ public void testToXContent() throws IOException {
8888
final NavigableMap<String, AutoFollowedCluster> trackingClusters = new TreeMap<>(
8989
Collections.singletonMap(randomAlphaOfLength(4), new AutoFollowedCluster(1L, 1L))
9090
);
91-
final AutoFollowStats autoFollowStats = new AutoFollowStats(
91+
autoFollowStats = new AutoFollowStats(
9292
randomNonNegativeLong(),
9393
randomNonNegativeLong(),
9494
randomNonNegativeLong(),

‎x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/monitoring/collector/ccr/FollowStatsMonitoringDocTests.java

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -117,7 +117,7 @@ public void testToXContent() throws IOException {
117117
)
118118
);
119119
final long timeSinceLastReadMillis = randomNonNegativeLong();
120-
final ShardFollowNodeTaskStatus status = new ShardFollowNodeTaskStatus(
120+
final ShardFollowNodeTaskStatus taskStatus = new ShardFollowNodeTaskStatus(
121121
"leader_cluster",
122122
"leader_index",
123123
"follower_index",
@@ -148,7 +148,7 @@ public void testToXContent() throws IOException {
148148
timeSinceLastReadMillis,
149149
new ElasticsearchException("fatal error")
150150
);
151-
final FollowStatsMonitoringDoc document = new FollowStatsMonitoringDoc("_cluster", timestamp, intervalMillis, node, status);
151+
final FollowStatsMonitoringDoc document = new FollowStatsMonitoringDoc("_cluster", timestamp, intervalMillis, node, taskStatus);
152152
final BytesReference xContent = XContentHelper.toXContent(document, XContentType.JSON, false);
153153
assertThat(
154154
xContent.utf8ToString(),
@@ -273,7 +273,7 @@ public void testShardFollowNodeTaskStatusFieldsMapped() throws IOException {
273273
final NavigableMap<Long, Tuple<Integer, ElasticsearchException>> fetchExceptions = new TreeMap<>(
274274
Collections.singletonMap(1L, Tuple.tuple(2, new ElasticsearchException("shard is sad")))
275275
);
276-
final ShardFollowNodeTaskStatus status = new ShardFollowNodeTaskStatus(
276+
final ShardFollowNodeTaskStatus taskStatus = new ShardFollowNodeTaskStatus(
277277
"remote_cluster",
278278
"leader_index",
279279
"follower_index",
@@ -305,7 +305,7 @@ public void testShardFollowNodeTaskStatusFieldsMapped() throws IOException {
305305
new ElasticsearchException("fatal error")
306306
);
307307
XContentBuilder builder = jsonBuilder();
308-
builder.value(status);
308+
builder.value(taskStatus);
309309
Map<String, Object> serializedStatus = XContentHelper.convertToMap(XContentType.JSON.xContent(), Strings.toString(builder), false);
310310

311311
Map<String, Object> template = XContentHelper.convertToMap(

‎x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/rollup/action/GetRollupIndexCapsAction.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@ public String[] indices() {
7474
}
7575

7676
@Override
77-
public IndicesRequest indices(String... indices) {
77+
public IndicesRequest indices(@SuppressWarnings("HiddenField") String... indices) {
7878
Objects.requireNonNull(indices, "indices must not be null");
7979
for (String index : indices) {
8080
Objects.requireNonNull(index, "index must not be null");

‎x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/scheduler/Cron.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -795,7 +795,7 @@ public static void validate(String expression) throws IllegalArgumentException {
795795
//
796796
////////////////////////////////////////////////////////////////////////////
797797

798-
private void buildExpression(String expression) {
798+
private void buildExpression(@SuppressWarnings("HiddenField") String expression) {
799799

800800
try {
801801

‎x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/search/action/AsyncSearchResponse.java

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -100,8 +100,8 @@ public void writeTo(StreamOutput out) throws IOException {
100100
out.writeLong(expirationTimeMillis);
101101
}
102102

103-
public AsyncSearchResponse clone(String id) {
104-
return new AsyncSearchResponse(id, searchResponse, error, isPartial, false, startTimeMillis, expirationTimeMillis);
103+
public AsyncSearchResponse clone(String searchId) {
104+
return new AsyncSearchResponse(searchId, searchResponse, error, isPartial, false, startTimeMillis, expirationTimeMillis);
105105
}
106106

107107
/**
@@ -165,8 +165,8 @@ public long getExpirationTime() {
165165
}
166166

167167
@Override
168-
public AsyncSearchResponse withExpirationTime(long expirationTimeMillis) {
169-
return new AsyncSearchResponse(id, searchResponse, error, isPartial, isRunning, startTimeMillis, expirationTimeMillis);
168+
public AsyncSearchResponse withExpirationTime(long expirationTime) {
169+
return new AsyncSearchResponse(id, searchResponse, error, isPartial, isRunning, startTimeMillis, expirationTime);
170170
}
171171

172172
@Override

‎x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/InvalidateApiKeyRequest.java

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -272,14 +272,16 @@ public int hashCode() {
272272
return Objects.hash(realmName, userName, ids, name, ownedByAuthenticatedUser);
273273
}
274274

275-
private void validateIds(@Nullable String[] ids) {
276-
if (ids != null) {
277-
if (ids.length == 0) {
275+
private void validateIds(@Nullable String[] idsToValidate) {
276+
if (idsToValidate != null) {
277+
if (idsToValidate.length == 0) {
278278
final ActionRequestValidationException validationException = new ActionRequestValidationException();
279279
validationException.addValidationError("Field [ids] cannot be an empty array");
280280
throw validationException;
281281
} else {
282-
final int[] idxOfBlankIds = IntStream.range(0, ids.length).filter(i -> Strings.hasText(ids[i]) == false).toArray();
282+
final int[] idxOfBlankIds = IntStream.range(0, idsToValidate.length)
283+
.filter(i -> Strings.hasText(idsToValidate[i]) == false)
284+
.toArray();
283285
if (idxOfBlankIds.length > 0) {
284286
final ActionRequestValidationException validationException = new ActionRequestValidationException();
285287
validationException.addValidationError(

‎x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/role/PutRoleRequest.java

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -98,9 +98,9 @@ public ActionRequestValidationException validate() {
9898
} catch (IllegalArgumentException e) {
9999
validationException = addValidationError(e.getMessage(), validationException);
100100
}
101-
for (String name : privilege.getPrivileges()) {
101+
for (String privilegeName : privilege.getPrivileges()) {
102102
try {
103-
ApplicationPrivilege.validatePrivilegeOrActionName(name);
103+
ApplicationPrivilege.validatePrivilegeOrActionName(privilegeName);
104104
} catch (IllegalArgumentException e) {
105105
validationException = addValidationError(e.getMessage(), validationException);
106106
}
@@ -120,12 +120,12 @@ public void name(String name) {
120120
this.name = name;
121121
}
122122

123-
public void cluster(String... clusterPrivileges) {
124-
this.clusterPrivileges = clusterPrivileges;
123+
public void cluster(String... clusterPrivilegesArray) {
124+
this.clusterPrivileges = clusterPrivilegesArray;
125125
}
126126

127-
public void conditionalCluster(ConfigurableClusterPrivilege... configurableClusterPrivileges) {
128-
this.configurableClusterPrivileges = configurableClusterPrivileges;
127+
public void conditionalCluster(ConfigurableClusterPrivilege... configurableClusterPrivilegesArray) {
128+
this.configurableClusterPrivileges = configurableClusterPrivilegesArray;
129129
}
130130

131131
public void addIndex(RoleDescriptor.IndicesPrivileges... privileges) {

0 commit comments

Comments
 (0)