Skip to content

Fix shadowed vars pt7 #80996

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Nov 29, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,15 @@ public class HiddenFieldCheck extends AbstractCheck {
/** Control whether to ignore constructor parameters. */
private boolean ignoreConstructorParameter;

/** Control whether to ignore variables in constructor bodies. */
private boolean ignoreConstructorBody;

/** Control whether to ignore parameters of abstract methods. */
private boolean ignoreAbstractMethods;

/** If set, specifies a regex of method names that should be ignored */
private String ignoredMethodNames;

@Override
public int[] getDefaultTokens() {
return getAcceptableTokens();
Expand Down Expand Up @@ -224,7 +230,8 @@ private void processVariable(DetailAST ast) {

if ((frame.containsStaticField(name) || isInstanceField(ast, name))
&& isMatchingRegexp(name) == false
&& isIgnoredParam(ast, name) == false) {
&& isIgnoredParam(ast, name) == false
&& isIgnoredVariable(ast, name) == false) {
log(nameAST, MSG_KEY, name);
}
}
Expand All @@ -238,7 +245,14 @@ && isIgnoredParam(ast, name) == false) {
* @return true if parameter is ignored.
*/
private boolean isIgnoredParam(DetailAST ast, String name) {
return isIgnoredSetterParam(ast, name) || isIgnoredConstructorParam(ast) || isIgnoredParamOfAbstractMethod(ast);
return isVariableInIgnoredMethod(ast, name)
|| isIgnoredSetterParam(ast, name)
|| isIgnoredConstructorParam(ast)
|| isIgnoredParamOfAbstractMethod(ast);
}

private boolean isIgnoredVariable(DetailAST ast, String name) {
return isIgnoredVariableInConstructorBody(ast, name);
}

/**
Expand Down Expand Up @@ -410,6 +424,42 @@ private boolean isIgnoredParamOfAbstractMethod(DetailAST ast) {
return result;
}

/**
* Decides whether to ignore an AST node that is the parameter of a method whose
* name matches the {@link #ignoredMethodNames} regex, if set.
* @param ast the AST to check
* @return true is the ast should be ignored because the parameter belongs to a
* method whose name matches the regex.
*/
private boolean isVariableInIgnoredMethod(DetailAST ast, String name) {
boolean result = false;
if (ignoredMethodNames != null && (ast.getType() == TokenTypes.PARAMETER_DEF || ast.getType() == TokenTypes.VARIABLE_DEF)) {
DetailAST method = ast.getParent();
while (method != null && method.getType() != TokenTypes.METHOD_DEF) {
method = method.getParent();
}
if (method != null && method.getType() == TokenTypes.METHOD_DEF) {
final String methodName = method.findFirstToken(TokenTypes.IDENT).getText();
result = methodName.matches(ignoredMethodNames);
}
}
return result;
}

private boolean isIgnoredVariableInConstructorBody(DetailAST ast, String name) {
boolean result = false;

if (ignoreConstructorBody && ast.getType() == TokenTypes.VARIABLE_DEF) {
DetailAST method = ast.getParent();
while (method != null && method.getType() != TokenTypes.CTOR_DEF) {
method = method.getParent();
}
result = method != null && method.getType() == TokenTypes.CTOR_DEF;
}

return result;
}

/**
* Setter to define the RegExp for names of variables and parameters to ignore.
*
Expand Down Expand Up @@ -463,6 +513,14 @@ public void setIgnoreAbstractMethods(boolean ignoreAbstractMethods) {
this.ignoreAbstractMethods = ignoreAbstractMethods;
}

public void setIgnoredMethodNames(String ignoredMethodNames) {
this.ignoredMethodNames = ignoredMethodNames;
}

public void setIgnoreConstructorBody(boolean ignoreConstructorBody) {
this.ignoreConstructorBody = ignoreConstructorBody;
}

/**
* Holds the names of static and instance fields of a type.
*/
Expand Down
4 changes: 3 additions & 1 deletion build-tools-internal/src/main/resources/checkstyle.xml
Original file line number Diff line number Diff line change
Expand Up @@ -111,10 +111,12 @@
<!--
<module name="org.elasticsearch.gradle.internal.checkstyle.HiddenFieldCheck">
<property name="ignoreConstructorParameter" value="true" />
<property name="ignoreConstructorBody" value="true"/>
<property name="ignoreSetter" value="true" />
<property name="setterCanReturnItsClass" value="true"/>
<property name="ignoreFormat" value="^(threadPool)$"/>
<property name="ignoreFormat" value="^(?:threadPool)$"/>
<property name="ignoreAbstractMethods" value="true"/>
<property name="ignoredMethodNames" value="^(?:createParser)$"/>
<message key="hidden.field" value="''{0}'' hides a field." />
</module>
-->
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1421,8 +1421,8 @@ protected void innerSendShardChangesRequest(
@Override
protected Scheduler.Cancellable scheduleBackgroundRetentionLeaseRenewal(final LongSupplier followerGlobalCheckpoint) {
if (scheduleRetentionLeaseRenewal.get()) {
final ScheduledThreadPoolExecutor scheduler = Scheduler.initScheduler(Settings.EMPTY, "test-scheduler");
final ScheduledFuture<?> future = scheduler.scheduleWithFixedDelay(
final ScheduledThreadPoolExecutor testScheduler = Scheduler.initScheduler(Settings.EMPTY, "test-scheduler");
final ScheduledFuture<?> future = testScheduler.scheduleWithFixedDelay(
() -> retentionLeaseRenewal.accept(followerGlobalCheckpoint.getAsLong()),
0,
TimeValue.timeValueMillis(200).millis(),
Expand All @@ -1433,7 +1433,7 @@ protected Scheduler.Cancellable scheduleBackgroundRetentionLeaseRenewal(final Lo
@Override
public boolean cancel() {
final boolean cancel = future.cancel(true);
scheduler.shutdown();
testScheduler.shutdown();
return cancel;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -109,8 +109,8 @@ public void testFollowingEngineRejectsNonFollowingIndex() throws IOException {

public void testIndexSeqNoIsMaintained() throws IOException {
final long seqNo = randomIntBetween(0, Integer.MAX_VALUE);
runIndexTest(seqNo, Engine.Operation.Origin.PRIMARY, (followingEngine, index) -> {
final Engine.IndexResult result = followingEngine.index(index);
runIndexTest(seqNo, Engine.Operation.Origin.PRIMARY, (followingEngine, indexToTest) -> {
final Engine.IndexResult result = followingEngine.index(indexToTest);
assertThat(result.getSeqNo(), equalTo(seqNo));
});
}
Expand Down Expand Up @@ -156,8 +156,8 @@ public void runIndexTest(
try (Store store = createStore(shardId, indexSettings, newDirectory())) {
final EngineConfig engineConfig = engineConfig(shardId, indexSettings, threadPool, store);
try (FollowingEngine followingEngine = createEngine(store, engineConfig)) {
final Engine.Index index = indexForFollowing("id", seqNo, origin);
consumer.accept(followingEngine, index);
final Engine.Index indexToTest = indexForFollowing("id", seqNo, origin);
consumer.accept(followingEngine, indexToTest);
}
}
}
Expand Down Expand Up @@ -226,16 +226,21 @@ public void testDoNotFillSeqNoGaps() throws Exception {
}

private EngineConfig engineConfig(
final ShardId shardId,
final ShardId shardIdValue,
final IndexSettings indexSettings,
final ThreadPool threadPool,
final Store store
) {
final IndexWriterConfig indexWriterConfig = newIndexWriterConfig();
final Path translogPath = createTempDir("translog");
final TranslogConfig translogConfig = new TranslogConfig(shardId, translogPath, indexSettings, BigArrays.NON_RECYCLING_INSTANCE);
final TranslogConfig translogConfig = new TranslogConfig(
shardIdValue,
translogPath,
indexSettings,
BigArrays.NON_RECYCLING_INSTANCE
);
return new EngineConfig(
shardId,
shardIdValue,
threadPool,
indexSettings,
null,
Expand Down Expand Up @@ -331,26 +336,26 @@ private Engine.Delete deleteForPrimary(String id) {
return new Engine.Delete(parsedDoc.id(), EngineTestCase.newUid(parsedDoc), primaryTerm.get());
}

private Engine.Result applyOperation(Engine engine, Engine.Operation op, long primaryTerm, Engine.Operation.Origin origin)
private Engine.Result applyOperation(Engine engine, Engine.Operation op, long primaryTermValue, Engine.Operation.Origin origin)
throws IOException {
final VersionType versionType = origin == Engine.Operation.Origin.PRIMARY ? VersionType.EXTERNAL : null;
final Engine.Result result;
if (op instanceof Engine.Index) {
Engine.Index index = (Engine.Index) op;
Engine.Index engineIndex = (Engine.Index) op;
result = engine.index(
new Engine.Index(
index.uid(),
index.parsedDoc(),
index.seqNo(),
primaryTerm,
index.version(),
engineIndex.uid(),
engineIndex.parsedDoc(),
engineIndex.seqNo(),
primaryTermValue,
engineIndex.version(),
versionType,
origin,
index.startTime(),
index.getAutoGeneratedIdTimestamp(),
index.isRetry(),
index.getIfSeqNo(),
index.getIfPrimaryTerm()
engineIndex.startTime(),
engineIndex.getAutoGeneratedIdTimestamp(),
engineIndex.isRetry(),
engineIndex.getIfSeqNo(),
engineIndex.getIfPrimaryTerm()
)
);
} else if (op instanceof Engine.Delete) {
Expand All @@ -360,7 +365,7 @@ private Engine.Result applyOperation(Engine engine, Engine.Operation op, long pr
delete.id(),
delete.uid(),
delete.seqNo(),
primaryTerm,
primaryTermValue,
delete.version(),
versionType,
origin,
Expand All @@ -371,7 +376,7 @@ private Engine.Result applyOperation(Engine engine, Engine.Operation op, long pr
);
} else {
Engine.NoOp noOp = (Engine.NoOp) op;
result = engine.noOp(new Engine.NoOp(noOp.seqNo(), primaryTerm, origin, noOp.startTime(), noOp.reason()));
result = engine.noOp(new Engine.NoOp(noOp.seqNo(), primaryTermValue, origin, noOp.startTime(), noOp.reason()));
}
return result;
}
Expand Down Expand Up @@ -828,7 +833,7 @@ public void testProcessOnceOnPrimary() throws Exception {
*/
public void testVerifyShardBeforeIndexClosingIsNoOp() throws IOException {
final long seqNo = randomIntBetween(0, Integer.MAX_VALUE);
runIndexTest(seqNo, Engine.Operation.Origin.PRIMARY, (followingEngine, index) -> {
runIndexTest(seqNo, Engine.Operation.Origin.PRIMARY, (followingEngine, indexToTest) -> {
globalCheckpoint.set(randomNonNegativeLong());
try {
followingEngine.verifyEngineBeforeIndexClosing();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ public void testToXContent() throws IOException {
final NavigableMap<String, AutoFollowedCluster> trackingClusters = new TreeMap<>(
Collections.singletonMap(randomAlphaOfLength(4), new AutoFollowedCluster(1L, 1L))
);
final AutoFollowStats autoFollowStats = new AutoFollowStats(
autoFollowStats = new AutoFollowStats(
randomNonNegativeLong(),
randomNonNegativeLong(),
randomNonNegativeLong(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ public void testToXContent() throws IOException {
)
);
final long timeSinceLastReadMillis = randomNonNegativeLong();
final ShardFollowNodeTaskStatus status = new ShardFollowNodeTaskStatus(
final ShardFollowNodeTaskStatus taskStatus = new ShardFollowNodeTaskStatus(
"leader_cluster",
"leader_index",
"follower_index",
Expand Down Expand Up @@ -148,7 +148,7 @@ public void testToXContent() throws IOException {
timeSinceLastReadMillis,
new ElasticsearchException("fatal error")
);
final FollowStatsMonitoringDoc document = new FollowStatsMonitoringDoc("_cluster", timestamp, intervalMillis, node, status);
final FollowStatsMonitoringDoc document = new FollowStatsMonitoringDoc("_cluster", timestamp, intervalMillis, node, taskStatus);
final BytesReference xContent = XContentHelper.toXContent(document, XContentType.JSON, false);
assertThat(
xContent.utf8ToString(),
Expand Down Expand Up @@ -273,7 +273,7 @@ public void testShardFollowNodeTaskStatusFieldsMapped() throws IOException {
final NavigableMap<Long, Tuple<Integer, ElasticsearchException>> fetchExceptions = new TreeMap<>(
Collections.singletonMap(1L, Tuple.tuple(2, new ElasticsearchException("shard is sad")))
);
final ShardFollowNodeTaskStatus status = new ShardFollowNodeTaskStatus(
final ShardFollowNodeTaskStatus taskStatus = new ShardFollowNodeTaskStatus(
"remote_cluster",
"leader_index",
"follower_index",
Expand Down Expand Up @@ -305,7 +305,7 @@ public void testShardFollowNodeTaskStatusFieldsMapped() throws IOException {
new ElasticsearchException("fatal error")
);
XContentBuilder builder = jsonBuilder();
builder.value(status);
builder.value(taskStatus);
Map<String, Object> serializedStatus = XContentHelper.convertToMap(XContentType.JSON.xContent(), Strings.toString(builder), false);

byte[] loadedTemplate = MonitoringTemplateRegistry.getTemplateConfigForMonitoredSystem(MonitoredSystem.ES).loadBytes();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ public String[] indices() {
}

@Override
public IndicesRequest indices(String... indices) {
public IndicesRequest indices(@SuppressWarnings("HiddenField") String... indices) {
Objects.requireNonNull(indices, "indices must not be null");
for (String index : indices) {
Objects.requireNonNull(index, "index must not be null");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -787,7 +787,7 @@ public static void validate(String expression) throws IllegalArgumentException {
//
////////////////////////////////////////////////////////////////////////////

private void buildExpression(String expression) {
private void buildExpression(@SuppressWarnings("HiddenField") String expression) {

try {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,8 +100,8 @@ public void writeTo(StreamOutput out) throws IOException {
out.writeLong(expirationTimeMillis);
}

public AsyncSearchResponse clone(String id) {
return new AsyncSearchResponse(id, searchResponse, error, isPartial, false, startTimeMillis, expirationTimeMillis);
public AsyncSearchResponse clone(String searchId) {
return new AsyncSearchResponse(searchId, searchResponse, error, isPartial, false, startTimeMillis, expirationTimeMillis);
}

/**
Expand Down Expand Up @@ -165,8 +165,8 @@ public long getExpirationTime() {
}

@Override
public AsyncSearchResponse withExpirationTime(long expirationTimeMillis) {
return new AsyncSearchResponse(id, searchResponse, error, isPartial, isRunning, startTimeMillis, expirationTimeMillis);
public AsyncSearchResponse withExpirationTime(long expirationTime) {
return new AsyncSearchResponse(id, searchResponse, error, isPartial, isRunning, startTimeMillis, expirationTime);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -343,7 +343,7 @@ public static String apiKeyHeaderValue(SecureString apiKey) {
* Returns a TrustManager to be used in a client SSLContext, which trusts all certificates that are signed
* by a specific CA certificate ( identified by its SHA256 fingerprint, {@code pinnedCaCertFingerPrint} )
*/
private TrustManager fingerprintTrustingTrustManager(String pinnedCaCertFingerprint) {
private TrustManager fingerprintTrustingTrustManager(String caCertFingerprint) {
final TrustManager trustManager = new X509TrustManager() {
public void checkClientTrusted(X509Certificate[] chain, String authType) throws CertificateException {}

Expand All @@ -354,7 +354,7 @@ public void checkServerTrusted(X509Certificate[] chain, String authType) throws
final Certificate caCertFromChain = chain[1];
MessageDigest sha256 = MessageDigests.sha256();
sha256.update(caCertFromChain.getEncoded());
if (MessageDigests.toHexString(sha256.digest()).equals(pinnedCaCertFingerprint) == false) {
if (MessageDigests.toHexString(sha256.digest()).equals(caCertFingerprint) == false) {
throw new CertificateException();
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -249,14 +249,16 @@ public int hashCode() {
return Objects.hash(realmName, userName, ids, name, ownedByAuthenticatedUser);
}

private void validateIds(@Nullable String[] ids) {
if (ids != null) {
if (ids.length == 0) {
private void validateIds(@Nullable String[] idsToValidate) {
if (idsToValidate != null) {
if (idsToValidate.length == 0) {
final ActionRequestValidationException validationException = new ActionRequestValidationException();
validationException.addValidationError("Field [ids] cannot be an empty array");
throw validationException;
} else {
final int[] idxOfBlankIds = IntStream.range(0, ids.length).filter(i -> Strings.hasText(ids[i]) == false).toArray();
final int[] idxOfBlankIds = IntStream.range(0, idsToValidate.length)
.filter(i -> Strings.hasText(idsToValidate[i]) == false)
.toArray();
if (idxOfBlankIds.length > 0) {
final ActionRequestValidationException validationException = new ActionRequestValidationException();
validationException.addValidationError(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,9 +95,9 @@ public ActionRequestValidationException validate() {
} catch (IllegalArgumentException e) {
validationException = addValidationError(e.getMessage(), validationException);
}
for (String name : privilege.getPrivileges()) {
for (String privilegeName : privilege.getPrivileges()) {
try {
ApplicationPrivilege.validatePrivilegeOrActionName(name);
ApplicationPrivilege.validatePrivilegeOrActionName(privilegeName);
} catch (IllegalArgumentException e) {
validationException = addValidationError(e.getMessage(), validationException);
}
Expand All @@ -117,12 +117,12 @@ public void name(String name) {
this.name = name;
}

public void cluster(String... clusterPrivileges) {
this.clusterPrivileges = clusterPrivileges;
public void cluster(String... clusterPrivilegesArray) {
this.clusterPrivileges = clusterPrivilegesArray;
}

public void conditionalCluster(ConfigurableClusterPrivilege... configurableClusterPrivileges) {
this.configurableClusterPrivileges = configurableClusterPrivileges;
public void conditionalCluster(ConfigurableClusterPrivilege... configurableClusterPrivilegesArray) {
this.configurableClusterPrivileges = configurableClusterPrivilegesArray;
}

public void addIndex(RoleDescriptor.IndicesPrivileges... privileges) {
Expand Down
Loading