Skip to content

remove UnusedLocalVariable #34489

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+57 −75
Open

remove UnusedLocalVariable #34489

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 9 additions & 3 deletions .editorconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,16 @@
root = true

[*.{adoc,bat,groovy,html,java,js,jsp,kt,kts,md,properties,py,rb,sh,sql,svg,txt,xml,xsd}]
[*]
charset = utf-8
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for clarification, is the intent to not make this the default?

I have never seen it otherwise, only globally, with the most common encoding being UTF-8.

This is new to me—my previous knowledge was that properties files were the only files not normally encoded in UTF-8, but now even they have switched.

"As of Java 9, properties files switched from being loaded using ISO-8859-1."
Source

end_of_line = lf
# indent_style = tab todo: verify
insert_final_newline = true

[*.{groovy,java,kt,kts,xml,xsd}]
indent_style = tab
indent_size = 4
continuation_indent_size = 8
end_of_line = lf
ij_continuation_indent_size = 8

[*.java]
ij_java_class_count_to_use_import_on_demand = 999
ij_java_names_count_to_use_import_on_demand = 999
5 changes: 4 additions & 1 deletion buildSrc/config/checkstyle/checkstyle.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@
<!-- Modifiers -->
<module name="com.puppycrawl.tools.checkstyle.checks.modifier.ModifierOrderCheck"/>

<!-- Best Practice -->
<module name="EmptyStatement"/>
<module name="UnusedLocalVariable"/>
<module name="VariableDeclarationUsageDistance"/>
</module>

</module>
17 changes: 8 additions & 9 deletions integration-tests/src/test/java/org/springframework/aot/test/ReflectionInvocationsTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@

import org.springframework.aot.hint.RuntimeHints;
import org.springframework.aot.test.agent.EnabledIfRuntimeHintsAgent;
import org.springframework.aot.test.agent.RuntimeHintsInvocations;
import org.springframework.aot.test.agent.RuntimeHintsRecorder;

import static org.assertj.core.api.Assertions.assertThat;

Expand All @@ -34,23 +34,22 @@ void sampleTest() {
RuntimeHints hints = new RuntimeHints();
hints.reflection().registerType(String.class);

RuntimeHintsInvocations invocations = org.springframework.aot.test.agent.RuntimeHintsRecorder.record(() -> {
assertThat(RuntimeHintsRecorder.record(() -> {
SampleReflection sample = new SampleReflection();
sample.sample(); // does Method[] methods = String.class.getMethods();
});
assertThat(invocations).match(hints);
sample.sample(); // String.class.getMethods();
})).match(hints);
}

@Test
void multipleCallsTest() {
RuntimeHints hints = new RuntimeHints();
hints.reflection().registerType(String.class);
hints.reflection().registerType(Integer.class);
RuntimeHintsInvocations invocations = org.springframework.aot.test.agent.RuntimeHintsRecorder.record(() -> {

assertThat(RuntimeHintsRecorder.record(() -> {
SampleReflection sample = new SampleReflection();
sample.multipleCalls(); // does Method[] methods = String.class.getMethods(); methods = Integer.class.getMethods();
});
assertThat(invocations).match(hints);
sample.multipleCalls(); // String.class.getMethods(); Integer.class.getMethods();
})).match(hints);
}

}
12 changes: 3 additions & 9 deletions integration-tests/src/test/java/org/springframework/aot/test/SampleReflection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,25 +16,19 @@

package org.springframework.aot.test;

import java.lang.reflect.Method;

/**
* @author Brian Clozel
* @since 6.0
*/
public class SampleReflection {

@SuppressWarnings("unused")
public void sample() {
String value = "Sample";
Method[] methods = String.class.getMethods();
String.class.getMethods();
}

@SuppressWarnings("unused")
public void multipleCalls() {
String value = "Sample";
Method[] methods = String.class.getMethods();
methods = Integer.class.getMethods();
String.class.getMethods();
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if the test is still working as expected, as I could not execute it locally.

Integer.class.getMethods();
}

}
35 changes: 5 additions & 30 deletions ...rc/test/java/org/springframework/beans/factory/config/ServiceLocatorFactoryBeanTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,16 +123,11 @@ void testStringArgGetter() throws Exception {
.addPropertyValue("serviceLocatorInterface", TestServiceLocator2.class)
.getBeanDefinition());

// test string-arg getter with null id
TestServiceLocator2 factory = (TestServiceLocator2) bf.getBean("factory");

@SuppressWarnings("unused")
TestService testBean = factory.getTestService(null);
// now test with explicit id
testBean = factory.getTestService("testService");
// now verify failure on bad id
assertThatExceptionOfType(NoSuchBeanDefinitionException.class).isThrownBy(() ->
factory.getTestService("bogusTestService"));
final var factory = (TestServiceLocator2) bf.getBean("factory"); // test string-arg getter
factory.getTestService(null); // with null id
factory.getTestService("testService"); // test with explicit id
assertThatExceptionOfType(NoSuchBeanDefinitionException.class)
.isThrownBy(() -> factory.getTestService("bogusTestService")); // verify failure on bad id
}

@Disabled @Test // worked when using an ApplicationContext (see commented), fails when using BeanFactory
Expand All @@ -145,14 +140,6 @@ public void testCombinedLocatorInterface() {
.addPropertyValue("serviceLocatorInterface", TestServiceLocator3.class)
.getBeanDefinition());

// StaticApplicationContext ctx = new StaticApplicationContext();
// ctx.registerPrototype("testService", TestService.class, new MutablePropertyValues());
// ctx.registerAlias("testService", "1");
// MutablePropertyValues mpv = new MutablePropertyValues();
// mpv.addPropertyValue("serviceLocatorInterface", TestServiceLocator3.class);
// ctx.registerSingleton("factory", ServiceLocatorFactoryBean.class, mpv);
// ctx.refresh();

TestServiceLocator3 factory = (TestServiceLocator3) bf.getBean("factory");
TestService testBean1 = factory.getTestService();
TestService testBean2 = factory.getTestService("testService");
Expand All @@ -177,16 +164,6 @@ public void testServiceMappings() {
.addPropertyValue("serviceLocatorInterface", TestServiceLocator3.class)
.addPropertyValue("serviceMappings", "=testService1\n1=testService1\n2=testService2")
.getBeanDefinition());

// StaticApplicationContext ctx = new StaticApplicationContext();
// ctx.registerPrototype("testService1", TestService.class, new MutablePropertyValues());
// ctx.registerPrototype("testService2", ExtendedTestService.class, new MutablePropertyValues());
// MutablePropertyValues mpv = new MutablePropertyValues();
// mpv.addPropertyValue("serviceLocatorInterface", TestServiceLocator3.class);
// mpv.addPropertyValue("serviceMappings", "=testService1\n1=testService1\n2=testService2");
// ctx.registerSingleton("factory", ServiceLocatorFactoryBean.class, mpv);
// ctx.refresh();

TestServiceLocator3 factory = (TestServiceLocator3) bf.getBean("factory");
TestService testBean1 = factory.getTestService();
TestService testBean2 = factory.getTestService("testService1");
Expand Down Expand Up @@ -303,8 +280,6 @@ public interface TestService2Locator {

public interface ServiceLocatorInterfaceWithExtraNonCompliantMethod {

TestService2 getTestService();

TestService2 getTestService(String serviceName, String defaultNotAllowedParameter);
}

Expand Down
3 changes: 1 addition & 2 deletions ...ssion/src/test/java/org/springframework/expression/spel/SpelCompilationCoverageTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7145,8 +7145,7 @@ public TestClass9(int i) {
static class HttpServlet3RequestFactory {

static Servlet3SecurityContextHolderAwareRequestWrapper getOne() {
HttpServlet3RequestFactory outer = new HttpServlet3RequestFactory();
return outer.new Servlet3SecurityContextHolderAwareRequestWrapper();
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

possible false positive on UnusedLocalVariable checkstyle/checkstyle#16419

return new HttpServlet3RequestFactory().new Servlet3SecurityContextHolderAwareRequestWrapper();
}

// private class Servlet3SecurityContextHolderAwareRequestWrapper extends SecurityContextHolderAwareRequestWrapper
Expand Down
30 changes: 11 additions & 19 deletions spring-test/src/test/java/org/springframework/test/web/client/samples/SampleTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,10 +70,9 @@ public void performGet() {
String responseBody = "{\"name\" : \"Ludwig van Beethoven\", \"someDouble\" : \"1.6035\"}";

this.mockServer.expect(requestTo("/composers/42")).andExpect(method(HttpMethod.GET))
.andRespond(withSuccess(responseBody, MediaType.APPLICATION_JSON));
.andRespond(withSuccess(responseBody, MediaType.APPLICATION_JSON));

@SuppressWarnings("unused")
Person ludwig = this.restTemplate.getForObject("/composers/{id}", Person.class, 42);
this.restTemplate.getForObject("/composers/{id}", Person.class, 42);

// We are only validating the request. The response is mocked out.
// hotel.getId() == 42
Expand All @@ -90,8 +89,7 @@ public void performGetManyTimes() {
this.mockServer.expect(manyTimes(), requestTo("/composers/42")).andExpect(method(HttpMethod.GET))
.andRespond(withSuccess(responseBody, MediaType.APPLICATION_JSON));

@SuppressWarnings("unused")
Person ludwig = this.restTemplate.getForObject("/composers/{id}", Person.class, 42);
this.restTemplate.getForObject("/composers/{id}", Person.class, 42);

// We are only validating the request. The response is mocked out.
// hotel.getId() == 42
Expand Down Expand Up @@ -142,8 +140,7 @@ public void performGetWithResponseBodyFromFile() {
this.mockServer.expect(requestTo("/composers/42")).andExpect(method(HttpMethod.GET))
.andRespond(withSuccess(responseBody, MediaType.APPLICATION_JSON));

@SuppressWarnings("unused")
Person ludwig = this.restTemplate.getForObject("/composers/{id}", Person.class, 42);
this.restTemplate.getForObject("/composers/{id}", Person.class, 42);

// hotel.getId() == 42
// hotel.getName().equals("Holiday Inn")
Expand All @@ -155,24 +152,19 @@ public void performGetWithResponseBodyFromFile() {
public void verify() {

this.mockServer.expect(requestTo("/number")).andExpect(method(HttpMethod.GET))
.andRespond(withSuccess("1", MediaType.TEXT_PLAIN));
.andRespond(withSuccess("1", MediaType.TEXT_PLAIN));

this.mockServer.expect(requestTo("/number")).andExpect(method(HttpMethod.GET))
.andRespond(withSuccess("2", MediaType.TEXT_PLAIN));
.andRespond(withSuccess("2", MediaType.TEXT_PLAIN));

this.mockServer.expect(requestTo("/number")).andExpect(method(HttpMethod.GET))
.andRespond(withSuccess("4", MediaType.TEXT_PLAIN));
.andRespond(withSuccess("4", MediaType.TEXT_PLAIN));

this.mockServer.expect(requestTo("/number")).andExpect(method(HttpMethod.GET))
.andRespond(withSuccess("8", MediaType.TEXT_PLAIN));

@SuppressWarnings("unused")
String result1 = this.restTemplate.getForObject("/number", String.class);
// result1 == "1"
.andRespond(withSuccess("8", MediaType.TEXT_PLAIN));

@SuppressWarnings("unused")
String result2 = this.restTemplate.getForObject("/number", String.class);
// result == "2"
assertThat(this.restTemplate.getForObject("/number", String.class)).isEqualTo("1");
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is some real benefit, imho. Testing the real assertion rather commenting about it

assertThat(this.restTemplate.getForObject("/number", String.class)).isEqualTo("2");

try {
this.mockServer.verify();
Expand Down Expand Up @@ -215,7 +207,7 @@ private ContentInterceptor(Resource resource) {

@Override
public ClientHttpResponse intercept(HttpRequest request, byte[] body,
ClientHttpRequestExecution execution) throws IOException {
ClientHttpRequestExecution execution) throws IOException {

ClientHttpResponse response = execution.execute(request, body);
byte[] expected = FileCopyUtils.copyToByteArray(this.resource.getInputStream());
Expand Down
2 changes: 1 addition & 1 deletion ...bmvc/src/test/java/org/springframework/web/servlet/handler/HandlerMethodMappingTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,7 @@ void getCorsConfigWithBeanNameHandler() throws Exception {

this.mapping.setApplicationContext(context);
this.mapping.registerMapping(key, beanName, this.method1);
HandlerMethod handlerMethod = this.mapping.getHandlerInternal(new MockHttpServletRequest("GET", key));
this.mapping.getHandlerInternal(new MockHttpServletRequest("GET", key));
}

@Test
Expand Down
4 changes: 4 additions & 0 deletions src/checkstyle/checkstyle.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,10 @@
<module name="io.spring.javaformat.checkstyle.check.SpringJavadocCheck"/>
<module name="io.spring.javaformat.checkstyle.check.SpringJUnit5Check"/>

<!-- Best Practice -->
<module name="EmptyStatement"/>
<module name="UnusedLocalVariable"/>
<!-- <module name="VariableDeclarationUsageDistance"/>--> <!-- wait for https://github.com/openrewrite/rewrite-static-analysis/issues/469 -->
</module>

</module>
12 changes: 11 additions & 1 deletion src/nohttp/checkstyle.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,14 @@
<property name="optional" value="true"/>
</module>
<module name="SuppressWithPlainTextCommentFilter"/>
</module>
<!-- TreeWalker Checks -->
<module name="TreeWalker">
<!-- Imports -->
<module name="AvoidStarImport"/>
<module name="RedundantImport"/>
<!-- ignored as samples need some flexibility https://github.com/spring-projects/spring-framework/pull/34477 -->
<!-- <module name="UnusedImports"/>-->
<!-- <module name="UnusedLocalVariable"/>-->
<!-- <module name="VariableDeclarationUsageDistance"/>-->
</module>
</module>