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.

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

Static code analyzer improvement #145

Merged
merged 8 commits into from
Mar 30, 2020
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ jobs:
command: ./gradlew javadoc
- run:
name: Run static code analyzer
command: ./gradlew spotbugsMain
command: ./gradlew checkstyleMain
PawelLipski marked this conversation as resolved.
Show resolved Hide resolved

- run:
name: Compile tests
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -261,7 +261,7 @@ private Optional<IGitCoreLocalBranch> getCoreBranchFromName(String branchName) {
}

@AllArgsConstructor(staticName = "of")
static private class GitMacheteBranchData {
private static class GitMacheteBranchData {
final IGitMacheteCommit pointedCommit;
final List<IGitMacheteCommit> commits;
final SyncToOriginStatus syncToOriginStatus;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,9 @@ public void save(IBranchLayout branchLayout, boolean backupOldFile) throws IOExc
var lines = printBranchesOntoStringList(branchLayout.getRootBranches(), 0);

if (backupOldFile) {
var pathToBackupFile = path.getParent().resolve(path.getFileName() + "~");
Path parentDir = path.getParent();
assert parentDir != null : "Can't get parent dir of branch relation file";
var pathToBackupFile = parentDir.resolve(path.getFileName() + "~");
Files.copy(path, pathToBackupFile, StandardCopyOption.REPLACE_EXISTING);
}

Expand Down
31 changes: 16 additions & 15 deletions build.gradle
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
plugins {
id 'org.gradle.java-library'

id 'checkstyle'
micpiotrowski marked this conversation as resolved.
Show resolved Hide resolved
id 'com.diffplug.gradle.spotless' version '3.27.2' apply false
id 'com.github.spotbugs' version '3.0.0' apply false
id 'io.freefair.lombok' version '4.1.6' apply false
id 'org.checkerframework' version '0.4.13' apply false
id 'org.jetbrains.intellij' version '0.4.17' apply false
Expand All @@ -27,18 +27,14 @@ task resolveDependencies {
}

subprojects {
def PLUGIN_VERSION = '0.0.39-SNAPSHOT'
def PLUGIN_VERSION = '0.0.40-SNAPSHOT'

group 'com.virtuslab'
version PLUGIN_VERSION

repositories {
jcenter()
mavenCentral()
// For spotbugs
maven {
url 'https://plugins.gradle.org/m2/'
}
}

apply plugin: 'org.gradle.java-library'
Expand Down Expand Up @@ -66,13 +62,11 @@ subprojects {
}
}

apply plugin: 'com.github.spotbugs'
spotbugs {
apply plugin: 'checkstyle'
checkstyle {
// ignoreFailures here because we want to set conditions that will cause build fail in violations plugin
PawelLipski marked this conversation as resolved.
Show resolved Hide resolved
ignoreFailures = true
effort = 'min'
showProgress = true
reportLevel = 'low'
ignoreFailures = false
checkstyleTest.enabled = false
}

apply plugin: 'se.bjurr.violations.violations-gradle-plugin'
Expand All @@ -93,11 +87,13 @@ subprojects {
// This is mandatory regardless of if you want to report violations between revisions or the entire repo.
// Many more formats available, see: https://github.com/tomasbjerre/violations-lib
violations = [
['FINDBUGS', buildDir.path, '.*/spotbugs/.*\\.xml\$', 'Spotbugs']
['CHECKSTYLE', buildDir.path, '.*/checkstyle/.*\\.xml\$', 'Checkstyle']
]
}
// Run violations task after each spotbugsMain run
spotbugsMain.finalizedBy(violations)

// Run violations task after each checkstyleMain run
checkstyleMain.finalizedBy(violations)


ext {
guiceVersion = '4.2.2'
Expand All @@ -107,6 +103,7 @@ subprojects {
powerMockVersion = '2.0.5'
slf4jVersion = '1.7.25'
vavrVersion = '0.10.2'
checkstyleVersion = '8.30'
micpiotrowski marked this conversation as resolved.
Show resolved Hide resolved

// For frontend modules other than `frontendUi`, we only use the Intellij plugin to provide dependencies,
// but don't want the associated tasks to be available.
Expand All @@ -122,6 +119,10 @@ subprojects {
verifyPlugin.enabled = false
}

dependencies {
implementation group: 'com.puppycrawl.tools', name: 'checkstyle', version: checkstyleVersion
micpiotrowski marked this conversation as resolved.
Show resolved Hide resolved
}

guice = { ->
dependencies {
implementation group: 'com.google.inject', name: 'guice', version: guiceVersion
Expand Down
73 changes: 73 additions & 0 deletions config/checkstyle/checkstyle.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
"-//Checkstyle//DTD Checkstyle Configuration 1.3//EN"
"https://checkstyle.org/dtds/configuration_1_3.dtd">

<!-- Examples from this file: https://github.com/checkstyle/checkstyle/blob/master/src/main/resources/google_checks.xml -->

<!-- List of directives: https://checkstyle.sourceforge.io/version/4.4/availablechecks.html -->

<module name="Checker">
<module name="NewlineAtEndOfFile"/>
<module name="TreeWalker">
<module name="AvoidStarImport"/>
PawelLipski marked this conversation as resolved.
Show resolved Hide resolved
<module name="ConstantName"/>
<module name="EmptyBlock"/>
micpiotrowski marked this conversation as resolved.
Show resolved Hide resolved
<module name="EqualsHashCodeCheck"/>
micpiotrowski marked this conversation as resolved.
Show resolved Hide resolved
<module name="EmptyBlock">
<property name="option" value="TEXT"/>
<property name="tokens"
value="LITERAL_TRY, LITERAL_FINALLY, LITERAL_IF, LITERAL_ELSE, LITERAL_SWITCH"/>
</module>
<module name="NeedBraces">
<property name="tokens"
value="LITERAL_DO, LITERAL_ELSE, LITERAL_FOR, LITERAL_IF, LITERAL_WHILE"/>
</module>
<module name="LeftCurly">
<property name="tokens"
value="ANNOTATION_DEF, CLASS_DEF, CTOR_DEF, ENUM_CONSTANT_DEF, ENUM_DEF,
INTERFACE_DEF, LAMBDA, LITERAL_CASE, LITERAL_CATCH, LITERAL_DEFAULT,
LITERAL_DO, LITERAL_ELSE, LITERAL_FINALLY, LITERAL_FOR, LITERAL_IF,
LITERAL_SWITCH, LITERAL_SYNCHRONIZED, LITERAL_TRY, LITERAL_WHILE, METHOD_DEF,
OBJBLOCK, STATIC_INIT"/>
</module>
<module name="RightCurly">
<property name="id" value="RightCurlySame"/>
<property name="tokens"
value="LITERAL_TRY, LITERAL_CATCH, LITERAL_FINALLY, LITERAL_IF, LITERAL_ELSE,
LITERAL_DO"/>
</module>
<module name="OneStatementPerLine"/>
<module name="MultipleVariableDeclarations"/>
<module name="ArrayTypeStyle"/>
<module name="MissingSwitchDefault"/>
<module name="FallThrough"/>
<module name="UpperEll"/>
<module name="ModifierOrder"/>
<module name="CovariantEquals"/>
<module name="EmptyStatement"/>
<module name="FallThrough"/>
micpiotrowski marked this conversation as resolved.
Show resolved Hide resolved
<module name="HiddenField">
<property name="ignoreConstructorParameter" value="true"/>
</module>
<module name="IllegalThrows"/>
<module name="IllegalType"/>
<module name="InnerAssignment"/>
<module name="MultipleStringLiterals"/>
<module name="MultipleVariableDeclarations"/>
<module name="NoWhitespaceAfter"/>
<module name="NoWhitespaceBefore"/>
<module name="PackageDeclaration"/>
<module name="ParameterAssignment"/>
<module name="RedundantImport"/>
micpiotrowski marked this conversation as resolved.
Show resolved Hide resolved
<module name="RedundantModifier"/>
<module name="StringLiteralEquality"/>
<module name="SuperClone"/>
<module name="UnnecessaryParentheses"/>
<module name="UnusedImports"/>
</module>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd also add: DefaultComesLast, DoubleCheckedLocking, ExplicitInitialization, FinalClass, FinalStatic, HideUtilityClassConstructor, IllegalCatch, IllegalInstantiation (for some of our classes that define of factory methods maybe?), InterfaceIsType, JUnitTestCase, MissingCtor, ModifiedControlVariable, MutableException, NestedTryDepth, RedundantThrows, StringLiteralEquality

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DoubleCheckedLocking was removed in 5.6 - I provided wrong link in top comment and source code (to old version) - corrected:

Removed the DoubleCheckedLocking check, as in Java 5 (and beyond), using the volatile keyword addresses the issue.
More details: https://checkstyle.sourceforge.io/releasenotes_old.html#Release_5.6

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ExplicitInitialization - I'm not sure if we want to use this - sometime it's useful to explicite initialize some fields to emphasize something. @mkondratek, @PawelLipski WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FinalStatic - as with DoubleCheckedLocking there is not that check in current version

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ExplicitInitialization Oh ok, I thought it requires us to use = null everywhere, not forbids ;D so skip it then

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

MissingCtor - we must take in consideration that checkstyle unfortunately doesn't very well cooperate with lombok - if at all - for example it doesn't see c'tors generated by lombok so this check blame all classes marked with @Data annotation.
It's seems like checkstyle doesn't check code after delombok phase but only the original one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

RedundantThrows - also unavailable in this version

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ MissingCtor - oh ok that's unlike e.g. Checker Framework... anyway, ofc let's leave this one out as well, that's not critical in any way

Copy link
Contributor Author

Choose a reason for hiding this comment

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

StringLiteralEquality - is already added to checks :)


PawelLipski marked this conversation as resolved.
Show resolved Hide resolved
<module name="SuppressionFilter">
micpiotrowski marked this conversation as resolved.
Show resolved Hide resolved
<property name="file" value="${config_loc}/suppressions.xml" />
</module>
</module>
micpiotrowski marked this conversation as resolved.
Show resolved Hide resolved
12 changes: 12 additions & 0 deletions config/checkstyle/suppressions.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
<?xml version="1.0"?>

<!DOCTYPE suppressions PUBLIC
"-//Checkstyle//DTD SuppressionFilter Configuration 1.2//EN"
"https://checkstyle.org/dtds/suppressions_1_2.dtd">

<suppressions>
<suppress checks="ConstantName" files="BackendFactoryModule\.java" lines="12" />
PawelLipski marked this conversation as resolved.
Show resolved Hide resolved
<suppress checks="ConstantName" files="RepositoryGraphFactory\.java" lines="15" />
<suppress checks="ConstantName" files="RepositoryGraph\.java" lines="25" />
<suppress checks="ConstantName" files="NullRepository\.java" lines="9" />
</suppressions>
micpiotrowski marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -7,23 +7,23 @@
public final class ColorDefinitions {
private ColorDefinitions() {}

private static final Color red = Color.decode("#FF0000");
private static final Color orange = Color.decode("#FDA909");
private static final Color dark_orange = Color.decode("#D68C00");
private static final Color yellow = Color.decode("#C4A000");
private static final Color green = Color.decode("#008000");
private static final Color gray = Color.decode("#AAAAAA");
private static final Color dark_gray = Color.decode("#555555");
private static final Color transparent = new Color(0, 0, 0, 0);
private static final Color RED_COLOR = Color.decode("#FF0000");
private static final Color ORANGE_COLOR = Color.decode("#FDA909");
private static final Color DARK_ORANGE_COLOR = Color.decode("#D68C00");
private static final Color YELLOW_COLOR = Color.decode("#C4A000");
private static final Color GREEN_COLOR = Color.decode("#008000");
private static final Color GRAY_COLOR = Color.decode("#AAAAAA");
private static final Color DARK_COLOR = Color.decode("#555555");
private static final Color TRANSPARENT_COLOR = new Color(0, 0, 0, 0);

/**
* {@link com.intellij.ui.JBColor} are pairs of colors for light and dark IDE theme. LHS colors names are their
* "general" description. They may differ from RHS (which are actual and specific colors).
*/
public static final JBColor RED = new JBColor(red, red);
public static final JBColor ORANGE = new JBColor(dark_orange, orange);
public static final JBColor YELLOW = new JBColor(yellow, yellow);
public static final JBColor GREEN = new JBColor(green, green);
public static final JBColor GRAY = new JBColor(dark_gray, gray);
public static final JBColor TRANSPARENT = new JBColor(transparent, transparent);
public static final JBColor RED = new JBColor(RED_COLOR, RED_COLOR);
public static final JBColor ORANGE = new JBColor(DARK_ORANGE_COLOR, ORANGE_COLOR);
public static final JBColor YELLOW = new JBColor(YELLOW_COLOR, YELLOW_COLOR);
public static final JBColor GREEN = new JBColor(GREEN_COLOR, GREEN_COLOR);
public static final JBColor GRAY = new JBColor(DARK_COLOR, GRAY_COLOR);
public static final JBColor TRANSPARENT = new JBColor(TRANSPARENT_COLOR, TRANSPARENT_COLOR);
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,14 @@ public final class GraphEdgeColorToJBColorMapper {

private GraphEdgeColorToJBColorMapper() {}

private static final Map<GraphEdgeColor, JBColor> colors = Map.of(
private static final Map<GraphEdgeColor, JBColor> COLORS = Map.of(
GraphEdgeColor.GRAY, ColorDefinitions.GRAY,
GraphEdgeColor.YELLOW, ColorDefinitions.YELLOW,
GraphEdgeColor.RED, ColorDefinitions.RED,
GraphEdgeColor.GREEN, ColorDefinitions.GREEN);

public static JBColor getColor(GraphEdgeColor graphEdgeColor) {
return colors.getOrDefault(graphEdgeColor, ColorDefinitions.TRANSPARENT);
return COLORS.getOrDefault(graphEdgeColor, ColorDefinitions.TRANSPARENT);
}

public static JBColor getColor(int colorId) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,13 @@
public final class SyncToOriginStatusToTextColorMapper {
private SyncToOriginStatusToTextColorMapper() {}

private static final Map<Integer, JBColor> colors = Map.of(
private static final Map<Integer, JBColor> COLORS = Map.of(
Untracked.getId(), ORANGE,
Ahead.getId(), RED,
Behind.getId(), RED,
Diverged.getId(), RED);

public static JBColor getColor(int statusId) {
return colors.getOrDefault(statusId, JBColor.GRAY);
return COLORS.getOrDefault(statusId, JBColor.GRAY);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,13 @@
public final class SyncToParentStatusToGraphEdgeColorMapper {
private SyncToParentStatusToGraphEdgeColorMapper() {}

private static final Map<SyncToParentStatus, GraphEdgeColor> edges = Map.of(
private static final Map<SyncToParentStatus, GraphEdgeColor> EDGES = Map.of(
SyncToParentStatus.Merged, GraphEdgeColor.GRAY,
SyncToParentStatus.InSyncButForkPointOff, GraphEdgeColor.YELLOW,
SyncToParentStatus.OutOfSync, GraphEdgeColor.RED,
SyncToParentStatus.InSync, GraphEdgeColor.GREEN);

public static GraphEdgeColor getGraphEdgeColor(SyncToParentStatus syncToParentStatus) {
return edges.getOrDefault(syncToParentStatus, GraphEdgeColor.TRANSPARENT);
return EDGES.getOrDefault(syncToParentStatus, GraphEdgeColor.TRANSPARENT);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,13 @@ public BranchElement(
this.attributes = isCurrentBranch ? UNDERLINE_BOLD_ATTRIBUTES : NORMAL_ATTRIBUTES;
}

private static final JBColor branchTextColor = new JBColor(Color.BLACK, Color.WHITE);
private static final JBColor BRANCH_TEXT_COLOR = new JBColor(Color.BLACK, Color.WHITE);

private static final SimpleTextAttributes UNDERLINE_BOLD_ATTRIBUTES = new SimpleTextAttributes(
SimpleTextAttributes.STYLE_UNDERLINE | SimpleTextAttributes.STYLE_BOLD, branchTextColor);
SimpleTextAttributes.STYLE_UNDERLINE | SimpleTextAttributes.STYLE_BOLD, BRANCH_TEXT_COLOR);

private static final SimpleTextAttributes NORMAL_ATTRIBUTES = new SimpleTextAttributes(
SimpleTextAttributes.STYLE_PLAIN, branchTextColor);
SimpleTextAttributes.STYLE_PLAIN, BRANCH_TEXT_COLOR);

@Getter
private final SimpleTextAttributes attributes;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,13 @@
public final class SyncToOriginStatusLabelGenerator {
private SyncToOriginStatusLabelGenerator() {}

private static final Map<Integer, String> labels = Map.of(
private static final Map<Integer, String> LABELS = Map.of(
Untracked.getId(), "untracked",
Ahead.getId(), "ahead of origin",
Behind.getId(), "behind origin",
Diverged.getId(), "diverged from origin");

public static String getLabel(int statusId) {
return labels.getOrDefault(statusId, "sync to origin unknown");
return LABELS.getOrDefault(statusId, "sync to origin unknown");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,15 @@ public class RepositoryGraphFactory {
private RepositoryGraph repositoryGraphWithoutCommits;
private IGitMacheteRepository repository;

public RepositoryGraph getRepositoryGraph(@Nullable IGitMacheteRepository repository, boolean isListingCommits) {
if (repository == null)
public RepositoryGraph getRepositoryGraph(@Nullable IGitMacheteRepository givenRepository, boolean isListingCommits) {
if (givenRepository == null) {
return nullRepositoryGraph;
if (repository != this.repository) {
this.repository = repository;
}

if (givenRepository != this.repository) {
this.repository = givenRepository;

RepositoryGraphBuilder repositoryGraphBuilder = new RepositoryGraphBuilder().repository(repository);
RepositoryGraphBuilder repositoryGraphBuilder = new RepositoryGraphBuilder().repository(givenRepository);
repositoryGraphWithCommits = repositoryGraphBuilder.branchGetCommitsStrategy(DEFAULT_GET_COMMITS).build();
repositoryGraphWithoutCommits = repositoryGraphBuilder.branchGetCommitsStrategy(EMPTY_GET_COMMITS).build();
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package com.virtuslab.gitmachete.frontend.ui.table;
package com.virtuslab.gitmachete.frontend.ui;
micpiotrowski marked this conversation as resolved.
Show resolved Hide resolved

import java.nio.file.Path;
import java.nio.file.Paths;
Expand All @@ -24,6 +24,8 @@
import com.virtuslab.gitmachete.backend.root.GitMacheteRepositoryBuilderFactory;
import com.virtuslab.gitmachete.frontend.graph.repository.RepositoryGraph;
import com.virtuslab.gitmachete.frontend.graph.repository.RepositoryGraphFactory;
import com.virtuslab.gitmachete.frontend.ui.table.GitMacheteGraphTable;
import com.virtuslab.gitmachete.frontend.ui.table.GraphTableModel;

public class GitMacheteGraphTableManager {
private static final Logger LOG = Logger.getInstance(GitMacheteGraphTableManager.class);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,9 @@
import lombok.Getter;

import com.virtuslab.gitmachete.frontend.actions.RebaseCurrentBranchOntoParentAction;
import com.virtuslab.gitmachete.frontend.ui.table.GitMacheteGraphTableManager;

public class GitMachetePanel {
private static final String TOGGLE_LIST_COMMIT_TEXT = "Toggle List Commits";

@Getter
private final GitMacheteGraphTableManager gitMacheteGraphTableManager;
Expand All @@ -30,7 +30,7 @@ public ActionToolbar createGitMacheteToolbar() {
DefaultActionGroup refresh = new DefaultActionGroup("Refresh", /* popup */ false);
refresh.add(new RefreshGitMacheteStatusAction());

DefaultActionGroup toggleListCommits = new DefaultActionGroup("Toggle List Commits", /* popup */ false);
DefaultActionGroup toggleListCommits = new DefaultActionGroup(TOGGLE_LIST_COMMIT_TEXT, /* popup */ false);
toggleListCommits.add(new ToggleListCommitsAction());

DefaultActionGroup rebaseCurrentBranchOntoParent = new DefaultActionGroup("Rebase Current Branch Onto Parent",
Expand Down Expand Up @@ -58,7 +58,7 @@ public void actionPerformed(AnActionEvent e) {

private class ToggleListCommitsAction extends ToggleAction implements DumbAware {
ToggleListCommitsAction() {
super("Toggle List Commits", "Toggle list commits", AllIcons.Actions.ShowHiddens);
super(TOGGLE_LIST_COMMIT_TEXT, TOGGLE_LIST_COMMIT_TEXT, AllIcons.Actions.ShowHiddens);
PawelLipski marked this conversation as resolved.
Show resolved Hide resolved
}

@Override
Expand Down