chore: Update to sonar-java 6.9.0 #156

slarse · 2020-10-19T13:45:47Z

This PR depends on #144 (that's why it looks huge), I will rebase on master once that's merged. Only b6b3383 and onward is actually new.

The transition to sonar-java 6.9.0 mostly went smoothly, but there were/are a few problems.

First, the interface of the JavaCheckVerifier/MultiFilesJavaCheckVerifier have been entirely deprecated, and so everything needed to be revamped. I've added an adapter, Verifier, that serves as the interface with the currently unstable API of the verifier functionality of sonar-java.

Second, some checks have slightly altered behavior. In particular, ArrayHashCodeAndToStringCheck doesn't flag the code in CodeFactory.java as non-compliant anymore: https://github.com/SpoonLabs/sorald/blob/ea7d6264d5ad02eb52189fed7cda65ebac516a6d/src/test/resources/CodeFactory.java#L19-L23

I'm not sure if this is a bug, I'll keep investigating.

Third, the miner test fails miserably. I haven't looked at that yet.

slarse · 2020-10-19T15:08:03Z

The miner test fails because of some very strange use of paths in the internals of sonar-java. Essentially, it assumes that the current working directory is the root of the project being analyzed, and then tries to resolve input files relative to it. I'm trying to work around it, but it may require us to patch sonar-java. It's not really designed to be used the way we're using it.

slarse · 2020-10-19T15:14:10Z

Actually, the relative paths thing doesn't appear to be the whole issue. Will need to keep debugging.

slarse · 2020-10-20T08:21:25Z

Now I've figured out the actual problem. We're essentially using the testing tooling for sonar-java, and a newly introduced feature is that the verifyIssues function looks for two things: lines with non-compliant code, and lines // Noncompliant comments. If it finds non-compliant code on a line that lacks such a comment, it will raise an assertion error.

Key code parts

Here it adds the parser that finds the comments
- And here's where the comment extraction happens
Here is where the matching between expected issues and actual issues occurs
- "Expected issues" as in lines that contain the // Noncompliant comments

This entirely defeats our use case, as the code we want to analyze will obviously not have comments marking non-compliant code. I will keep digging a bit to see if there's any way around this behavior, but I don't think we have any other choice than to fork the project. It'd be a pretty low-maintenance fork, though.

fermadeiral · 2020-10-20T09:02:15Z

I'm trying to work around it, but it may require us to patch sonar-java. It's not really designed to be used the way we're using it.

I agree with you, sonar-java is not designed to be used through the java API (it's designed to be used through the SonarSource products, i.e., SonarQube, SonarCloud, and SonarLint), which is why I believe they might not accept patches from us that change their API, but we can try!

a newly introduced feature is that the verifyIssues function looks for two things: lines with non-compliant code, and lines // Noncompliant comments. If it finds non-compliant code on a line that lacks such a comment, it will raise an assertion error.

Arghhhh.

I will keep digging a bit to see if there's any way around this behavior

Ok.

I think it would be nice to discuss this issue by voice (in person or remotely).

slarse · 2020-10-20T09:12:00Z

I think it would be nice to discuss this issue by voice (in person or remotely).

I agree. We may have time to touch upon it during the sync meeting today at 14. At the very least, we should have time to schedule such a discussion for some other time :)

slarse · 2020-10-20T10:54:37Z

a newly introduced feature is that the verifyIssues function looks for two things: lines with non-compliant code, and lines // Noncompliant comments. If it finds non-compliant code on a line that lacks such a comment, it will raise an assertion error.

Okay, so there is a very simple fix for this in the testing utilities. Simply change the method here like so:

  private void checkIssues(Set<AnalyzerMessage> issues) {
+   if (customIssueVerifier != null) {
+     customIssueVerifier.accept(issues);
+   }
    if (expectations.expectNoIssues()) {
      assertNoIssues(issues);
    } else if (expectations.expectIssueAtFileLevel() || expectations.expectIssueAtProjectLevel()) {
      assertComponentIssue(issues);
    } else {
      assertMultipleIssues(issues);
    }
-   if (customIssueVerifier != null) {
-     customIssueVerifier.accept(issues);
-   }
  }

In other words, invoke the customIssueVerifier.accept method before checking. This way, the custom issue collector that we define gets the list of issues regardless of if they're found to be "expected" or not. ~~I've not been able to get the tests for sonar-java to run because of signature errors on the jdt jarfiles, so I can't tell if it breaks anything.~~

Got it to run, does not break any tests. This is however only something of a band-aid, as there are still assertion errors being thrown left and right. It'd be better to be able to just get the analyzer messages, I'll see if I can figure that out without too many changes.

slarse · 2020-10-20T11:45:07Z

I think a better solution here is to not use the testing methods (e.g. verifyIssues) in production, but rather build our own wrappers around the core components. The testing methods do a whole lot more than we want to do. Essentially, all we want sonar-java to do for us outside of our unit tests is to scan ASTs for issues, and then we take it from there.

slarse · 2020-10-21T11:10:50Z

I've been messing about with this all day. Essentially, doing this the "proper" way does not seem possible, because there is currently no way to include both sonarlint-core and sonar-java in the same project. The problem is that the former is signed, and the latter isn't, and they have some overlapping packages which causes security exceptions. I've been trying for hours and can't get around it, short of manually removing the signatures from the jarfiles.

There's another way, but that still involves using some amount of testing infrastructure. It's the easiest for now, though, so I'll go with that. With a proper facade around the sonarqube stuff, future refactoring should be simple.

codecov · 2020-10-23T14:15:41Z

Codecov Report

Merging #156 into master will increase coverage by 0.17%.
The diff coverage is 92.98%.

@@             Coverage Diff              @@
##             master     #156      +/-   ##
============================================
+ Coverage     82.25%   82.43%   +0.17%     
- Complexity      367      374       +7     
============================================
  Files            39       40       +1     
  Lines          1471     1514      +43     
  Branches        215      216       +1     
============================================
+ Hits           1210     1248      +38     
- Misses          159      164       +5     
  Partials        102      102

Impacted Files	Coverage Δ	Complexity Δ
src/main/java/sorald/Constants.java	`75.00% <ø> (ø)`	`1.00 <0.00> (ø)`
src/main/java/sorald/sonar/Checks.java	`93.02% <ø> (ø)`	`12.00 <0.00> (ø)`
src/main/java/sorald/FileUtils.java	`75.00% <75.00%> (ø)`	`4.00 <4.00> (?)`
src/main/java/sorald/sonar/RuleVerifier.java	`88.37% <92.10%> (-1.63%)`	`7.00 <7.00> (+3.00)`	⬇️
src/main/java/sorald/DefaultRepair.java	`96.55% <100.00%> (+0.06%)`	`16.00 <0.00> (ø)`
src/main/java/sorald/SegmentRepair.java	`87.09% <100.00%> (+0.65%)`	`12.00 <0.00> (ø)`
src/main/java/sorald/SoraldConfig.java	`100.00% <100.00%> (ø)`	`23.00 <1.00> (ø)`
src/main/java/sorald/miner/MineSonarWarnings.java	`74.01% <100.00%> (ø)`	`13.00 <0.00> (ø)`
...java/sorald/processor/SoraldAbstractProcessor.java	`85.71% <100.00%> (+0.23%)`	`23.00 <0.00> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c76d525...ac530a2. Read the comment docs.

slarse

@fermadeiral I'm not sure I'd call this ready for merge, but I think it's time you look it over.

Essentially, I've managed to upgrade to sonar-java 6.9.0. I ran into quite a few issues with inconsistent use of relative/absolute and non-normalized/normalized paths, which prompted me to open #166 .

There's also one point of breakage: The previously noncompliant code in CodeFactory.java is now not detected as noncompliant. I've ignored it for now, I have not been able to solve it, nor figure out why it happens.

I've also found our use case is a little bit special. sonar is built on a client/server model, and our application is both the server (that generates the analyzer messages) and the client (that consumes them). It doesn't quite mesh with the design of the libraries. These parts are also largely undocumented, and so this is something of trial and error on my part. I've put in a few hacks here (see review comments) to make it work for now, but I expect to be able to remove them as my understanding of how sonar is built improves.

To be clear, though, the way sorald used sonar before was also a hack, so this is not a step backwards. Perhaps a bit sideways :)

Let me know what you think of all of this, and if you have any ideas for the particular hacks that I've employed. As you've been working on this longer, you may know things I don't.

slarse · 2020-10-23T14:25:15Z

src/main/java/sorald/sonar/RuleVerifier.java

+                new VisitorsBridge(
+                        checks,
+                        // TODO set the classpath to something reasonable
+                        Collections.emptyList(),


This argument sets the classpath for the VisitorsBridge. It's later supplied to the parser, so I'm assuming that it's used to resolve types etc (but I haven't checked inside the parser). We will probably want to set it to something reasonable in the context. For example, when analyzing a project that uses maven, the full classpath can be generated with mvn dependency:build-classpath.

slarse · 2020-10-23T14:26:04Z

src/main/java/sorald/sonar/RuleVerifier.java

+        // FIXME The SensorContextTester is an internal and unstable component in sonar,
+        //       we should implement our own SensorContext
+        SensorContextTester context = SensorContextTester.create(baseDir);


Like the comment in the code says, we shouldn't be using the internal SensorContextTester, but it will serve for the time being.

slarse · 2020-10-23T14:26:50Z

src/main/java/sorald/sonar/RuleVerifier.java

+            // FIXME I'm very unsure about supplying null for all of these constructor values.
+            //       They should not be null, but at this time I don't know that to put there, and
+            //       with our current usage this hack appears to work.
+            super(null, fs, null, null, null, null);


Like the comment says, this is a pretty nasty hack. It appears to work, but it's definitely not pretty nor safe as it initializes fields that should not be null to null.

fermadeiral · 2020-10-28T12:13:37Z

Conflict.

slarse · 2020-10-28T13:38:13Z

Conflicts fixed, and also applied a trivial optimization in 163e4c2 that makes execution of checks that don't require symbolic execution several times faster.

Let's postpone a review of this until next week, I'll probably need the rest of this week to conjure up the rule prioritization scripts anyway :)

fermadeiral · 2020-10-29T16:15:03Z

@fermadeiral I'm not sure I'd call this ready for merge, but I think it's time you look it over.

I've done it.

There's also one point of breakage: The previously noncompliant code in CodeFactory.java is now not detected as noncompliant. I've ignored it for now, I have not been able to solve it, nor figure out why it happens.

In the current stage of the things, this is minor in my opinion, I'm happy you just ignored it.

To be clear, though, the way sorald used sonar before was also a hack, so this is not a step backwards. Perhaps a bit sideways :)

Maybe we will always need to hack, because the SonarSource team didn't design its products to be used like we want. I see this PR as a step forwards because now we use a newer version of sonar java at least.

For now, I think this PR is pretty good. Ready to merge?

slarse · 2020-10-29T16:32:32Z

@fermadeiral Yes, ready to merge!

fermadeiral · 2020-10-29T18:05:21Z

@slarse thank you for this great PR!

algomaster99 · 2021-12-07T17:19:34Z

pom.xml

+            <exclusions>
+                <exclusion>
+                    <!-- must be excluded as it is signed, which causes problems with unsigned version from sonar -->
+                    <groupId>org.eclipse.jdt</groupId>
+                    <artifactId>org.eclipse.jdt.core</artifactId>
+                </exclusion>
+            </exclusions>


@slarse Do you remember what problems were arising in the absence of its exclusion? I am using the local build of spoon with this change in the comment, but sorald throws an error that it cannot find the .get method for HashtableOfPackage. I am assuming it is because of this line.

I tried removing the exclusion and now it throws OutOfMemory: Java heap space. Weird.

As I recall, Sonar-java 6.9 pulled in (or bundled, maybe) an unsigned jdt package, while the one pulled in from Spoon is signed. Signed as in there's a GPG signature on it.

That caused errors on building it.

As both Spoon and Sonar-Java use jdt, you need to make sure that the version they depend on is the same. Otherwise you'll get strange errors.

slarse added the WiP label Oct 19, 2020

slarse mentioned this pull request Oct 20, 2020

update to sonar-java >=6.3 #89

Closed

slarse force-pushed the issue/89-update-to-sonar-java-6.9 branch from 56ec751 to f5a7e62 Compare October 20, 2020 09:43

This was referenced Oct 21, 2020

refactor: Encapsulate sonar-java verification facilities in adapter #162

Merged

refactor: Add facade around AnalyzerMessage #163

Merged

slarse added 6 commits October 23, 2020 08:10

Upgrade to sonar-java 6.9

8f0ebc9

Fix issues with paths

cb7a334

Ignore the code factory test for now

c136067

Refactor RuleVerifier

f6bd695

Move comment in pom into proper scope

05db8af

Add comment that we may want to set the Java version

7cc7297

slarse force-pushed the issue/89-update-to-sonar-java-6.9 branch from 889c5f5 to 7cc7297 Compare October 23, 2020 14:06

Format code with spotless

462d205

Fix weird comment formatting

6e3e2b7

slarse commented Oct 23, 2020

View reviewed changes

slarse added ready-for-review and removed WiP labels Oct 23, 2020

slarse requested a review from fermadeiral October 26, 2020 10:40

slarse mentioned this pull request Oct 27, 2020

Optimize warnings miner by running all checks in one pass #168

Closed

slarse changed the title ~~wip: Update to sonar-java 6.9.0~~ chore: Update to sonar-java 6.9.0 Oct 28, 2020

fermadeiral removed the ready-for-review label Oct 28, 2020

fermadeiral added the WiP label Oct 28, 2020

slarse added 2 commits October 28, 2020 14:24

Merge branch 'master' into issue/89-update-to-sonar-java-6.9

1a721d9

Apply optimization by only running symbolic execution if necessary

163e4c2

Bump spoon version to 8.3.0

ac530a2

slarse mentioned this pull request Oct 29, 2020

refactor: Run all checks in one pass in warnings miner #175

Merged

slarse added ready-for-review and removed WiP labels Oct 29, 2020

fermadeiral merged commit 1961033 into ASSERT-KTH:master Oct 29, 2020

slarse deleted the issue/89-update-to-sonar-java-6.9 branch November 11, 2020 09:51

algomaster99 reviewed Dec 7, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Update to sonar-java 6.9.0 #156

chore: Update to sonar-java 6.9.0 #156

slarse commented Oct 19, 2020 •

edited

Loading

slarse commented Oct 19, 2020

slarse commented Oct 19, 2020

slarse commented Oct 20, 2020 •

edited

Loading

fermadeiral commented Oct 20, 2020

slarse commented Oct 20, 2020

slarse commented Oct 20, 2020 •

edited

Loading

slarse commented Oct 20, 2020

slarse commented Oct 21, 2020 •

edited

Loading

codecov bot commented Oct 23, 2020 •

edited

Loading

slarse left a comment

slarse Oct 23, 2020

slarse Oct 23, 2020

slarse Oct 23, 2020

fermadeiral commented Oct 28, 2020

slarse commented Oct 28, 2020

fermadeiral commented Oct 29, 2020

slarse commented Oct 29, 2020

fermadeiral commented Oct 29, 2020

algomaster99 Dec 7, 2021 •

edited

Loading

slarse Dec 7, 2021

chore: Update to sonar-java 6.9.0 #156

chore: Update to sonar-java 6.9.0 #156

Conversation

slarse commented Oct 19, 2020 • edited Loading

slarse commented Oct 19, 2020

slarse commented Oct 19, 2020

slarse commented Oct 20, 2020 • edited Loading

fermadeiral commented Oct 20, 2020

slarse commented Oct 20, 2020

slarse commented Oct 20, 2020 • edited Loading

slarse commented Oct 20, 2020

slarse commented Oct 21, 2020 • edited Loading

codecov bot commented Oct 23, 2020 • edited Loading

Codecov Report

slarse left a comment

Choose a reason for hiding this comment

slarse Oct 23, 2020

Choose a reason for hiding this comment

slarse Oct 23, 2020

Choose a reason for hiding this comment

slarse Oct 23, 2020

Choose a reason for hiding this comment

fermadeiral commented Oct 28, 2020

slarse commented Oct 28, 2020

fermadeiral commented Oct 29, 2020

slarse commented Oct 29, 2020

fermadeiral commented Oct 29, 2020

algomaster99 Dec 7, 2021 • edited Loading

Choose a reason for hiding this comment

slarse Dec 7, 2021

Choose a reason for hiding this comment

slarse commented Oct 19, 2020 •

edited

Loading

slarse commented Oct 20, 2020 •

edited

Loading

slarse commented Oct 20, 2020 •

edited

Loading

slarse commented Oct 21, 2020 •

edited

Loading

codecov bot commented Oct 23, 2020 •

edited

Loading

algomaster99 Dec 7, 2021 •

edited

Loading