Skip to content
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

Add -Werror for javac #107

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Add -Werror for javac #107

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
2e0a364
add -Werror for javac
nimakarimipour Dec 7, 2022
3ce7e93
undo unnecessary changes
nimakarimipour Dec 7, 2022
b16d41e
update comment
nimakarimipour Dec 7, 2022
f14c539
use from dependencies.gradle
nimakarimipour Dec 7, 2022
f293923
remove all warnings
nimakarimipour Dec 29, 2022
a519178
reformat
nimakarimipour Dec 29, 2022
a10d195
merge master
nimakarimipour Dec 29, 2022
fb4d27c
update comment
nimakarimipour Dec 29, 2022
6517fba
change linkedList to arrayList
nimakarimipour Jan 2, 2023
75423ca
merge master
nimakarimipour Jan 2, 2023
309184b
Merge branch 'master' into nimak/add-werror
nimakarimipour Mar 12, 2023
a3d4fe0
update
nimakarimipour Mar 12, 2023
5e76579
merge master into this
nimakarimipour Mar 24, 2023
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
2 changes: 1 addition & 1 deletion annotator-core/src/main/java/edu/ucr/cs/riple/core/metadata/graph/ConflictGraph.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ public Node addNodeToVertices(Fix fix) {
* in the same group. A greedy algorithm is used to find the solution.
*/
// TODO: Remove SuppressWarnings below later.
@SuppressWarnings("JdkObsolete")
@SuppressWarnings({"JdkObsolete", "unchecked"})
Copy link
Member

Choose a reason for hiding this comment

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

Why not just fix the error instead of suppressing?

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't know how I can resolve the warning for creating array of LinkedLists. Since I was confident that there will be no problem with the code I just silenced it.

However, I just changed it to use List of Lists 6517fba, I don't think this will impact the performance. But if you think this will actually impact the performance, please let me know and I will undo this. Thank you.

public void findGroups() {
this.groups.clear();
Collection<Node> allNodes = nodes.values();
Expand Down
4 changes: 4 additions & 0 deletions build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,9 @@ subprojects { proj ->
proj.dependencies {
errorprone deps.build.errorProneCore
errorproneJavac deps.build.errorProneJavac
// just to prevent javac warnings, we currently pass "-Werror" as compiler arguments,
// all warnings must be resolved.
compileOnly deps.build.errorProneCoreOld
Comment on lines +51 to +53
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this change

Copy link
Member Author

Choose a reason for hiding this comment

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

it seems once error prone is in the path we get the error below for compiling annotator-core:

warning: unknown enum constant SeverityLevel.SUGGESTION
  reason: class file for com.google.errorprone.BugPattern$SeverityLevel not found

Added a dependency to resolve this issue.

if(proj.name != "ban-mutable-static"){
annotationProcessor project(":checks:ban-mutable-static")
}
Expand All @@ -66,6 +69,7 @@ subprojects { proj ->
}
}
}
options.compilerArgs += '-Werror'
}

repositories {
Expand Down
1 change: 1 addition & 0 deletions gradle/dependencies.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ def build = [
javaparser : "com.github.javaparser:javaparser-core:${versions.javaparser}",
commonscli : "commons-cli:commons-cli:${versions.cli}",
commonsio : "commons-io:commons-io:${versions.commonsio}",
errorProneCoreOld : "com.google.errorprone:error_prone_core:${oldestErrorProneApi}",
errorProneCore : "com.google.errorprone:error_prone_core:${versions.errorProne}",
errorProneJavac : "com.google.errorprone:javac:9+181-r4173-1",
errorProneCheckApi : "com.google.errorprone:error_prone_check_api:${versions.errorProneApi}",
Expand Down
1 change: 1 addition & 0 deletions injector/src/main/java/edu/ucr/cs/riple/injector/changes/RemoveAnnotation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ public Modification visit(NodeWithAnnotations<?> node, Range range) {
}

@Override
@SuppressWarnings("unchecked")
Copy link
Member

Choose a reason for hiding this comment

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

Again why suppress

Copy link
Member Author

Choose a reason for hiding this comment

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

@msridhar The only way to resolve this warning is to change the json dependency to org.json from com.googlecode.json-simple:json-simple since it is compiled with a very old javac (the latest release is for 2012). I am definitely interested in updating this dependency to use org.json but that will include a lot of changes which makes this PR pretty large. Should I do it in a follow up PR with an issue for now to track it ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just created a followup PR that removes the added @SuppressWarnings in this PR and all existing ones by changing dependency to org.json. #123.

public JSONObject getJson() {
JSONObject res = super.getJson();
res.put("INJECT", false);
Expand Down
4 changes: 2 additions & 2 deletions injector/src/test/java/edu/ucr/cs/riple/injector/BasicTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ public class BasicTest extends BaseInjectorTest {

@Test
public void skip_duplicate_annotation() {
Change Change =
Change change =
new AddMarkerAnnotation(
new OnMethod("Super.java", "com.uber.Super", "test()"), "javax.annotation.Nullable");
injectorTestHelper
Expand All @@ -57,7 +57,7 @@ public void skip_duplicate_annotation() {
" return new Object();",
" }",
"}")
.addChanges(Change, Change.duplicate(), Change.duplicate())
.addChanges(change, change.duplicate(), change.duplicate())
.start();
}

Expand Down
3 changes: 2 additions & 1 deletion injector/src/test/java/edu/ucr/cs/riple/injector/ClassSearchTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import edu.ucr.cs.riple.injector.location.OnMethod;
import java.io.ByteArrayOutputStream;
import java.io.PrintStream;
import java.nio.charset.Charset;
import java.util.Collections;
import org.junit.Test;
import org.junit.runner.RunWith;
Expand Down Expand Up @@ -465,7 +466,7 @@ public void proper_report_in_err_std_for_class_not_found_test() {
new OnMethod("Main.java", "com.test.NotIncluded", "foo(java.lang.Object)"),
"javax.annotation.Nullable"))
.start();
assertTrue(err.toString().contains(expectedErrorMessage));
assertTrue(err.toString(Charset.defaultCharset()).contains(expectedErrorMessage));
}

@Test
Expand Down
3 changes: 2 additions & 1 deletion injector/src/test/java/edu/ucr/cs/riple/injector/tools/Utility.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@

package edu.ucr.cs.riple.injector.tools;

import com.google.common.base.Splitter;
import java.nio.file.Path;

public class Utility {
Expand All @@ -38,7 +39,7 @@ public class Utility {
*/
public static Path pathOf(Path root, String path) {
Path ans = root;
String[] dirs = path.split("/");
Iterable<String> dirs = Splitter.on('/').split(path);
for (String dir : dirs) {
ans = ans.resolve(dir);
}
Expand Down