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

Apply spotbugs plugin #2005

Merged
merged 2 commits into from
Sep 14, 2023
Merged

Apply spotbugs plugin #2005

merged 2 commits into from
Sep 14, 2023

Conversation

cmnrd
Copy link
Collaborator

@cmnrd cmnrd commented Sep 13, 2023

This PR applies the spotbugs gradle plugin. spotbugs is a static code analysis tool that can detect a lot of bug patterns in Java (and Kotlin).

This is a selection of the type of issues reported by spotbugs:

H V MS: org.lflang.generator.GeneratorResult.NOTHING isn't final but should be  At GeneratorResult.java:[line 14]
H C HE: org.lflang.TimeValue doesn't define a hashCode() method but it is used in a hashed context in org.lflang.federated.generator.FederateInstance.stpToNetworkActionMap  In FederateInstance.java
M P UuF: Unused field: org.lflang.analyses.c.CToUclidVisitor.unchangedTriggers  In CToUclidVisitor.java
M V EI2: new org.lflang.federated.generator.FedEmitter(FedFileConfig, Reactor, MessageReporter, RtiConfig) may expose internal representation by storing an externally mutable object into FedEmitter.messageReporter  At FedEmitter.java:[line 29]
M D NP: Possible null pointer dereference in org.lflang.generator.cpp.CppStandaloneGenerator.generatePlatformFiles() due to return value of called method  Dereferenced at CppStandaloneGenerator.kt:[line 38]
M D BC: Questionable cast from Collection to abstract class java.util.List in org.lflang.generator.cpp.CppStandaloneGenerator.compareVersion(String, String)  At CppStandaloneGenerator.kt:[line 187]
M B FS: Format string should use %n rather than \n in org.lflang.ast.FormattingUtil.lineWrapComment(String, int, String)  At FormattingUtil.java:[line 172]
M P SBSC: org.lflang.analyses.uclid.UclidGenerator.generateActionAxioms() concatenates strings using + in a loop  At UclidGenerator.java:[line 987]

Spotbugs is run automatically as part of the check and build gradle tasks. The plugin is currently configured to treat all issues that it finds as warnings, not errors. Thus, the build succeeds although spotbugs reports problems.

Fixing all issues reported by spotbugs will keep us busy for a while. To keep this manageable, I think we should fix warnings in several smaller PRs and treat found issues as errors as soon as we have cleaned up the codebase.

This is a first step towards resolving #1806.

@cmnrd cmnrd mentioned this pull request Sep 13, 2023
@lhstrh
Copy link
Member

lhstrh commented Sep 13, 2023

This is a great idea. Let's go ahead and fix those bugs/warnings!

@cmnrd cmnrd marked this pull request as ready for review September 14, 2023 12:57
@cmnrd cmnrd requested a review from lhstrh September 14, 2023 12:57
@cmnrd
Copy link
Collaborator Author

cmnrd commented Sep 14, 2023

I spend some time today fixing issues found by spotbugs. Some are quite easy to fix, but for others the actual issue is not so obvious and the problem is actually a bad design. Thus, spotbugs actually helps to improve the design by a lot, but it also takes a while to fix the reported issues.

I think that fixing all issues as part of this PR is not feasible. It would create a humongous PR that would be difficult to keep up to date and merge. Thus, I advocate for merging this in now, treating the reported issues as warnings. Then we can fix issues in independent PRs and eventually switch back to treating issues as errors once we cleaned up the code base.

@lhstrh
Copy link
Member

lhstrh commented Sep 14, 2023

Sounds like the right approach to me as well.

@cmnrd cmnrd added this pull request to the merge queue Sep 14, 2023
Merged via the queue into master with commit 30271bd Sep 14, 2023
@cmnrd cmnrd deleted the spotbugs branch September 14, 2023 18:58
@lhstrh lhstrh added the cleanup label Jan 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants